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

Fixed deadlock on acceptCh #34

Closed
wants to merge 1 commit into from
Closed

Fixed deadlock on acceptCh #34

wants to merge 1 commit into from

Conversation

enobufs
Copy link
Member

@enobufs enobufs commented Apr 3, 2019

Resolves #30
This fixes AcceptStream() dead-lock.

I also confirmed this fixes pion/webrtc#578

@enobufs enobufs added the review label Apr 3, 2019
@enobufs enobufs requested a review from Sean-Der April 3, 2019 01:23
@enobufs enobufs self-assigned this Apr 3, 2019
@enobufs enobufs mentioned this pull request Apr 3, 2019
@enobufs enobufs requested a review from backkem April 3, 2019 06:58
Copy link
Member

@backkem backkem left a comment

Choose a reason for hiding this comment

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

Ok, so the problem seems to be:

  • There is a new stream incoming.
  • Nothing is listening on acceptCh since the caller of AcceptStream is still processing the previous incoming stream.
  • Processing of the previous incoming stream is blocked because createStream holds the SCTP lock while sending on acceptCh.

Regarding the current solution: This does have the disadvantage that it breaks up the atomicity of handleChunk. Maybe this can lead to unexpected behavior?

Possible alternatives:

  • Send on acceptCh asynchronously. This does mean the order of opening channels is lost. That doesn't seem like a big loss thought.
  • Buffer the incoming streams and notify AcceptStream asynchronously, similar to readNotifier.

@enobufs
Copy link
Member Author

enobufs commented Apr 3, 2019

My apologies. This does not fix the problem as I just saw the dead-lock with this change happening. Let me close this PR. I misunderstand how datachannel was waiting on the AcceptStream earlier. The datachannel IS waiting on the acceptCh properly even when the deadlock occurs. I just don't see new data for the new stream coming in when that happens. I will continue to look for why.

@enobufs enobufs closed this Apr 3, 2019
@enobufs enobufs removed the review label Apr 3, 2019
@Sean-Der Sean-Der deleted the issue-30 branch February 10, 2020 01:50
@Sean-Der Sean-Der restored the issue-30 branch February 10, 2020 01:51
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.

TestDataChannel_Send occasionally fails (Possible) Deadlock found by pions/webrtc CI
2 participants