protocols/identify: Move I/O to ConnectionHandler

[mxinden]

Description

I/O should not happen in the NetworkBehaviour implementation. It should happen in the ConnectionHandler implementation.

Motivation

All NetworkBehaviour implementations are driven by a single future task. Opposite to that we spawn a new future task for every connection. Doing as much as possible, especially I/O, on the connection tasks and not on the main task allows logic to potentially run in parallel.

Current Implementation

Today libp2p-identify drives the future returning a response to a remote in the NetworkBehaviour::poll implementation.

rust-libp2p/protocols/identify/src/identify.rs

Lines 53 to 54 in 66c2755

	/// Pending replies to send.
	pending_replies: VecDeque<Reply>,

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

No

[thomaseizinger]

The problem with today's implementation of identify is that we only know the observed address in the NetworkBehaviour. To fix this, we will have to implement IntoConnectionHandler for the identify protocol and grab the observed address out of the ConnectedPoint. Going further, this information can be passed into the InboundUpgrade implementation via the OpenInboundInfo type in ConnectedHandler.

[jxs]

@thomaseizinger could you shed some light on how you envisioned passing the observed address from Behaviour to Handler ?

I can't really see where to do that as Behaviour::new_handler() doesn't have access to the MultiAddress.
Meanwhile I started implementing this using NetworkBehaviourAction::NotifyHandler to pass the MultiAddress and the Substream to the Handler, but it doesn't seem semantically correct passing the Substream from the Handler to the Behaviour to then get it back from the Behaviour again, I feel this should be encapsulated in the Handler.

Thanks :)

[thomaseizinger]

You need to implement IntoConnectionHandler instead of ConnectionHandler. That will give you access to the ConnectedPoint.

[jxs]

right, thanks Thomas. What about listen_addresses and supported_protocols needed to create the Identify Info?
We only get them from PollParameters on NetworkBehaviour::poll not ConnectionHandler:poll and we need .
This maybe also affects the development #3153.

[thomaseizinger]

right, thanks Thomas. What about listen_addresses and supported_protocols needed to create the Identify Info?
We only get them from PollParameters on NetworkBehaviour::poll not ConnectionHandler:poll and we need .
This maybe also affects the development #3153.

I think you will need to store those in the NetworkBehaviour and pass them on in new_handler.

A couple of refactorings coming in will affect this but overall, this should still work I believe!

[mxinden]

right, thanks Thomas. What about listen_addresses and supported_protocols needed to create the Identify Info?
We only get them from PollParameters on NetworkBehaviour::poll not ConnectionHandler:poll and we need .
This maybe also affects the development #3153.

I think you will need to store those in the NetworkBehaviour and pass them on in new_handler.

Discussed out-of-band with @jxs today. Passing them in new_handler is not possible as the set of supported protocols might change during the lifetime of a ConnectionHandler. I know it doesn't today, but it will in the future, see e.g. #2521.

Instead I suggest passing an event up from the ConnectionHandler when identify information like supported protocols is needed. The NetworkBehaviour would respond to the event, retrieving the list of supported protocols from the PollParameters in NetworkBehaviour::poll.

[thomaseizinger]

right, thanks Thomas. What about listen_addresses and supported_protocols needed to create the Identify Info?
We only get them from PollParameters on NetworkBehaviour::poll not ConnectionHandler:poll and we need .
This maybe also affects the development #3153.

I think you will need to store those in the NetworkBehaviour and pass them on in new_handler.

Discussed out-of-band with @jxs today. Passing them in new_handler is not possible as the set of supported protocols might change during the lifetime of a ConnectionHandler. I know it doesn't today, but it will in the future, see e.g. #2521.

Instead I suggest passing an event up from the ConnectionHandler when identify information like supported protocols is needed. The NetworkBehaviour would respond to the event, retrieving the list of supported protocols from the PollParameters in NetworkBehaviour::poll.

Even once we have a mechanism where they change, I'd expect the behaviour to be notified. It feels more appropriate to pass it down when it changes instead of requesting it every time. Esp. because that allows the handler to answer incoming streams without interacting with the behaviour which simplifies things a lot.

[mxinden]

I'd expect the behaviour to be notified.

👍 like we do for new listen addresses via FromSwarm::NewListenAddr I think we should have an event informing a NetworkBehaviour of a new listen protocol. The NetworkBehaviour can then pass that event down to the ConnectionHandler.

Given that we don't have these mechanisms yet, I suggest proceeding with the above, that is have the ConnectionHandler request the information from the NetworkBehaviour. We do the same in libp2p-relay where one needs to send ones external addresses to a remote.

[thomaseizinger]

It makes the implementation of the handler more complicated because you now have to suspend the substream and can't use async-await to implement the protocol (unless you use a channel).

At the moment, we know that the protocols don't change so we just only send the update once. Later, once they can change, we can send the update every time it is modified.

Unless we disagree on the bigger idea that we don't want to push state into the handler, this should be preferred where possible because it is IMO a simpler design.