swarm/src/connection: Prioritize handler over negotiating streams #2638
Conversation
The `HandlerWrapper` polls three components: 1. `ConnectionHandler` 2. Outbound negotiating streams 3. Inbound negotiating streams The `ConnectionHandler` itself might itself poll already negotiated streams. By polling the three components above in the listed order one: - Prioritizes local work and work coming from negotiated streams over negotiating streams. - Prioritizes outbound negotiating streams over inbound negotiating streams, i.e. outbound requests over inbound requests.
| } | ||
|
|
||
| // After the `inject_*` calls, the [`ConnectionHandler`] might be able to make progress. | ||
| continue; |
There was a problem hiding this comment.
I think we could also recurse here and thus avoid the loop? Not sure if that is necessarily easier to understand.
There was a problem hiding this comment.
I don't follow. What do you mean with "recurse"? As in a recursive call to the same method?
There was a problem hiding this comment.
Yeah. You should be able to just call self.poll here I believe?
There was a problem hiding this comment.
I have not seen recursion used on any serious Rust project. Not saying that it does not exist. Thus far I have avoided recursion in Rust due to:
- No Tail Call Ellimination
- Call stack limits
- Sensing that this is not idiomatic, thus not intuitive
Am I missing something? Do the above arguments not apply here?
There was a problem hiding this comment.
I think in this particular case, the recursion is very likely to stop after one call, is it not?
But I understand that this might be too much of a detail to notice and thus make the code harder to understand. Also, it is not guaranteed and thus risky to use with the usual problems of recursion.
Happy for the loop to stay, was just an idea :)
| match res { | ||
| ConnectionHandlerEvent::Custom(event) => { | ||
| return Poll::Ready(Ok(Event::Custom(event))); | ||
| } | ||
| ConnectionHandlerEvent::OutboundSubstreamRequest { protocol } => { | ||
| let id = self.unique_dial_upgrade_id; | ||
| let timeout = *protocol.timeout(); | ||
| self.unique_dial_upgrade_id += 1; | ||
| let (upgrade, info) = protocol.into_upgrade(); | ||
| self.queued_dial_upgrades.push((id, SendWrapper(upgrade))); | ||
| return Poll::Ready(Ok(Event::OutboundSubstreamRequest(( | ||
| id, info, timeout, | ||
| )))); | ||
| } | ||
| ConnectionHandlerEvent::Close(err) => return Poll::Ready(Err(err.into())), |
There was a problem hiding this comment.
It is not immediately clear that all these branches will return because of the noise of constructing the return value. We could extract this into a separate function (self.poll_handler(cx)) and only have 1 return statement to make this more obvious. What do you think?
There was a problem hiding this comment.
What do you think of 1de8039 @thomaseizinger?
There was a problem hiding this comment.
Yeah that is better :)
There was a problem hiding this comment.
LGTM.
One question (not related to this PR but something I was wondering about while reviewing):
When asking the handler whether to keep the connection alive or not, in case of KeepAlive::Until it checks if the "until" timestamp is in the future.
If it is, we set the Shutdown::Later accordingly.
But in case that it is in the past (checked_duration_since returns None), we do not do anything. Shouldn't in that case the shutdown be set to Shutdown::Asap?
rust-libp2p/swarm/src/connection/handler_wrapper.rs
Lines 385 to 389 in 8f0f9a8
Am I missing something?
Not sure what the reasoning for this was. I could imagine that a |
Description
The
HandlerWrapperpolls three components:ConnectionHandlerThe
ConnectionHandleritself might itself poll already negotiated streams.By polling the three components above in the listed order one:
negotiating streams.
streams, i.e. outbound requests over inbound requests.
Links to any relevant issues
Follows the general theme of #2627 and #2626.
Change checklist