{core/,swarm/}: Dial with handler and return handler on error and closed #2191

mxinden · 2021-08-12T15:41:17Z

Background

As promised in #2118 (reply in thread) I wanted to trial adding something similar to ProtocolsHandler::OutboundOpenInfo for connections instead of substreams to NetworkBehaviourAction::{DialPeer,DialAddress}.

#2118 (comment) details the benefits. Long story short, it allows one to attach state to a dial request, thus not having to keep track of it within a NetworkBehaviour implementation.

While trialing the above, I had the ideas for #2182 and #2183, both simplifying trait parameters, one in libp2p-swarm and one in libp2p-core. Passing an opaque OpenInfo type along with NetworkBehaviourAction::{DialPeer,DialAddress} would require yet another trait parameter. To circumvent this additional complexity, I came up with the below, in other words with the change proposed in this pull request.

Proposal

Require NetworkBehaviourAction::{DialPeer,DialAddress} to contain a ProtocolsHandler (see diff below). This allows a behaviour to attach custom state to its handler. The behaviour would no longer need to track this state separately during connection establishment, thus reducing state required in a behaviour. E.g. in the case of libp2p-kad the behaviour can include a GetRecord request in its handler, or e.g. in the case of libp2p-request-response the behaviour can include the first request in the handler.

Diff

+pub enum NetworkBehaviourAction<TInEvent, TOutEvent, THandler> {
     /// Instructs the `Swarm` to return an event when it is being polled.
     GenerateEvent(TOutEvent),
 
@@ -237,6 +238,7 @@ pub enum NetworkBehaviourAction<TInEvent, TOutEvent> {
     DialAddress {
         /// The address to dial.
         address: Multiaddr,
+        handler: THandler,
     },
 
     /// Instructs the swarm to dial a known `PeerId`.
@@ -254,6 +256,7 @@ pub enum NetworkBehaviourAction<TInEvent, TOutEvent> {
         peer_id: PeerId,
         /// The condition for initiating a new dialing attempt.
         condition: DialPeerCondition,
+        handler: THandler,
     },

Return ProtocolsHandler on connection error and close (see diff below). This allows a behaviour to extract its custom state previously included in the handler on connection failure and connection closing. E.g. in the case of libp2p-kad the behaviour could extract the attached GetRecord from the handler of the failed connection and then start another connection attempt with a new handler with the same GetRecord or bubble up an error to the user.

Diff

diff --git a/core/src/network/event.rs b/core/src/network/event.rs
index 7b415826..82f12b13 100644
--- a/core/src/network/event.rs
+++ b/core/src/network/event.rs
@@ -92,6 +92,7 @@ where
         send_back_addr: Multiaddr,
         /// The error that happened.
         error: PendingConnectionError<TTrans::Error>,
+        handler: THandler,
     },
 
     /// A new connection to a peer has been established.
@@ -124,12 +125,13 @@ where
         error: Option<ConnectionError<<THandler::Handler as ConnectionHandler>::Error>>,
         /// The remaining number of established connections to the same peer.
         num_established: u32,
+        handler: THandler::Handler,
     },
 
     /// A dialing attempt to an address of a peer failed.
     DialError {
         /// The number of remaining dialing attempts.
-        attempts_remaining: u32,
+        attempts_remaining: DialAttemptsRemaining<THandler>,
 
         /// Id of the peer we were trying to dial.
         peer_id: PeerId,
@@ -148,6 +150,8 @@ where
 
         /// The error that happened.
         error: PendingConnectionError<TTrans::Error>,
+
+        handler: THandler,
     },
 
     /// An established connection produced an event.
@@ -169,6 +173,21 @@ where
     },
 }
 
+pub enum DialAttemptsRemaining<THandler> {
+    // TODO: Make this a NonZeroU32.
+    Some(u32),
+    None(THandler),
+}

What do people think? Do you see additional use-cases this would support or do you see use-cases that would not be but should be supported? Especially interested n @thomaseizinger opinion given your initial proposal on #2118.

thomaseizinger

Very nice idea. I think it is a good direction.

swarm/src/lib.rs

thomaseizinger · 2021-08-16T12:18:48Z

swarm/src/protocols_handler/node_handler.rs

@@ -65,6 +65,11 @@ where
        self.substream_upgrade_protocol_override = version;
        self
    }
+
+    // TODO: Rethink this method.


I've always found the indirection between ConnectionHandler and ProtocolsHandler a bit annoying. I understand that it is necessary because libp2p-core can't know about the swarm abstraction. I do wonder though on whether or not libp2p-core couldn't provide the ProtocolsHandler abstraction right away instead of us having to effectively re-define almost the same interface.

That is an option worth exploring. Though I don't think we should do so in this pull request to separate concerns. Note that moving ProtocolsHandler into libp2p-core would require moving concepts like KeepAlive into libp2p-core as well.

This logic seems to be a leftover of libp2p#889 and unused today.

thomaseizinger · 2021-08-19T22:29:02Z

swarm/src/behaviour.rs

+pub enum NetworkBehaviourAction<
+    TOutEvent,
+    THandler: IntoProtocolsHandler,
+    TInEvent = THandlerInEvent<THandler>,
+> {


Similar to THandlerInEvent, I think we could define NetworkBehaviourAction in terms of the NetworkBehaviour trait. That would reduce the amount of type parameters down to one.

It would also make it easier to refer to because you can just say NetworkBehaviourAction<Self> within the NetworkBehaviour.

Thoughts?

Ah we would have to give up our map_XYZ functions then.

Good idea @thomaseizinger which I started to explore. See mxinden#5

Ah we would have to give up our map_XYZ functions then.

Not as far as I can tell with additional trait parameters (each defaulting to a derivative of TBehaviour). See PR above.

Unfortunately I don't see a way to use TBehaviour in NetworkBehaviourAction. Unless I do so, compilation fails.

Let's move the discussion to mxinden#5.

mxinden · 2021-08-25T09:21:57Z

Reviews of all kinds are very much appreciated! (//CC @thomaseizinger in case you have time and interest.)

thomaseizinger · 2021-08-25T11:59:24Z

swarm/src/behaviour.rs

+    /// (ie. the objects returned by `new_handler`) can communicate by passing messages. Messages
+    /// sent from the handler to the behaviour are injected with [`NetworkBehaviour::inject_event`],
+    /// and the behaviour can send a message to the handler by making [`NetworkBehaviour::poll`]
+    /// return [`NetworkBehaviourAction::SendEvent`].


Did you rename it? Otherwise I think this is actually called NotifyHandler :)

👍 It was previously SendEvent, I changed it to ['NetworkBehaviour::SendEvent']. 4d0faf9 changes it to ['NetworkBehaviour::NotifyHandler'].

thomaseizinger

Nice work, thanks for taking this on!

Left a few nits but from what I can see, this should work :)

Maybe a test or an example within libp2p-swarm that demonstrates this usage could be useful? I am talking about the actual usecase of carrying a message within the handler and reporting back to the user in case it failed or sending it directly once the connection succeeds.

core/src/connection/manager.rs

core/src/connection/pool.rs

core/src/network.rs

thomaseizinger · 2021-08-26T01:39:26Z

core/src/network.rs

+        handler: THandler,
+    },
+    /// The dialing attempt is rejected because the peer being dialed is the local peer.
+    LocalPeerId { handler: THandler },


Maybe SelfDial is a more descriptive name?

I want to stress the fact that the peer ids match. This would e.g. not work by matching multiaddresses. All that said, I don't think LocalPeerId is the perfect name either.

thomaseizinger · 2021-08-26T01:42:56Z

core/src/network/event.rs

+        match attempts_remaining {
+            DialAttemptsRemaining::Some(attempts) => (*attempts).into(),
+            DialAttemptsRemaining::None(_) => 0,
+        }


I think this could be written like this at which point it is questionable that we need the From implementation?

Generally, wouldn't this be cleaner to be a regular function within an impl DialAttemptsRemaining?

Suggested change

match attempts_remaining {

DialAttemptsRemaining::Some(attempts) => (*attempts).into(),

DialAttemptsRemaining::None(_) => 0,

}

attempts_remaining.map(|attempts| attempts.get()).unwrap_or(0)

I don't see a way to do the above, given that the close of map_err would take ownership of handler, which is later on used. I don't think rustc is able to detect the early return.

I guess this comment should go somewhere else? Or is this a GitHub glitch?

thomaseizinger · 2021-08-26T01:43:50Z

protocols/gossipsub/src/behaviour/tests.rs

@@ -747,7 +747,7 @@ mod tests {

        // check that our subscriptions are sent to each of the peers
        // collect all the SendEvents
-        let send_events: Vec<&NetworkBehaviourAction<Arc<GossipsubHandlerIn>, GossipsubEvent>> = gs
+        let send_events: Vec<_> = gs


Co-authored-by: Thomas Eizinger <[email protected]>

mxinden · 2021-08-26T20:12:17Z

Maybe a test or an example within libp2p-swarm that demonstrates this usage could be useful? I am talking about the actual usecase of carrying a message within the handler and reporting back to the user in case it failed or sending it directly once the connection succeeds.

Yep. Good idea. I added a doc example, which thus both showcases the use-case and is continuously tested. (814ff4b)

Also note that the DCUtR implementation builds on top of this patch already: #2076

swarm/src/behaviour.rs

thomaseizinger · 2021-08-27T02:39:28Z

swarm/src/lib.rs

+
+    /// [`NetworkBehaviourAction::DialAddress`] and [`NetworkBehaviourAction::DialPeer`] require a
+    /// handler. This handler can be used to carry state. See corresponding doc comments.
+    #[test]
+    fn use_handler_to_carry_state() {}


Is that a left-over from adding a dedicated test but then resorting to a doc-test?

Ups. Thanks for the pointer. Removed in 60c7261.

Co-authored-by: Thomas Eizinger <[email protected]>

mxinden · 2021-08-30T12:21:39Z

Unless there are any objections or further suggestions, I will merge here sometime this week.

thomaseizinger

LGTM

protocols/gossipsub/src/behaviour.rs

Co-authored-by: Thomas Eizinger <[email protected]>

* protocols/gossipsub: Fix inconsistency in mesh peer tracking (libp2p#2189) Co-authored-by: Age Manning <[email protected]> * misc/metrics: Add auxiliary crate to record events as OpenMetrics (libp2p#2063) This commit adds an auxiliary crate recording protocol and Swarm events and exposing them as metrics in the OpenMetrics format. * README: Mention [email protected] * examples/: Add file sharing example (libp2p#2186) Basic file sharing application with peers either providing or locating and getting files by name. While obviously showcasing how to build a basic file sharing application, the actual goal of this example is **to show how to integrate rust-libp2p into a larger application**. Architectural properties - Clean clonable async/await interface ([`Client`]) to interact with the network layer. - Single task driving the network layer, no locks required. * examples/README: Give an overview over the many examples (libp2p#2194) * protocols/kad: Enable filtering of (provider) records (libp2p#2163) Introduce `KademliaStoreInserts` option, which allows to filter records. Co-authored-by: Max Inden <[email protected]> * swarm/src/protocols_handler: Impl ProtocolsHandler on either::Either (libp2p#2192) Implement ProtocolsHandler on either::Either representing either of two ProtocolsHandler implementations. Co-authored-by: Thomas Eizinger <[email protected]> * *: Make libp2p-core default features optional (libp2p#2181) Co-authored-by: Max Inden <[email protected]> * core/: Remove DisconnectedPeer::set_connected and Pool::add (libp2p#2195) This logic seems to be a leftover of libp2p#889 and unused today. * protocols/gossipsub: Use ed25519 in tests (libp2p#2197) With f2905c0 the secp256k1 feature is disabled by default. Instead of enabling it in the dev-dependency, simply use ed25519. * build(deps): Update minicbor requirement from 0.10 to 0.11 (libp2p#2200) Updates the requirements on [minicbor](https://gitlab.com/twittner/minicbor) to permit the latest version. - [Release notes](https://gitlab.com/twittner/minicbor/tags) - [Changelog](https://gitlab.com/twittner/minicbor/blob/master/CHANGELOG.md) - [Commits](https://gitlab.com/twittner/minicbor/compare/minicbor-v0.10.0...minicbor-v0.11.0) --- updated-dependencies: - dependency-name: minicbor dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): Update salsa20 requirement from 0.8 to 0.9 (libp2p#2206) * build(deps): Update salsa20 requirement from 0.8 to 0.9 Updates the requirements on [salsa20](https://github.com/RustCrypto/stream-ciphers) to permit the latest version. - [Release notes](https://github.com/RustCrypto/stream-ciphers/releases) - [Commits](RustCrypto/stream-ciphers@ctr-v0.8.0...salsa20-v0.9.0) --- updated-dependencies: - dependency-name: salsa20 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * *: Bump pnet to v0.22 Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Max Inden <[email protected]> * *: Dial with handler and return handler on error and closed (libp2p#2191) Require `NetworkBehaviourAction::{DialPeer,DialAddress}` to contain a `ProtocolsHandler`. This allows a behaviour to attach custom state to its handler. The behaviour would no longer need to track this state separately during connection establishment, thus reducing state required in a behaviour. E.g. in the case of `libp2p-kad` the behaviour can include a `GetRecord` request in its handler, or e.g. in the case of `libp2p-request-response` the behaviour can include the first request in the handler. Return `ProtocolsHandler` on connection error and close. This allows a behaviour to extract its custom state previously included in the handler on connection failure and connection closing. E.g. in the case of `libp2p-kad` the behaviour could extract the attached `GetRecord` from the handler of the failed connection and then start another connection attempt with a new handler with the same `GetRecord` or bubble up an error to the user. Co-authored-by: Thomas Eizinger <[email protected]> * core/: Remove deprecated read/write functions (libp2p#2213) Co-authored-by: Max Inden <[email protected]> * protocols/ping: Revise naming of symbols (libp2p#2215) Co-authored-by: Max Inden <[email protected]> * protocols/rendezvous: Implement protocol (libp2p#2107) Implement the libp2p rendezvous protocol. > A lightweight mechanism for generalized peer discovery. It can be used for bootstrap purposes, real time peer discovery, application specific routing, and so on. Co-authored-by: rishflab <[email protected]> Co-authored-by: Daniel Karzel <[email protected]> * core/src/network/event.rs: Fix typo (libp2p#2218) * protocols/mdns: Do not fire all timers at the same time. (libp2p#2212) Co-authored-by: Max Inden <[email protected]> * misc/metrics/src/kad: Set query_duration lowest bucket to 0.1 sec (libp2p#2219) Probability for a Kademlia query to return in less than 100 milliseconds is low, thus increasing the lower bucket to improve accuracy within the higher ranges. * misc/metrics/src/swarm: Expose role on connections_closed (libp2p#2220) Expose whether closed connection was a Dialer or Listener. * .github/workflows/ci.yml: Use clang 11 (libp2p#2233) * protocols/rendezvous: Update prost (libp2p#2226) Co-authored-by: Max Inden <[email protected]> * *: Fix clippy warnings (libp2p#2227) * swarm-derive/: Make event_process = false the default (libp2p#2214) Co-authored-by: Max Inden <[email protected]> Co-authored-by: Max Inden <[email protected]> Co-authored-by: Age Manning <[email protected]> Co-authored-by: Ruben De Smet <[email protected]> Co-authored-by: Thomas Eizinger <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: rishflab <[email protected]> Co-authored-by: Daniel Karzel <[email protected]> Co-authored-by: David Craven <[email protected]>

Fix incoming dial failure handling for connection pool behaviour. Since the recent update of libp2p, [this change](libp2p/rust-libp2p#2191) introduces state to dial request that can be transmitted in `inject_dial_failure`. So there are a couple of cases where the function is called but we can ignore the error since the connection is still to be completed or is already connected.

Fix incoming dial failure handling for connection pool behaviour. Since the recent update of libp2p, [this change](libp2p/rust-libp2p#2191) introduces state to dial request that can be transmitted in `inject_dial_failure`. So there are a couple of cases where the function is called but we can ignore the error since the connection is still to be completed or is already connected. This fixes #447.

Fix incoming dial failure handling for connection pool behavior. Since the recent update of libp2p, [this change](libp2p/rust-libp2p#2191) introduces state to dial request that can be transmitted in `inject_dial_failure`. So there are a couple of cases where the function is called but we can ignore the error since the connection is still to be completed or is already connected. This fixes #447.

mxinden added 3 commits August 12, 2021 16:17

core/: Return handler on connection error and closed

a332591

swarm/: Inject handler on connection error and closed

46904a6

swarm/src/behaviour: Provide handler with Dial and DialAddr

d756be1

thomaseizinger reviewed Aug 16, 2021

View reviewed changes

mxinden added 11 commits August 18, 2021 15:09

Merge branch 'libp2p/master' into handler

595623f

swarm/src/behaviour: Add default trait para on NetworkBehaviourAction

705842f

core/src/connection/manager: Fully close a task on disconnect

d744b4a

core/: Remove DisconnectedPeer::set_connected and Pool::add

5c2aef6

This logic seems to be a leftover of libp2p#889 and unused today.

core/src/connection: Report ConnectionLimit through task

ce0d278

core/: Emit Event::Failed on aborted pending connection

425e777

core/tests: Adjust to type changes

5ff6397

core/CHANGELOG: Add entry for ConnectionLimit change

7d9285f

protocols/*: Update

682f6be

Merge branch 'libp2p/master' into handler

9262c03

protocols/*: Update

62c5e13

thomaseizinger reviewed Aug 19, 2021

View reviewed changes

mxinden added 8 commits August 20, 2021 13:44

swarm-derive: Adjust to changes

174693a

core/: Fix ConectionClose and PendingAborted reporting

a78de13

*: Format with rustfmt

d4960b7

core/src/connection: Remove outdated doc comment

b73139a

swarm/src/toggle: Fix TODOs

aa02e5f

protocols/: Remove unused imports

2c9f0d3

Merge branch 'libp2p/master' into handler

6d7c73a

core/src/network/event: Use NoZeroU32

a56980e

mxinden mentioned this pull request Aug 24, 2021

swarm/src/behaviour: Make NetworkBehaviourAction generic over TBehaviour mxinden/rust-libp2p#5

Closed

mxinden added 5 commits August 24, 2021 11:10

swarm/src/protocols_handler: Rename to into_protocols_handler

1c3ed2e

swarm/src/behaviour: Introduce NetworkBehaviour::inject_listen_failure

32fc84e

swarm/src/lib: Inject handler on DialPeerCondition false

7ea4908

core/src/connection: Assume manager to always close handler

fbd4681

swarm-derive: Add comments

6787e77

swarm-derive: Fix typo

4d0faf9

thomaseizinger reviewed Aug 25, 2021

View reviewed changes

thomaseizinger reviewed Aug 26, 2021

View reviewed changes

mxinden and others added 4 commits August 26, 2021 17:25

Apply suggestions from code review

7a45e7b

Co-authored-by: Thomas Eizinger <[email protected]>

core/src/network: Revert map_err

cef949c

core/src/network: Use custom method on DialAttemptsRemaining

ceb77e5

swarm: Add doc example for carrying state in handler

814ff4b

Merge branch 'libp2p/master' into handler

90df72a

thomaseizinger reviewed Aug 27, 2021

View reviewed changes

mxinden and others added 5 commits August 30, 2021 10:43

swarm/src/lib: Remove use_handler_to_carry_state

60c7261

core/tests/network_dial_error: Use get_attempts

a2f1819

swarm/src/behaviour.rs: Use heading for doc example

b905545

Co-authored-by: Thomas Eizinger <[email protected]>

core/tests: Format with rustfmt

99f81d0

Merge branch 'master' into handler

1c5eb8e

thomaseizinger approved these changes Aug 31, 2021

View reviewed changes

protocols/gossipsub/src/behaviour.rs Outdated Show resolved Hide resolved

protocols/gossipsub/src/behaviour.rs: Remove unnecesary assignment

c09198e

Co-authored-by: Thomas Eizinger <[email protected]>

mxinden merged commit c161acf into libp2p:master Aug 31, 2021

AgeManning mentioned this pull request Sep 23, 2021

Invalid Swarm behaviour #2242

Closed

jsdanielh mentioned this pull request Nov 22, 2021

Fix network issues nimiq/core-rs-albatross#448

Merged

2 tasks

mxinden mentioned this pull request Dec 13, 2021

protocols/relay: Implement circuit relay v2 protocol #2059

Merged

elenaf9 mentioned this pull request Dec 10, 2022

feat(swarm)!: Allow NetworkBehaviours to manage incoming connections #3099

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

{core/,swarm/}: Dial with handler and return handler on error and closed #2191

{core/,swarm/}: Dial with handler and return handler on error and closed #2191

mxinden commented Aug 12, 2021

thomaseizinger left a comment

thomaseizinger Aug 16, 2021

mxinden Aug 24, 2021

thomaseizinger Aug 19, 2021

thomaseizinger Aug 19, 2021

mxinden Aug 24, 2021

mxinden commented Aug 25, 2021

thomaseizinger Aug 25, 2021

mxinden Aug 25, 2021

thomaseizinger left a comment

thomaseizinger Aug 26, 2021

mxinden Aug 26, 2021

thomaseizinger Aug 26, 2021

mxinden Aug 26, 2021

thomaseizinger Aug 27, 2021

thomaseizinger Aug 26, 2021

mxinden commented Aug 26, 2021 •

edited

Loading

thomaseizinger Aug 27, 2021

mxinden Aug 30, 2021

mxinden commented Aug 30, 2021

thomaseizinger left a comment

{core/,swarm/}: Dial with handler and return handler on error and closed #2191

{core/,swarm/}: Dial with handler and return handler on error and closed #2191

Conversation

mxinden commented Aug 12, 2021

Background

Proposal

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Aug 30, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden commented Aug 26, 2021 •

edited

Loading