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

Jump to bottom

cmd/vet: copylock where sync.Mutex gets immediately reassigned on the following line #20261

Closed
kevinburke opened this issue May 5, 2017 · 6 comments
Closed

cmd/vet: copylock where sync.Mutex gets immediately reassigned on the following line #20261

kevinburke opened this issue May 5, 2017 · 6 comments
Labels
FrozenDueToAge
Milestone
Go1.10

Comments

@kevinburke
Copy link
Contributor

Here's some code that triggers an error in go vet (initially found in go-mgo/mgo#435):

	scopy := *session
	scopy.m = sync.RWMutex{}
	scopy.creds = creds
	s = &scopy

Basically, give me a new object that has all of the same fields as the old one, minus these two that I want to give new values to. This triggers a vet error:

session.go:555: assignment copies lock value to scopy: mgo.Session contains sync.RWMutex

You can see it gets overwritten on the next line. Since scopy is only live in the function, the code seems like it's correct.

We could copy over all of the fields in the struct, but that would be cumbersome as it's pretty large.

type Session struct {
	m                sync.RWMutex
	cluster_         *mongoCluster
	slaveSocket      *mongoSocket
	masterSocket     *mongoSocket
	slaveOk          bool
	consistency      Mode
	queryConfig      query
	safeOp           *queryOp
	syncTimeout      time.Duration
	sockTimeout      time.Duration
	defaultdb        string
	sourcedb         string
	dialCred         *Credential
	creds            []Credential
	poolLimit        int
	bypassValidation bool
}

Wondering if you had thoughts here.
@josharian josharian changed the title vet: copylock false positive where sync.Mutex gets immediately reassigned on the following line cmd/vet: copylock false positive where sync.Mutex gets immediately reassigned on the following line May 5, 2017
@josharian
Copy link
Contributor

Yes, that's a false positive. Fixing vet would involve doing some non-trivial analysis, which I'm not sure we want to do (?). I don't see a workaround other than copying over all the fields in the struct (probably inside a helper method).

cc @robpike @valyala in case they have ideas

@cespare
Copy link
Contributor

cespare commented May 5, 2017

You can change the Session type by putting most of its fields (but not m) in a nested struct. See the fix for the same issue in Regexp.Copy: https://go-review.googlesource.com/c/20841/.

@cespare
Copy link
Contributor

cespare commented May 5, 2017

By the way, I wrote this exact same kind of code originally for Regexp.Copy in https://go-review.googlesource.com/c/16110 and the consensus of the discussion in https://go-review.googlesource.com/c/20841 seemed to be that it's not actually safe. (Or at least, the race detector will complain.) So I don't think it's just a vet issue.

@cespare
Copy link
Contributor

cespare commented May 5, 2017 •
edited

Perhaps the reasoning is: *session reads the mutex possibly concurrent to some write (e.g. Lock) and that's a data race even if the result of the read isn't observed.

@bradfitz bradfitz added this to the Go1.10 milestone May 5, 2017
@valyala
Copy link
Contributor

valyala commented May 7, 2017

IMHO, the copylock check works as intended in this case due to the reason @cespare mentioned above

@robpike
Copy link
Contributor

robpike commented May 7, 2017

I agree, you're copying a lock and that's what it's complaining about.

The simplest workaround would be to make m a pointer, and then say

scopy := *session
scopy.m = &sync.RWMutex{}

although that does allocate more. Alternatively, copy all fields but the mutex in a copy method.

@robpike robpike closed this as completed May 7, 2017
@kevinburke kevinburke changed the title cmd/vet: copylock false positive where sync.Mutex gets immediately reassigned on the following line cmd/vet: copylock where sync.Mutex gets immediately reassigned on the following line May 7, 2017
@golang golang locked and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
7 participants
@bradfitz @josharian @kevinburke @valyala @cespare @robpike @gopherbot