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

go-vet: assignment copies lock value of tls.Config #254

Closed
iaburton opened this issue Mar 28, 2019 · 3 comments · Fixed by #255
Closed

go-vet: assignment copies lock value of tls.Config #254

iaburton opened this issue Mar 28, 2019 · 3 comments · Fixed by #255

Comments

@iaburton
Copy link
Contributor

While there are some minor linting issues, I noticed go-vet pointing out

assignment copies lock value to conf: crypto/tls.Config contains sync.Once contains sync.Mutex

Here:

go-nsq/conn.go

Line 390 in 61f49c0

conf = *tlsConf

Since the tls.Config is an option in the nsq.Config, you can't be certain it isn't already being used, and therefore synchronization of the Once/Mutex are lost in the copied value. Depending on the minimum Go version this package supports, the solution might be as simple as Cloning the tls.Config with the provided method added in Go1.8 https://golang.org/pkg/crypto/tls/#Config.Clone

@ploxiln
Copy link
Member

ploxiln commented Mar 28, 2019

We don't try to support go < 1.8, Clone() will do fine. Want to open a pull request with that change?

iaburton added a commit to iaburton/go-nsq that referenced this issue Mar 28, 2019
@iaburton
Copy link
Contributor Author

Done 👍

@ploxiln
Copy link
Member

ploxiln commented Mar 28, 2019

closing in favor of #255

@ploxiln ploxiln closed this as completed Mar 28, 2019
ploxiln pushed a commit to iaburton/go-nsq that referenced this issue Dec 24, 2019
also move tls.Config clone after net.SplitHostPort

closes nsqio#254
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

Successfully merging a pull request may close this issue.

2 participants