src/connection: Process command or socket result immediately #138
Conversation
> The `frame` future might be _ready_ with an `Error` from the underlying socket (i.e. here `libp2p-websocket`). Though given that the result of the `control_command` `Future` is handled first, `on_control_command` is called despite `frame` having returned an `Error`. `on_control_command` itself may try to write to the underlying socket, which will panic given that the socket returned an error earlier via the `frame` `Future`. With this patch, once any of `next_stream_command`, `next_control_command` or `next_frame` `Future` is ready, the result is processed right away, without additionally polling the remaining pending `Future`s, thus surfacing errors as early as possible. See libp2p/rust-libp2p#2598 for details.
|
I am not happy with At this point I don't think we should refactor this Yamux implementation. Instead I suggest to focus on the two new transport implementations libp2p/rust-libp2p#2289 and libp2p/rust-libp2p#2622, as these leverage more powerful stream multiplexing protocols (i.e. QUIC's multiplexer and SCTP). |
| let combined_future = future::select( | ||
| future::select(next_stream_command, next_control_command), | ||
| next_frame, | ||
| ); | ||
| match combined_future.await { | ||
| Either::Left((Either::Left((cmd, _)), _)) => self.on_stream_command(cmd).await?, | ||
| Either::Left((Either::Right((cmd, _)), _)) => self.on_control_command(cmd).await?, | ||
| Either::Right((frame, _)) => { | ||
| if let Some(stream) = | ||
| self.on_frame(frame.transpose().map_err(Into::into)).await? | ||
| { | ||
| return Ok(Some(stream)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Have you considered using the futures::select! macro instead of nesting future::select calls?
There was a problem hiding this comment.
I have, though I favor futures::future::select over futures::select! due to the following reasons:
-
While more verbose, I find
futures::future::selecteasier to reason about thanfutures::select!. -
In general I prefer functions over macros.
-
I understand the inner workings of
futures::future::select. The implementation is very simple. -
I don't fully understand the guarantees of
futures::select!, e.g.:If multiple futures are ready, one will be pseudo-randomly selected at runtime.
https://docs.rs/futures/latest/futures/macro.select.html
What happens to the result of the non-selected ready futures?
|
Been tested without further panics libp2p/rust-libp2p#2598 (comment) |
With this patch, once any of
next_stream_command,next_control_commandornext_frameFutureis ready, the result is processed right away, withoutadditionally polling the remaining pending
Futures, thus surfacing errors asearly as possible.
See libp2p/rust-libp2p#2598 for details.
@elenaf9 @thomaseizinger could either of you do me a favor and take a look here? Happy to expand on the pull request description in case more context is needed.