fix(multistream-select): don't wait for negotiation in poll_close #4019
Conversation
With `Version::V1Lazy` and negotiation of a singlel protocol only, a stream initiator optimistically
sends application data right after proposing its protocol. More specifically an application can
write data via `AsyncWrite::poll_write` even though the remote has not yet confirmed the stream protocol.
This saves one round-trip.
``` mermaid
sequenceDiagram
A->>B: "/multistream/1.0.0"
A->>B: "/perf/1.0.0"
A->>B: <some-perf-protocol-data>
B->>A: "/multistream/1.0.0"
B->>A: "/perf/1.0.0"
B->>A: <some-perf-protocol-data>
```
When considering stream closing, i.e. `AsyncWrite::poll_close`, and using stream closing as an
operation in ones protocol, e.g. using stream closing to signal the end of a request, this becomes tricky.
The behavior without this commit was as following:
``` mermaid
sequenceDiagram
A->>B: "/multistream/1.0.0"
A->>B: "/perf/1.0.0"
A->>B: <some-perf-protocol-data>
Note right of A: Call `AsyncWrite::poll_close` which first waits for the<br/>optimistic multistream-select negotiation to finish, before closing the stream,<br/> i.e. setting the FIN bit.
B->>A: "/multistream/1.0.0"
B->>A: "/perf/1.0.0"
Note right of B: Waiting for A to close the stream (i.e. set the `FIN` bit)<br/>before sending the response.
A->>B: FIN
B->>A: <some-perf-protocol-data>
```
The above takes 2 round trips:
1. Send the optimistic multistream-select protocol proposals as well as the initiator protocol
payload and waits for the confirmation of the protocols.
2. Close the stream, i.e. sends the `FIN` bit and waits for the responder protocol payload.
This commit proposes that the stream initiator should not wait for the multistream-select protocol
confirmation when closing the stream, but close the stream within the first round-trip.
``` mermaid
sequenceDiagram
A->>B: "/multistream/1.0.0"
A->>B: "/perf/1.0.0"
A->>B: <some-perf-protocol-data>
A->>B: FIN
B->>A: "/multistream/1.0.0"
B->>A: "/perf/1.0.0"
B->>A: <some-perf-protocol-data>
```
This takes 1 round-trip.
The downside of this commit is, that the stream initiator will no longer notice a negotiation error
when closing the stream. They will only notice it when reading from the stream. E.g. say that B does
not support "/perf/1.0.0", A will only notice on `AsyncRead::poll_read`, not on
`AsyncWrite::poll_close`. This is problematic for protocols where A only sends data, but never
receives data, i.e. never calls `AsyncRead::poll_read`. Though one can argue that such protocol is
flawed in the first place. With a response-less protocol, as even if negotiation succceeds, A
doesn't know whether B received the protocol payload.
|
I think this is a very interesting idea. I wonder if we should/could a log statement somehow that warns the user about this behaviour. |
Good idea. See 22668da. |
|
Confirmed with libp2p/test-plans#184 that this saves 1 round trip when e.g. using the perf protocol. Thus moving out of draft. Ready for review. |
|
Thank you for pushing for the test @thomaseizinger. Added. Succeeds with and fails without the patch. |
|
If you didn't want to go through the TCP listener dance, you could use |
#4032 updates all |
…1-lazy-poll-close-no-poll
|
@thomaseizinger latest commit updates this pull request to use |
|
This pull request has merge conflicts. Could you please resolve them @mxinden? 🙏 |
This includes libp2p/rust-libp2p#4019.
Description
With
Version::V1Lazyand negotiation of a single protocol, a stream initiator optimisticallysends application data right after proposing its protocol. More specifically an application can
write data via
AsyncWrite::poll_writeeven though the remote has not yet confirmed the stream protocol.This saves one round-trip.
sequenceDiagram A->>B: "/multistream/1.0.0" A->>B: "/perf/1.0.0" A->>B: <some-perf-protocol-data> B->>A: "/multistream/1.0.0" B->>A: "/perf/1.0.0" B->>A: <some-perf-protocol-data>When considering stream closing, i.e.
AsyncWrite::poll_close, and using stream closing as anoperation in ones protocol, e.g. using stream closing to signal the end of a request, this becomes tricky.
The behavior without this commit was as following:
sequenceDiagram A->>B: "/multistream/1.0.0" A->>B: "/perf/1.0.0" A->>B: <some-perf-protocol-data> Note left of A: Call `AsyncWrite::poll_close` which first waits for the<br/>optimistic multistream-select negotiation to finish, before closing the stream,<br/> i.e. setting the FIN bit. B->>A: "/multistream/1.0.0" B->>A: "/perf/1.0.0" Note right of B: Waiting for A to close the stream (i.e. set the `FIN` bit)<br/>before sending the response. A->>B: FIN B->>A: <some-perf-protocol-data>The above takes 2 round trips:
payload and waits for the confirmation of the protocols.
FINbit and waits for the responder protocol payload.This commit proposes that the stream initiator should not wait for the multistream-select protocol
confirmation when closing the stream, but close the stream within the first round-trip.
sequenceDiagram A->>B: "/multistream/1.0.0" A->>B: "/perf/1.0.0" A->>B: <some-perf-protocol-data> A->>B: FIN B->>A: "/multistream/1.0.0" B->>A: "/perf/1.0.0" B->>A: <some-perf-protocol-data>This takes 1 round-trip.
The downside of this commit is, that the stream initiator will no longer notice a negotiation error
when closing the stream. They will only notice it when reading from the stream. E.g. say that B does
not support "/perf/1.0.0", A will only notice on
AsyncRead::poll_read, not onAsyncWrite::poll_close. This is problematic for protocols where A only sends data, but neverreceives data, i.e. never calls
AsyncRead::poll_read. Though one can argue that such protocol isflawed in the first place. With a response-less protocol, as even if negotiation succceeds, A
doesn't know whether B received the protocol payload.
Notes & open questions
Confirmed against libp2p-perf.max-inden.de/ and with libp2p/test-plans#184.