webrtc/: Add message framing to support half-close and reset of stream#1
Conversation
The WebRTC browser APIs do not support half-closing nor resets of streams. This commit defines a message framing schema to support this functionality on top of the browser APIs.
|
For those that want to play around with this, framing size test is here mxinden/rust-libp2p@59617de. |
|
I implemented this proposal on top of libp2p/rust-libp2p#2622. See https://github.com/mxinden/rust-libp2p/tree/webrtc-message-framing. (Happy path only.) |
|
JS implementation of this proposal libp2p/js-libp2p-webrtc#20 from @John-LittleBearLabs. |
| // The sender will no longer read messages. | ||
| CLOSE_READ = 1; | ||
| // The local endpoint abruptly terminates the stream. The remote endpoint | ||
| // may discard any in-flight data. |
There was a problem hiding this comment.
Let's not mix this up with the read side of the stream. RESET only resets write side, NOT the read side.
There was a problem hiding this comment.
@John-LittleBearLabs note the above change in semantics. Please comment in case you have questions / concerns.
| additional multiplexing. | ||
| Following [Connection Security](#connection-security). | ||
|
|
||
| The WebRTC browser APIs do not support half-closing nor resets of streams. |
There was a problem hiding this comment.
just so we're on the same page. when you're talking about reseting a stream, you're not talking about https://datatracker.ietf.org/doc/html/draft-ietf-rtcweb-data-channel-13#section-6.7 (webrtc signalling mechanism to tell remote that data channel was closed), right?
There was a problem hiding this comment.
The above description is misleading. Thanks for the note. I pushed a60234c updating the section.
WebRTC's reset as well as the corresponding RTCDataChannel.close() reset both sending and receiving. We would like to support the former without the latter.
All that said, in the end I don't see this mechanism as particularly useful. I expect readers to read messages from the stream in order, and not do any buffering to the application above. Thus a receiver would receive the RESET after having passed all previous messages to the application already. Thus acting just as if it received a FIN flag, i.e. closing its read side, but not discarding any messages.
I don't have a strong opinion on adding RESET. I guess it doesn't hurt and might enable future optimizations.
The above is not to be generalized to the entire proposal. While I don't see much value in the RESET flag, there is value in being able to half-close a stream via the FIN and STOP_SENDING flag.
Does the above make sense @melekes? These are rather subtle semantics. Please comment in case my explanations and reasoning in the proposal don't make sense to you.
There was a problem hiding this comment.
Thanks for the clarification 👍 The proposed framing looks good to me. Regarding RESET - depending on the implementation there could be some internal buffering. This buffer could be erased once RESET is received. Not sure how useful it would be in practice though.
Instead of resetting both ends of a stream, RESET only resets the sending side of a stream. This is in line with the QUIC stream reset mechanism.
|
@marten-seemann thanks for the review. I addressed your comments. Do you want to take another look? |
|
Unless there are any objections, I will merge here end of the week. |
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Looks good to me. Found two typos :)
marten-seemann
left a comment
marten-seemann
left a comment
There was a problem hiding this comment.
Do we need to specify what happens when receiving a STOP_SENDING? If we want to go in parallel with RFC 9000 we'd need to say that you send a RESET.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
|
I was not aware of this convention @marten-seemann. Thanks for pointing it out. I think it makes sense to be consistent with RFC 9000. 31ac65d includes the convention for our adaptation of the QUIC multiplexer. |
For the sake of consistency with the QUIC RFC, rename RESET to RESET_STREAM.
Instead of executing the additional Noise handshake on a plain `RTCDataChannel`, frame the Noise messages with the proposed message framing mechanism already. Benefits: - The additional Noise handshake can already make use of the proper closing mechanism. Note that `RTCDataChannel.close` may drop any in-flight data. See https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/close. - Simplifies implementations as there is only one way to write on an `RTCDataChannel`. - Allows us to role out new versions of the Noise handshake in the future by adding a version to the message framing Protobuf.
|
Bringing 7a8ebc0 to everyone's attention here:
Let me know in case you see any issues with this. |
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
|
I am going to merge here. In case you have any follow-ups please raise them on libp2p#412. |
|
Moving a question from @melekes on melekes/rust-libp2p#8 (comment) here.
My rational for introducing it in this pull request was for the sake of consistency with the QUIC specification. I am assuming this can be useful for applications leveraging the acknowledgement of the closing of the read side. I can not find any past discussions on the matter @marten-seemann do you know more? |
In QUIC it's needed to have an accurate accounting of the number of bytes sent for connection-level flow control purposes (RESET_STREAM contains the final offset of that stream). How does flow control work in WebRTC? Is it purely based on streams? Then we probably don't need to send a RESET_STREAM. |
Each data channel is a SCTP stream (http://www.myreadingroom.co.in/notes-and-studymaterial/68-dcn/858-flow-control-in-sctp.html). |
|
Thanks @marten-seemann and @melekes. Removed requirement with libp2p@65a894a. //CC @ckousik and @John-LittleBearLabs |
4 participants
Note this pull request targets libp2p#412 and not the master branch.
The WebRTC browser APIs do not support half-closing nor resets of streams. This
commit defines a message framing schema to support this functionality on top of
the browser APIs.
This is a first proposal. This has not yet been implemented and will thus most likely change.
Input very much appreciated.
See libp2p#412 (comment) and libp2p#412 (comment) for past discussions on this topic.