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: close webrtc streams without data loss #2073

Merged
merged 17 commits into from
Oct 6, 2023

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Sep 23, 2023

  • Gracefully close streams on muxer shutdown
  • Refactor initiator/recipient flows for clarity
  • Wait for bufferedAmount to be 0 before closing a datachannel
  • Close datachannels on both initiator and recipient
  • Implements FIN_ACK for closing datachannels without data loss

Supersedes #2048

Depends on:

@achingbrain achingbrain requested a review from a team as a code owner September 23, 2023 19:22
@achingbrain achingbrain mentioned this pull request Sep 23, 2023
2 tasks
@achingbrain achingbrain changed the title fix: close webrtc streams fix: wait for webrtc stream buffers to empty before closing Sep 23, 2023
@wemeetagain
Copy link
Member

@achingbrain is this ready for review?

@achingbrain
Copy link
Member Author

@wemeetagain yes!

Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Looks good so far, left some comments.

Chrome does not always send all messages after closing a datachannel
even if `bufferedAmount` is `0` before closing.

This PR adds a `FIN_ACK` message that is sent in reply to a `FIN`
message - because all messages are send in-order, when this is
received we know the remote has received all of our data messages
and it's safe to close the channel.
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Great job on this, one minor comment. lgtm

packages/transport-webrtc/src/stream.ts Outdated Show resolved Hide resolved
Co-authored-by: Chad Nehemiah <chad.nehemiah94@gmail.com>
@achingbrain achingbrain changed the title fix: wait for webrtc stream buffers to empty before closing fix: close webrtc streams without data loss Oct 6, 2023
@achingbrain achingbrain merged commit c97dea0 into master Oct 6, 2023
17 checks passed
@achingbrain achingbrain deleted the fix/webrtc-stream-closing branch October 6, 2023 12:00
@achingbrain achingbrain restored the fix/webrtc-stream-closing branch October 6, 2023 13:15
achingbrain added a commit that referenced this pull request Oct 6, 2023
- Gracefully close streams on muxer shutdown
- Refactor initiator/recipient flows for clarity
- Wait for `bufferedAmount` to be `0` before closing a datachannel
- Close datachannels on both initiator and recipient
- Implements FIN_ACK for closing datachannels without data loss

Supersedes #2048

---------

Co-authored-by: Chad Nehemiah <chad.nehemiah94@gmail.com>
@SgtPooki SgtPooki deleted the fix/webrtc-stream-closing branch November 15, 2023 12:33
This was referenced Jan 18, 2024
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