-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[WIP] Fix race in DataChannel.open() #510
Conversation
Pull Request Test Coverage Report for Build 2476
💛 - Coveralls |
@@ -182,12 +182,12 @@ func (d *DataChannel) open(sctpTransport *SCTPTransport) error { | |||
|
|||
dc, err := datachannel.Dial(d.sctpTransport.association, *d.ID, cfg) | |||
if err != nil { | |||
d.mu.RUnlock() | |||
d.mu.Unlock() | |||
return err | |||
} | |||
|
|||
d.ReadyState = DataChannelStateOpen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this so we just take the writelock for setting d.ReadyState
I just don't want to be wasteful having a write lock for the whole function. If this is a bad idea please push back!
d.mu.RUnlock()
d.mu.Lock()
d.ReadyState = DataChannelStateOpen
d.mu.Unlock()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it wouldn't give us much because:
- This method would virtually 100% succeed in production use (write lock will be held in most of times).
- Sticking with only one kind of lock is relatively less error-prone.
- DataChannel.open() won't be called that often anyway (I wouldn't worry)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A agree with all that, I was overthinking it. More complicated code for no real gain :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great catch here :)
Now it uses write lock in this PR.
No regression in unit tests, race detection nor golangci-lint in my local run.
I noticed the was a race (reported as #509), not caused by this modification.
Resolves #505