Skip to content

Conversation

@enobufs
Copy link
Member

@enobufs enobufs commented Mar 10, 2019

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

@enobufs enobufs added the review label Mar 10, 2019
@enobufs enobufs changed the title [WIP] Switch from read lock to write lock [WIP] Race in DataChannel.open() Mar 10, 2019
@enobufs enobufs changed the title [WIP] Race in DataChannel.open() [WIP] Fix race in DataChannel.open() Mar 10, 2019
@coveralls
Copy link

coveralls commented Mar 10, 2019

Pull Request Test Coverage Report for Build 2476

  • 2 of 4 (50.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 80.452%

Changes Missing Coverage Covered Lines Changed/Added Lines %
datachannel.go 2 4 50.0%
Files with Coverage Reduction New Missed Lines %
lossy_stream.go 2 81.13%
Totals Coverage Status
Change from base Build 2466: -0.04%
Covered Lines: 4091
Relevant Lines: 5085

💛 - Coveralls

return err
}

d.ReadyState = DataChannelStateOpen
Copy link
Member

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()

Copy link
Member Author

@enobufs enobufs Mar 10, 2019

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:

  1. This method would virtually 100% succeed in production use (write lock will be held in most of times).
  2. Sticking with only one kind of lock is relatively less error-prone.
  3. DataChannel.open() won't be called that often anyway (I wouldn't worry)

Copy link
Member

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 :)

Copy link
Member

@Sean-Der Sean-Der left a 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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants