Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sync: detect copying of Mutex #6188

Open
dvyukov opened this issue Aug 19, 2013 · 13 comments
Open

sync: detect copying of Mutex #6188

dvyukov opened this issue Aug 19, 2013 · 13 comments
Milestone

Comments

@dvyukov
Copy link
Member

dvyukov commented Aug 19, 2013

Discussion on golang-dev:
https://groups.google.com/forum/#!searchin/golang-dev/copying$20sync.mutex/golang-dev/3jCp3vd4BQ8/dqjjePtV8iwJ

sync.Mutex can use the same technique as sync.Cond to detect copying.

Package docs says "Values containing the types defined in this package should not
be copied".

It will also help to simplify runtime code and make Mutexes faster.

Brad raised the concern that it will increase size of Mutex.
@robpike
Copy link
Contributor

robpike commented Aug 20, 2013

Comment 1:

Labels changed: removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 2:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 3:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 4:

Labels changed: added repo-main.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@bradfitz
Copy link
Contributor

bradfitz commented Feb 2, 2017

There's a vet check for this nowadays (copylock), and #18084 is about making vet always run during tests.

@dvyukov, do you still want to do this?

/cc @minux @josharian @aclements

@dvyukov
Copy link
Member Author

dvyukov commented Feb 2, 2017

This would help to resolve #17953 because if we detect copying we can have per-mutex wait list.

@aclements
Copy link
Member

@dvyukov, given that copying is likely to break a mutex anyway and we have a vet check, can't we just go ahead and do the per-mutex wait list? Why should that be blocked on having an internal copy check?

@dvyukov
Copy link
Member Author

dvyukov commented Feb 2, 2017

@rsc required the runtime check for sync.Cond when I did per-Cond waitlist. But that was before the vet check.

@rsc
Copy link
Contributor

rsc commented Feb 10, 2017

I have a different fix for #17953.

@agnivade
Copy link
Contributor

agnivade commented Jul 2, 2019

@dvyukov - Just wanted to check up on this. #17953 is closed now. Is this still required ?

@dvyukov
Copy link
Member Author

dvyukov commented Jul 2, 2019

Do we detect copying of sync.Mutex now? As far as I see #17953 was about a different thing.
If we do detect, then we can remove the additional runtime checking (if we still have it).

@agnivade
Copy link
Contributor

agnivade commented Jul 2, 2019

Forgive me if I have misunderstood something. I was just following the original thread of discussion. Brad suggested that there is a vet check which detects copying of sync.Mutex which is run as part of tests. And then you mentioned that it would still help to resolve #17953. And now, #17953 is resolved using a different fix.

So, do we still need this now ?

@dvyukov
Copy link
Member Author

dvyukov commented Jul 2, 2019

Depends on if we detect copying of sync.Mutex or not. If we do, then this can be considered fixed after we check if this part is still relevant:

It will also help to simplify runtime code and make Mutexes faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants