libp2p
swarm: Deprecate `ConnectionHandler::{In,Out}boundOpenInfo`
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: https://github.com/libp2p/rust-libp2p/blob/ee775137f30c19dfe43598400b1dda9fcdd46f51/swarm/src/connection.rs#L289-L303
- [ ] https://github.com/libp2p/rust-libp2p/pull/3760
- [ ] https://github.com/libp2p/rust-libp2p/pull/3762
- [ ] https://github.com/libp2p/rust-libp2p/pull/3763
- [ ] https://github.com/libp2p/rust-libp2p/issues/4075
- [ ] https://github.com/libp2p/rust-libp2p/pull/3914
- [ ] https://github.com/libp2p/rust-libp2p/issues/4510
- [ ] https://github.com/libp2p/rust-libp2p/pull/4901
- [ ] #4900
- [ ] Write tutorial on how to write your own `ConnectionHandler`, documenting use of `futures-bounded` and oneshots
With the :+1: from @mxinden and @elenaf9, I am removing the decision-pending label and putting this up for grabs.
Linking the original discussion here: https://github.com/libp2p/rust-libp2p/issues/1709
The motivation for adding this was to properly deal with upgrade errors. Hence, I'd suggest that we proceed here once https://github.com/libp2p/rust-libp2p/issues/2863 is implemented. Depending on the outcome of the discussion in https://github.com/libp2p/rust-libp2p/discussions/3353#discussioncomment-4733947, this requirement will go away completely and we should be able to remove these associated types without harming existing usecases.
https://github.com/libp2p/rust-libp2p/pull/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 https://github.com/libp2p/rust-libp2p/issues/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.
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.
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.
https://github.com/libp2p/rust-libp2p/pull/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.
The remaining items for this one are:
- ~
libp2p-rendezvous: Ideally fixed by doing #3878.~ libp2p-relay: Currently usesOutboundOpenInfoto track data alongside the actual upgrade. To remove this, we need to also stop using theInbound/OutboundUpgrademechanism, otherwise we cannot correctly associate the data currently tracked in the "info" with the upgrade, see https://github.com/libp2p/rust-libp2p/issues/4075.libp2p-request-response: Uses these both associated types to track theRequestId. We likely need to move away fromInbound/OutboundUpgradethere at the same time too. I've attempted to do this here: https://github.com/libp2p/rust-libp2p/pull/3914
In https://github.com/libp2p/rust-libp2p/pull/4275, there is a discussion about whether we should actually remove these associated types.
A middle-ground could be to only keep OutboundOpenInfo perhaps? Opening a stream is always asynchronous, meaning it could be useful for a handler to delegate the tracking of this state to the connection.
On the other hand, this will cause a disparity between inbound and outbound streams.
I am a bit torn. On the one hand, I think a smaller interface is much easier to get started with. On the other hand, I see the value of delegating some functionality to Connection.
Overall, I am still leaning towards a reduced interface being better. Every time you hand data off to a framework, you kind of need to understand how it works to build your app: "If I put data in here, when do I get it back?" I think it is a net-positive if the user doesn't have to think about this, even if it means they need to write a bit more code.
Perhaps we can create some utility state containers that they can compose into their ConnectionHandler to make this easier!
cc @dgarus
Hi!
Every time you hand data off to a framework, you kind of need to understand how it works to build your app: "If I put data in here, when do I get it back?"
IMHO, removing the associated types doesn't solve this problem. The user has to decide how to store a state to process stream, how and where gets it. So he should know and do much more rather than with the associated types.
For every case when we need to store a state, we have to add the same code, that is unnecessary code duplication.
Extra code complicates the struct to read and understand and leads to errors.
So he should know and do much more rather than with the associated types.
This is what I am not so sure about. Storing / mutating state is "just programming". The user is free to do that in which ever way they want, without constraints from libp2p. I think that rust-libp2p would be easier to understand if it were smaller (from an API perspective).
We are already locked in on the model that ConnectionHandler is a state machine. Assuming that users understand that, tracking some more state around pending outbound streams shouldn't be too much to ask?
For every case when we need to store a state, we have to add the same code, that is unnecessary code duplication.
We might be able to mitigate that by adding helper components that users can compose into their ConnectionHandler.
We might be able to mitigate that by adding helper components that users can compose into their ConnectionHandler.
Yes, this is a necessary thing.
We might be able to mitigate that by adding helper components that users can compose into their ConnectionHandler.
Yes, this is a necessary thing.
I have some ideas here. The gist is that we need a struct that we can delegate the ConnectionEvents too and it can do the correct accounting around the associated data, similar to how we have structs like ListenAddresses.
We might need to make some layout changes to ConnectionEvent to make it really smooth.
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?
A brief search around the codebase showed that there is no use case for the open info. Should this be added to
0.54milestone?
Yes we could. We would have to issue a deprecation first to make upstream users aware that these associated types are going away.