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

Comments

@dvyukov
Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2013

Comment 1:

Labels changed: removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 2:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 3:

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 4:

Labels changed: added repo-main.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

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

@aclements

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

@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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

I have a different fix for #17953.

@agnivade

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

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

@dvyukov

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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
Projects
None yet
6 participants
You can’t perform that action at this time.