rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

feat(webrtc): implement fin ack

Open tesol2y090 opened this issue 1 year ago • 3 comments

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
  • https://github.com/libp2p/rust-libp2p/issues/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

  • [x] 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
  • [x] 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 avatar Nov 24 '24 14:11 tesol2y090

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

elenaf9 avatar Dec 10 '24 11:12 elenaf9

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

mergify[bot] avatar Feb 16 '25 16:02 mergify[bot]

Regarding whether poll_close should wait for the FIN_ACK or not (see https://github.com/libp2p/rust-libp2p/pull/5687#discussion_r1898455099). 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?

elenaf9 avatar Mar 03 '25 02:03 elenaf9