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

feat(webrtc): implement fin ack #5687

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

tesol2y090
Copy link
Contributor

@tesol2y090 tesol2y090 commented Nov 24, 2024

Description

Implements FIN_ACK support for WebRTC streams to ensure reliable closure handshaking as specified in the WebRTC spec. This adds:

  • FIN_ACK flag handling in state machine
  • Timeout mechanism for FIN_ACK wait
  • Proper state transitions for graceful closure
  • webrtc: support FIN_ACK #4600

Notes & open questions

  • Using Instant for deadline tracking instead of tokio timer to avoid extra dependencies
  • Timeout is set to 10 seconds - should this be configurable?
  • State machine ensures FIN_ACK is only sent/processed in appropriate states

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
    • Added documentation for FIN_ACK related functions
    • Updated state machine documentation
  • I have added tests that prove my fix is effective or that my feature works
    • Added tests for FIN_ACK state transitions
    • Added tests for timeout behavior
    • Added tests for edge cases

@tesol2y090 tesol2y090 changed the title Feat/webrtc fin ack [WIP] Feat/webrtc fin ack Nov 24, 2024
@tesol2y090 tesol2y090 changed the title [WIP] Feat/webrtc fin ack Feat/webrtc fin ack Dec 1, 2024
@tesol2y090 tesol2y090 marked this pull request as ready for review December 1, 2024 08:43
@tesol2y090 tesol2y090 changed the title Feat/webrtc fin ack feat(webrtc): implement fin ack Dec 1, 2024
@elenaf9 elenaf9 self-requested a review December 10, 2024 11:08
@elenaf9 elenaf9 linked an issue Dec 10, 2024 that may be closed by this pull request
@elenaf9
Copy link
Contributor

elenaf9 commented Dec 10, 2024

Thank you for your contribution @tesol2y090 and sorry for the delay here! Will try to give this a review in the next days.

Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here @tesol2y090.

Some first comments.

.expect("to not close twice")
.send(GracefullyClosed {});
// Check if we've received FIN_ACK or deadline has passed
if self.state.fin_ack_received()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this, but don't we need to somehow continue reading here to be able to receive the FIN_ACK?

With AsyncWriteExt for instance a future will be created that only returns after poll_close succeeded i.e. no concurrent call to poll_read happens. With the current impl, I don't think a FIN_ACK will ever be read in that case?

On the other hand, we'd then have to discard the read messages or buffer them. Not sure how this is handled e.g. in QUIC, or in the go implementation.

Copy link
Contributor

@elenaf9 elenaf9 Dec 31, 2024

Choose a reason for hiding this comment

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

So I looked into the QUIC and TCP transports to check how they handle this in their poll_close function.
However, as far as I can tell neither of them wait in poll_close for the sent FIN to be acknowledged in the first place.

Unfortunately I couldn't find any prior discussion about this.

Copy link
Contributor

mergify bot commented Feb 16, 2025

This pull request has merge conflicts. Could you please resolve them @tesol2y090? 🙏

@jxs jxs added the need/author-input Needs input from the original author label Feb 17, 2025
@elenaf9
Copy link
Contributor

elenaf9 commented Mar 3, 2025

Regarding whether poll_close should wait for the FIN_ACK or not (see #5687 (comment)).
As already mentioned, the current implementation won't work when there are blocking calls to close().await, because without a concurrent call to poll_read the corresponding FIN_ACK will never be read and we'll always timeout.

If the read side is closed anyway, we can just read from within poll_close, parse the flags, and discard any other messages analogous to how poll_write is doing it.

If the read side isn't closed yet we have two options:

  1. Don't wait for the FIN_ACK. But that would mean that we wouldn't have the guarantee that all data has been received.
  2. Read from the stream and buffer the incoming messages up to a BUFFER_MAX (I think we can use the existing read_buffer for that?). If the max has been reached before the FIN_ACK we just return same like in case of a timeout.

I think we should go with nr. 2, but I wonder if there are any better options that I didn't think of (cc @dariusc93 @jxs).
What do you think @tesol2y090?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

webrtc: support FIN_ACK
3 participants