swarm: Deprecate ConnectionHandler::{In,Out}boundOpenInfo

[thomaseizinger]

Currently, a user can specify an arbitrary payload type that can be passed a long with the opening of an inbound or an outbound stream. In general, being able to pass along user data with an asynchronous operation can be helpful because one doesn't need to track this information locally. Furthermore, for some asynchronous operations (like connection dialing in the case of relay's and the dcutr protocol) it is very important that one can 1-to-1 associate the initiated action with the result of the asynchronous operation.

Streams however are completely interchangeable in libp2p once the protocol to be spoken on top of it has been negotiated. Thus, we should be able to remove ConnectionHandler::InboundOpenInfo and ConnectionHandler::OutboundOpenInfo completely in favor of ConnectionHandlers tracking the user data in a FIFO queue.

In fact, that is what we are doing within swarm::Connection anyway. Upon opening a new stream, we just associate it with the next upgrade from the list:

rust-libp2p/swarm/src/connection.rs

Lines 289 to 303 in ee77513

	match muxing.poll_outbound_unpin(cx)? {
	Poll::Pending => {}
	Poll::Ready(substream) => {
	let (user_data, timeout, upgrade) = requested_substream.extract();
	negotiating_out.push(SubstreamUpgrade::new_outbound(
	substream,
	user_data,
	timeout,
	upgrade,
	*substream_upgrade_protocol_override,
	));
	continue; // Go back to the top, handler can potentially make progress again.
	}

Tasks

Beta Give feedback

1. refactor(kad): don't use OutboundOpenInfo #3760
   send-it +1
2. refactor(dcutr): don't use OutboundOpenInfo #3762
   send-it trivial +2
3. refactor(perf): don't use OutboundOpenInfo #3763
   send-it trivial +2
4. relay: refactor stream handling away from {In,Out}boundUpgrade #4075
   difficulty:moderate help wanted +2
   
5. refactor(request-response): don't use upgrade infrastructure #3914
   send-it +1
6. swarm: Design utility type for tracking pending stream openings #4510
   decision-pending difficulty:hard +2
   
7. refactor(kad): use oneshots and async-await for outbound streams #4901
   send-it +1
8. refactor(relay): use oneshot's to track requested streams #4900
   send-it +1
9. Write tutorial on how to write your own ConnectionHandler, documenting use of futures-bounded and oneshots

[thomaseizinger]

With the 👍 from @mxinden and @elenaf9, I am removing the decision-pending label and putting this up for grabs.

[thomaseizinger]

Linking the original discussion here: #1709

The motivation for adding this was to properly deal with upgrade errors. Hence, I'd suggest that we proceed here once #2863 is implemented. Depending on the outcome of the discussion in #3353 (comment), this requirement will go away completely and we should be able to remove these associated types without harming existing usecases.

#1710 also mentions that there is no guarantee that a call to listen_protocol is followed by what is now ConnectionEvent::FullyNegotiatedInbound or ConnectionEvent::ListenUpgradeError. The only scenario I can think of where this is the case is when the connection gets shut down in the meantime. In every other case, we pair these calls up.

Long-term, I'd like us to only expose a list of protocols from a ConnectionHandler. That will result in a much simpler interface, especially once #2831 is shipped as well.

I wonder what a good migration plan is from one to the other interface. It is quite a big change so I'd want to offer a smooth migration path to our users.

[thomaseizinger]

One thing that just came to my mind that is easier with the current design:

Being able to pass state to as part of creating a new stream allows ConnectionHandlers to simplify their state because you don't have to keep track of "how many streams have I requested and how many do I need?"

With the current design, you just drain a list of commands and open a new stream per command, passing along the data they need.

[mxinden]

because you don't have to keep track of "how many streams have I requested and how many do I need?"

Long term ConnectionHandlers need to be able to receive backpressure on stream creation. I.e. they should limit the amount of in-flight stream requests. Thus I would argue that they need this state anyways.

[thomaseizinger]

#3763 is a pretty classic refactoring that users will have to do. The new version is more verbose but I think that for new-comers, it is easier to understand because there is a smaller API surface to learn.

[drHuangMHT]

A brief search around the codebase showed that there is no use case for the open info. Should this be added to 0.54 milestone?

[thomaseizinger]

A brief search around the codebase showed that there is no use case for the open info. Should this be added to 0.54 milestone?

Yes we could. We would have to issue a deprecation first to make upstream users aware that these associated types are going away.