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

fix race w/ *Conn.requestTimeout #112

Closed
wants to merge 1 commit into from

Conversation

vetinari
Copy link
Contributor

@vetinari vetinari commented May 5, 2017

conn.go Outdated
l.requestTimeout = timeout
l.timeoutMutex.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be simpler to make this an atomic value and do lockless load/store. We already have a shim for < go1.3 using locks

@vetinari
Copy link
Contributor Author

vetinari commented May 8, 2017

using the atomicValue struct triggered the race during travis build...

@liggitt
Copy link
Contributor

liggitt commented May 8, 2017

using the atomicValue struct triggered the race during travis build...

that's concerning... looks like the 1.3 version needs to hold a pointer to the lock

@vetinari
Copy link
Contributor Author

vetinari commented May 8, 2017

1.3 was ok, it hit 1.5: https://travis-ci.org/go-ldap/ldap/builds/229842330, the 1.3 failure was a network fetch error while setting up: https://travis-ci.org/go-ldap/ldap/builds/229846728

@liggitt
Copy link
Contributor

liggitt commented May 9, 2017

hmm, that's still concerning, but the atomic load/store int64 is nice and clean and sidesteps that issue. go ahead and squash and remove the unused mutex, then LGTM

@vetinari
Copy link
Contributor Author

hmm... https://travis-ci.org/go-ldap/ldap/jobs/230628746 ... what does the race detector find? concurrent r/w access to the *Conn struct's members?

@liggitt liggitt mentioned this pull request Aug 24, 2017
This was referenced Sep 23, 2017
@johnweldon
Copy link
Member

@vetinari I think this is handled by the recent merge #134

@vetinari
Copy link
Contributor Author

fixed by #134

@vetinari vetinari closed this Sep 25, 2017
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 this pull request may close these issues.

3 participants