transports/websocket: Handle websocket CLOSE with reason code #2085
Conversation
Cleanup
Remove `Binary` and `Text` variants from `Incoming` Add a `Data` enum with `Binary` and `Text` members Move into_bytes and AsRef impls from `Incoming` to `Data`
| Pong(Vec<u8>) | ||
| Pong(Vec<u8>), | ||
| /// Close reason. | ||
| Closed(CloseReason), |
There was a problem hiding this comment.
To me, it is still a bit confusing why this is treated as an Event and thus returned via Incoming instead of being treated as an error and thus be returned via Error::Closed.
I am not familiar enough with soketto, thus I don't think this should block the effort.
Background for other readers: paritytech/soketto#29 (review).
There was a problem hiding this comment.
It's not clear cut what the semantics should be. I went for the Error::Closed approach at first too, but @tomaka pointed out that the closing of the connection does carry information (a code in the simple case, but the specs allow for essentially any amount of "extra data").
The use case I have in mind for this is "collaborative backoff" where friendly peers can use the close reason to ask for respite, e.g. substrate telemetry asking nodes to send less data or go away for a while. In that context I think making the close reason an Error might be unfortunate.
There was a problem hiding this comment.
The general consensus is that whenever a socket/connection produces an error, it means that the connection is no longer in a usable state. This isn't the case after receiving a WebSocket Closed frame. Receiving a closed frame is a normal situation, and the connection is still in a normal state and can be used to send more data or a Closed frame in return.
One drawback is: if Closed was instead an error, we would have a compile-time guarantee to not receive any other message after this closing. This guarantee can no longer be enforced by the compiler, but I think that this is fine.
EDIT: actually no, because the stream could continue to produce non-errors after having produced an error. We're not losing any compile-time guarantee.
| Binary(Vec<u8>), | ||
| /// UTF-8 encoded application data. | ||
| Text(Vec<u8>), | ||
| pub enum Incoming { |
Co-authored-by: Max Inden <mail@max-inden.de>
|
Friendly ping @dvdplm. |
mxinden
changed the title
…aration-btween-data-and-notdata
…n-data-and-notdata' of github.com:libp2p/rust-libp2p into dp-close-reason-as-incoming-with-saner-separation-btween-data-and-notdata
…aration-btween-data-and-notdata
|
@mxinden Any chance of this getting in for v0.40? |
dvdplm
changed the title
Not into That said, I am happy to release @dvdplm would you mind including a changelog section for |
Happy to do that. In this PR or separately? |
|
This PR please. |
|
Merged via #2319. |
thomaseizinger
deleted the
dp-close-reason-as-incoming-with-saner-separation-btween-data-and-notdata
branch
Pass websocket CLOSE reason to the app as an
Incoming::Closed(soketto::CloseReason).This PR is a companion to paritytech/soketto#31 and lets applications know what the remote end claimed to be the reason for closing the connection (if any).
IncomingDatais now renamed toIncomingand the actual data (text or binary) is moved into its ownDataenum, which in turn allowsinto_bytes()and theAsRefimpl to apply more sanely to something that is actually data.