Skip to content

feat(webrtc): fin-ack functionality #6084

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

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

Conversation

kamuik16
Copy link
Contributor

Description

ref #4600

This may not be the ideal and best solution, always open to suggestions and comments :)

Changes:

  • Added FIN_ACK = 3 flag to WebRTC protobuf message definition
  • Extended state machine with ReadClosedNeedFinAck and WriteSentFinWaitingForAck states
  • Modified stream implementation to automatically send FIN_ACK upon receiving FIN
  • Updated close logic to wait for FIN_ACK before considering write-half closed
  • Added comprehensive tests for FIN_ACK handshake scenarios

Behaviour:

  • When receiving FIN → automatically send FIN_ACK
  • When sending FIN → wait for FIN_ACK before closing write-half
  • Supports simultaneous close from both ends without data loss
  • Maintains backwards compatibility with existing WebRTC implementations

Change checklist

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@elenaf9
Copy link
Member

elenaf9 commented Jun 28, 2025

Thanks for the PR @kamuik16.

There is already #5687 to implement this, did you have a look at that PR? If so, would you mind briefly elaborating the differences? :)

@kamuik16
Copy link
Contributor Author

Thanks for the PR @kamuik16.

There is already #5687 to implement this, did you have a look at that PR? If so, would you mind briefly elaborating the differences? :)

From my perspective, the key differences boil down to three things:

State granularity

My PR (#6084): I modelled every handshake step as its own State variant (e.g. ReadClosedNeedFinAck, WriteSentFinWaitingForAck), so each transition is explicit and exhaustively tested.

#5687: They collapsed “waiting for ACK” into flags on a few core states, cutting the total variants and match-arm boilerplate in half.

ACK injection & driving reads

Mine: I check state.needs_fin_ack() at the top of every poll_* and piggy-back the FIN_ACK whenever the next op arrives—lean but relies on the caller continuing to call read or write.

#5687: They only inject in response to a FIN in the read loop (and once in poll_close), and in poll_close, they loop reads up to a 10s deadline so a simple close().await still pulls in the peer’s ACK.

Timeout handling

Mine: I let the close-barrier error out if no ACK ever comes, without tracking real time.

#5687: They store a 10 s fin_ack_deadline, log a warning on expiry, and force-close—making the timeout behavior deterministic.

In short, I went for a fully explicit, state-machine-first design; #5687 trades some state complexity for built-in timeout logic and a lightweight read loop in poll_close.

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.

2 participants