swarm/handler: Split `inject_listen_upgrade_error` into two

[mxinden]

Description

Split the ConectionHandler::inject_listen_upgrade_error into two methods:

1. inject_listen_negotiation_error for errors during the multistream select protocol execution. Not specifically related to the protocol of this ConnectionHandler.
2. inject_listen_upgrade_error for errors during the actualy InboundUpgrade. Specific to the protocol of this ConnectionHandler.

Motivation

Using the same method for both use-cases is error prone. E.g. see libp2p-relay where we print an error for a potentially unrelated issue:

https://github.com/libp2p/rust-libp2p/blob/66c275520d8cf56152cce9a1c24bb4e2c42e0833/protocols/relay/src/v2/client/handler.rs#L379-L383

Current Implementation

Today ConnectionHandler::inject_listen_upgrade_error is called for both multistream select negotiation errors and protocol specific upgrade errors.

Are you planning to do it yourself in a pull request?

No

//CC @dignifiedquire as we ran into this during our recent debugging session.

Still remember discussing this a long time ago with @romanb.

[thomaseizinger]

I am in favor of this. The error nesting is quite annoying to deal with!

[maschad]

I can take a look into it this week.

[mxinden]

1. inject_listen_negotiation_error for errors during the multistream select protocol execution. Not specifically related to the protocol of this ConnectionHandler.

In case neither us nor any user ever makes use of this method, we should consider not introducing it in the first place.

[mxinden]

Thanks for the update @jxs I am bit preoccupied with some issues on js-libp2p so I haven't had a chance to work on this. Perhaps it may be best to assign this to someone else?

https://github.com/libp2p/rust-libp2p/pull/3098#issuecomment-1348526516

@jxs would you be interested in tackling this one?

[jxs]

no worries Max, I'll take it!

[maschad]

no worries Max, I'll take it!

Thanks @jxs !

[jxs]

update: waiting on https://github.com/libp2p/rust-libp2p/pull/3264 to be merged to start tackling this.

[jxs]

@mxinden there's an issue with the separation of errors, DialUpgradeError error can also be a Timeout see https://github.com/libp2p/rust-libp2p/blob/9c96bbb54b23d29d103f4f57e3016a5420f1df95/swarm/src/connection.rs#L183-L186

to address this, we can have ConnectionEvent::DialNegotiationError along with ConnectionEvent::DialUpgradeError following the same logic suggested with ListenNegotiationError and ListenUpgradeError. But both DialNegotiationError and ListenNegotiationError will have a nested ConnectionHandlerNegotiationError :

#[derive(Copy, Clone, Debug)]
pub enum ConnectionHandlerNegotiationError {
    /// The opening attempt timed out before the negotiation was fully completed.
    Timeout,
    /// There was an error in the timer used.
    Timer,
}

Given that this issue was created when the api was still with the many inject_* methods instead of the on_connection_event with the ConnectionEvent enum, the difference will be that instead of having ListenUpgradeError with a nested ConnectionHandlerUpgradeError itself also with a nested UpgradeError, we will be having DialNegotiationError and ListenNegotiationError with ConnectionHandlerNegotiationError instead.

Basically switching a third level of enum variant, ConnectionEvent::ListenUpgradeError::UpgradeError::Err
for two second level enum variants,
ConnectionEvent::DialNegotiationError::ConnectionHandlerNegotiationError and
ConnectionEvent::ListenNegotiationError::ConnectionHandlerNegotiationError.

Meanwhile the Timer variant doesn't seem to be used, if you confirm that we can just have ListenTimeoutError and DialTimeoutError with DialUpgradeError and ListenUpgradeError

CC @thomaseizinger

[mxinden]

Basically switching a third level of enum variant, ConnectionEvent::ListenUpgradeError::UpgradeError::Err for two second level enum variants, ConnectionEvent::DialNegotiationError::ConnectionHandlerNegotiationError and ConnectionEvent::ListenNegotiationError::ConnectionHandlerNegotiationError.

Just to make sure we are on the same page, the ConnectionHandler does not need to be aware of ListenNegotiationError. In other words, given that negotiation of the protocol itself failed on the stream, there is no value in notifying the ConnectionHandler about it, as the ConnectionHandler doesn't know whether the stream was for them, or for some other ConnectionHandler.

Meanwhile the Timer variant doesn't seem to be used, if you confirm that we can just have ListenTimeoutError and DialTimeoutError with DialUpgradeError and ListenUpgradeError

Sounds good to me. Might be a relic from when we used a different timer library.

[jxs]

Just to make sure we are on the same page, the ConnectionHandler does not need to be aware of ListenNegotiationError. In other words, given that negotiation of the protocol itself failed on the stream, there is no value in notifying the ConnectionHandler about it, as the ConnectionHandler doesn't know whether the stream was for them, or for some other ConnectionHandler.

Ok, thanks for the clarification Max. But then who should deal with the Timeouts when negotiating streams, or rather, where were you thinking having inject_listen_negotiation_error ?

[mxinden]

But then who should deal with the Timeouts when negotiating streams, or rather, where were you thinking having inject_listen_negotiation_error ?

On the outbound side we know which ConnectionHandler initiated the outbound stream request. Thus we should notify the ConnectionHandler of the negotiation failure.

On the inbound side, we don't know which ConnectionHandler should handle the error, thus I suggest simply logging the error on debug within libp2p-swarm connection.rs. Alternative is to bubble up an event through Swarm, though unless someone asks for it, I would suggest logging only.

Does the reasoning above make sense?

[jxs]

yes, thanks Max! Submitted #3307

[thomaseizinger]

This is stale and has been resolved by trimming down ListenUpgradeError to just the error emitted by InboundUpgrade.