swarm: Deprecate `ConnectionHandler::{In,Out}boundOpenInfo` #3268

thomaseizinger · 2022-12-20T03:37:33Z

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

Give feedback

refactor(kad): don't use OutboundOpenInfo #3760

send-it
refactor(dcutr): don't use OutboundOpenInfo #3762

send-it trivial
refactor(perf): don't use OutboundOpenInfo #3763

send-it trivial
relay: refactor stream handling away from {In,Out}boundUpgrade #4075

difficulty:moderate help wanted
refactor(request-response): don't use upgrade infrastructure #3914

send-it
swarm: Design utility type for tracking pending stream openings #4510

decision-pending difficulty:hard
refactor(kad): use oneshots and async-await for outbound streams #4901

send-it
refactor(relay): use oneshot's to track requested streams #4900

send-it
Write tutorial on how to write your own ConnectionHandler, documenting use of futures-bounded and oneshots
Options

The text was updated successfully, but these errors were encountered:

thomaseizinger · 2023-01-18T11:29:33Z

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

thomaseizinger · 2023-01-20T05:47:42Z

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 · 2023-03-12T21:20:32Z

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 · 2023-03-13T14:09:00Z

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 · 2023-04-11T06:45:39Z

#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.

In a previous refactoring, we actually removed the usage of this openinfo so this is just removing dead code. Related: #3268. Pull-Request: #3762.

Instead of passing the command along, we store it in a buffer and retrieve it once the stream is upgraded. Related: #3268. Pull-Request: #3763.

As part of pushing #3268 forward, remove the use of `OutboundOpenInfo` from `libp2p-kad`. Related #3268. Pull-Request: #3760.

This patch refactors `libp2p-request-response` to not use the "upgrade infrastructure" provided by `libp2p-swarm`. Instead, we directly convert the negotiated streams into futures that read and write the messages. Related: #3268. Related: #2863. Pull-Request: #3914. Co-authored-by: Yiannis Marangos <yiannis@eiger.co>

This refactoring addresses several aspects of the current handler implementation: - Remove the manual state machine for outbound streams in favor of using `async-await`. - Use `oneshot`s to track the number of requested outbound streams - Use `futures_bounded::FuturesMap` to track the execution of a stream, thus applying a timeout to the entire request. Resolves: #3130. Related: #3268. Related: #4510. Pull-Request: #4901.

This patch refactors `libp2p-request-response` to not use the "upgrade infrastructure" provided by `libp2p-swarm`. Instead, we directly convert the negotiated streams into futures that read and write the messages. Related: libp2p#3268. Related: libp2p#2863. Pull-Request: libp2p#3914. Co-authored-by: Yiannis Marangos <yiannis@eiger.co>

drHuangMHT · 2024-03-18T02:59:48Z

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 · 2024-03-18T04:52:49Z

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.

thomaseizinger added difficulty:moderate decision-pending Marks issues where a decision is pending before we can move forward. labels Dec 20, 2022

thomaseizinger mentioned this issue Jan 1, 2023

docs(coding-guidelines): Add request response correlation #3290

Merged

4 tasks

thomaseizinger mentioned this issue Jan 9, 2023

refactor(swarm)!: refactor ListenUpgradeError and DialUpgradeError #3307

Closed

4 tasks

thomaseizinger added help wanted priority:nicetohave and removed decision-pending Marks issues where a decision is pending before we can move forward. labels Jan 18, 2023

thomaseizinger self-assigned this Jan 20, 2023

thomaseizinger removed their assignment Mar 20, 2023

thomaseizinger mentioned this issue Mar 29, 2023

fix(gossipsub): gracefully disable handler on stream errors #3625

Merged

4 tasks

This was referenced Apr 10, 2023

refactor(kad): don't use OutboundOpenInfo #3760

Merged

refactor(dcutr): don't use OutboundOpenInfo #3762

Merged

refactor(perf): don't use OutboundOpenInfo #3763

Merged

mergify bot pushed a commit that referenced this issue Apr 28, 2023

refactor(dcutr): don't use OutboundOpenInfo

66466c4

In a previous refactoring, we actually removed the usage of this openinfo so this is just removing dead code. Related: #3268. Pull-Request: #3762.

mergify bot pushed a commit that referenced this issue Apr 28, 2023

refactor(perf): don't use OutboundOpenInfo

8445079

Instead of passing the command along, we store it in a buffer and retrieve it once the stream is upgraded. Related: #3268. Pull-Request: #3763.

mergify bot pushed a commit that referenced this issue Apr 28, 2023

refactor(kad): don't use OutboundOpenInfo

99ad3b6

As part of pushing #3268 forward, remove the use of `OutboundOpenInfo` from `libp2p-kad`. Related #3268. Pull-Request: #3760.

This comment was marked as outdated.

Sign in to view

thomaseizinger mentioned this issue May 11, 2023

refactor(request-response): don't use upgrade infrastructure #3914

Merged

4 tasks

This was referenced Jun 13, 2023

swarm/src/protocols_handler: Make ProtocolsHandlerEvent depend on `ProtocolsHandler only #2389

Closed

refactor(rendezvous): rewrite using libp2p-request-response #4051

Merged

thomaseizinger mentioned this issue Aug 23, 2023

refactor(relay): move stream-handling away from {In,Out}boundUpgrade #4275

Merged

4 tasks

This comment was marked as outdated.

Sign in to view

thomaseizinger added this to the Simplify ConnectionHandler trait milestone Sep 18, 2023

This was referenced Sep 18, 2023

swarm: Design utility type for tracking pending stream openings #4510

Closed

Simplify ConnectionHandler trait by removing as many associated types as possible #2863

Open

thomaseizinger removed this from the Simplify ConnectionHandler trait milestone Sep 19, 2023

thomaseizinger mentioned this issue Nov 14, 2023

feat: introduce InflightProtocolDataQueue #4834

Closed

4 tasks

thomaseizinger mentioned this issue Nov 21, 2023

refactor(kad): use oneshots and async-await for outbound streams #4901

Merged

4 tasks

thomaseizinger mentioned this issue Dec 9, 2023

Excessive calls to poll and cloning of Wakers. #4990

Open

drHuangMHT linked a pull request Mar 18, 2024 that will close this issue

refactor: mark {In, Out}boundOpenInfo as deprecated #5242

Draft

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm: Deprecate `ConnectionHandler::{In,Out}boundOpenInfo` #3268

swarm: Deprecate `ConnectionHandler::{In,Out}boundOpenInfo` #3268

thomaseizinger commented Dec 20, 2022 •

edited

Tasks

thomaseizinger commented Jan 18, 2023

thomaseizinger commented Jan 20, 2023

thomaseizinger commented Mar 12, 2023

mxinden commented Mar 13, 2023

thomaseizinger commented Apr 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

drHuangMHT commented Mar 18, 2024

thomaseizinger commented Mar 18, 2024

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

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

Comments

thomaseizinger commented Dec 20, 2022 • edited

Tasks

thomaseizinger commented Jan 18, 2023

thomaseizinger commented Jan 20, 2023

thomaseizinger commented Mar 12, 2023

mxinden commented Mar 13, 2023

thomaseizinger commented Apr 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

drHuangMHT commented Mar 18, 2024

thomaseizinger commented Mar 18, 2024

swarm: Deprecate `ConnectionHandler::{In,Out}boundOpenInfo` #3268

swarm: Deprecate `ConnectionHandler::{In,Out}boundOpenInfo` #3268

thomaseizinger commented Dec 20, 2022 •

edited