swarm: Add `FromFn` `ConnectionHandler` #2852

thomaseizinger · 2022-08-28T20:12:22Z

Description

The from_fn handler is a building block for describing protocols that have little or no connection-specific state. It encourages you to implement your protocol as a function of data without suspending the IO stream. This allows you to use regular async functions to express whatever message handshake you want.

To allow sharing of data between connections, from_fn uses an abstraction called Shared. It implements the observer pattern and pushes new versions of this data to all connections every time it gets updated. Each new stream (inbound or outbound) gets a copy of this state when created.

The stream handlers can either complete with a single event or emit several events as they progress. Thus, this abstraction should also help with the much requested "streaming response" protocol.

With from_fn, you still need to implement your own NetworkBehaviour. This is considered to be relatively easy though. Not having to implement a ConnectionHandler is a major step towards better usability.

Links to any relevant issues

Open Questions

I think we need a better name. Any suggestions?
- async_fn_handler?
Should AutoNat also be converted?
Should identify also be converted?
Should Kademlia be converted? As long as can fit the DHT into memory, this would be possible.
What do we do with libp2p-request-response?

Open tasks

Propose ReadyUpgrade as separate PR: core/upgrade/: Add ReadyUpgrade #2855
Add a utility for notifying all handlers
Implement limit for max incoming substreams (const generic vs regular number?)
Implement limit for max pending dials?
Fix TODOs in code
Experiment with this handler in other protocols (ping? rendezvous? request-response?)

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger · 2022-08-29T10:13:48Z

One problem we currently have is that with NotifyHandler on NetworkBehaviourAction, we cannot notify all connections which makes it cumbersome to use the InEvent::UpdateState variant.

@mxinden @elenaf9 What do you think of adding NotifyHandler::All?

thomaseizinger · 2022-08-30T10:00:16Z

I looked at using this implementation for the ping protocol and I think it would be doable apart from I thing: Overriding keep_alive. The current ping ConnectionHandler allows the user the configure whether the connection should be kept alive. We could drop this functionality in favor of the DummyBehaviour (should we rename that one to StaticKeepAliveBehaviour to be easier to discover? I think overriding keep_alive is orthogonal to the ping behaviour.)

Thoughts?

mxinden · 2022-09-02T06:25:04Z

I think overriding keep_alive is orthogonal to the ping behaviour.

Agreed. See #2778.

mxinden

I have to give this abstraction more thought. I find this abstraction to be very similar to libp2p-request-response. I have to think about whether it makes sense to have this next to libp2p-request-response, have this replace libp2p-request-response or have libp2p-request-response build on top of this.

swarm/src/handler/from_fn.rs

thomaseizinger · 2022-09-02T09:03:00Z

I have to give this abstraction more thought. I find this abstraction to be very similar to libp2p-request-response. I have to think about whether it makes sense to have this next to libp2p-request-response, have this replace libp2p-request-response or have libp2p-request-response build on top of this.

My preference would be to first see if we can successfully use this abstraction within our monorepo, e.g. have libp2p-request-response build on top of this.

In addition, I'd like to somehow survey people to see how they use it / whether they know about this abstraction. Perhaps more examples can also help with that.

swarm/src/handler/from_fn.rs

elenaf9

👍 I like the general idea.

I tried using this locally for rendezvous and we would have to use this mechanism to push the registration data from the behaviour down to the connections.

Did you manage to implement the rendezvous or any other protocol completely with this handler?

Is the name any good?

No strong opinion. Maybe FromUpgrade would describe it a bit better?

@mxinden @elenaf9 What do you think of adding NotifyHandler::All?

I don't see a reason not to add it, and updating the state of all handlers sounds like a valid use-case, so I'm in favor of it.

swarm/src/handler/from_fn.rs

thomaseizinger · 2022-09-04T09:39:08Z

@mxinden @elenaf9 What do you think of adding NotifyHandler::All?

I don't see a reason not to add it, and updating the state of all handlers sounds like a valid use-case, so I'm in favor of it.

Cool, I'll open a separate PR for it then :)

thomaseizinger · 2022-09-04T09:43:53Z

I tried using this locally for rendezvous and we would have to use this mechanism to push the registration data from the behaviour down to the connections.

Did you manage to implement the rendezvous or any other protocol completely with this handler?

I don't have a working example yet:

Rendezvous is tricky because we kind of want NotifyHandler::All to continuously push the state down to each handler.
Ping is tricky because it currently exposes a "keep connection alive" mechanism that doesn't really have anything to do with ping. I've opened swarm: Split off "keep alive" functionality from DummyConnectionHandler #2859 as a first step of resolving this.
I haven't tried using it for libp2p-request-response but that one should be fairly easy I think.

mxinden · 2022-09-05T06:15:46Z

@mxinden @elenaf9 What do you think of adding NotifyHandler::All?

I don't see a reason not to add it, and updating the state of all handlers sounds like a valid use-case, so I'm in favor of it.

Cool, I'll open a separate PR for it then :)

Note that this would require ConnectionHandler::InEvent to be Clone. See #1880.

thomaseizinger · 2022-09-05T16:56:14Z

@mxinden @elenaf9 What do you think of adding NotifyHandler::All?

I don't see a reason not to add it, and updating the state of all handlers sounds like a valid use-case, so I'm in favor of it.

Cool, I'll open a separate PR for it then :)

Note that this would require ConnectionHandler::InEvent to be Clone. See #1880.

Thanks for linking this. I guess we've come full circle. I was thinking that it will require Clone but I hadn't yet explored, what the implications of such a bound are. I was hoping to constrain it somehow to just that one variant but I think that is probably only possible with unsafe code.

To make this handler really useful, we'd need some kind of helper that sends an action to all connections.

This allows us to capture the `remote_peer_id` and `connection_point` of the connection.

thomaseizinger · 2022-11-11T05:16:09Z

Another update on this:

I've introduced a builder pattern to builder the initial handler. This makes it much more convenient to configure a handler that doesn't accept inbound streams for example or will never make outbound streams. A lot of this is now also enforced in the type system.

I am gonna try to full convert libp2p-rendezous at some point and see how it works. For any protocol where the substream finishes eventually, I believe this is a great fit.

At the moment, we can't convert libp2p-ping because it reuses the substream for future pings. I might look into using Stream as an abstraction though instead of Future to support protocols that keep the substream open for longer.

This clone is not very expensive and offers a huge ergonomic improvement.

thomaseizinger · 2022-11-12T01:36:59Z

The latest patches introduce the notion of a Stream handler which allows us to implement protocols such as libp2p-ping that reuse their stream and emit events as they go along.

I think this is getting close to fulfilling the requirements of #2657.

thomaseizinger · 2022-11-12T06:36:27Z

swarm/src/handler/from_fn.rs

+    pub fn register_connection(&mut self, peer_id: PeerId, id: ConnectionId) {
+        self.connections.insert((peer_id, id));
+    }
+
+    pub fn unregister_connection(&mut self, peer_id: PeerId, id: ConnectionId) {
+        self.connections.remove(&(peer_id, id));
+    }


Once #2832 is fixed, we can make this more ergonomic by just requiring a reference to the FromSwarm event and we can pick out the data as we need.

thomaseizinger · 2022-11-16T13:40:04Z

Rendezvous is now also fully migrated and functional.

thomaseizinger · 2022-11-16T13:42:17Z

protocols/rendezvous/src/server.rs

+            from_fn::OutEvent::InboundEmitted(Ok(Some(InboundOutEvent::NewRegistration(
+                new_registration,
+            )))) => {
+                let (registration, expiry) = self.registrations.add(new_registration);
+                self.next_expiry.push(expiry);
+
+                vec![NetworkBehaviourAction::GenerateEvent(
+                    Event::PeerRegistered { peer, registration },
+                )]
+            }
+            from_fn::OutEvent::InboundEmitted(Ok(Some(InboundOutEvent::RegistrationFailed {
+                error,
+                namespace,
+            }))) => {
+                vec![NetworkBehaviourAction::GenerateEvent(
+                    Event::PeerNotRegistered {
+                        peer,
+                        error,
+                        namespace,
+                    },
+                )]
+            }
+            from_fn::OutEvent::InboundEmitted(Ok(Some(InboundOutEvent::Discovered {
+                cookie,
+                registrations,
+                previous_registrations,
+            }))) => {
+                self.registrations
+                    .cookies
+                    .insert(cookie, previous_registrations);
+
+                vec![NetworkBehaviourAction::GenerateEvent(
+                    Event::DiscoverServed {
+                        enquirer: peer,
+                        registrations,
+                    },
+                )]
+            }
+            from_fn::OutEvent::InboundEmitted(Ok(Some(InboundOutEvent::DiscoverFailed {
+                error,
+            }))) => {
+                vec![NetworkBehaviourAction::GenerateEvent(
+                    Event::DiscoverNotServed {
+                        enquirer: peer,
+                        error,
+                    },
+                )]
+            }
+            from_fn::OutEvent::InboundEmitted(Ok(Some(InboundOutEvent::Unregister(namespace)))) => {
+                self.registrations.remove(namespace.clone(), peer);
+
+                vec![NetworkBehaviourAction::GenerateEvent(
+                    Event::PeerUnregistered { peer, namespace },
+                )]


A lot of code here is only present to make things observable from the outside. We don't really need to emit all these events in order for the protocol to be functional.

thomaseizinger · 2022-11-16T13:44:48Z

I think this is ready for a proper review @mxinden @jxs @elenaf9!

I've updated the PR description.

rkuhn

Yes, this does indeed look quite promising!

rkuhn · 2022-11-16T17:04:05Z

protocols/ping/src/lib.rs

+                        }
+                    })
+                }
+            })


yes, this looks pretty neat!

rkuhn · 2022-11-16T17:07:23Z

swarm/src/handler/from_fn.rs

+    pub fn with_inbound_handler<TInbound, TInboundStream>(
+        self,
+        max_inbound: usize,
+        handler: impl Fn(NegotiatedSubstream, PeerId, ConnectedPoint, Arc<TState>) -> TInboundStream


Please include the protocol name (also for outbound).

At the moment, there is only one protocol possible so it will always be that one if that function is called.

I know, and I’d like to change that. All prefabricated upgrades today ignore the negotiated protocol name, which to my mind leaves the second most important value provided by libp2p on the table (the first one being the ability to dial a PeerId).

rkuhn · 2022-11-16T17:12:54Z

swarm/src/handler/from_fn.rs

+    max_inbound: usize,
+    inbound_handler: Option<
+        Box<
+            dyn Fn(


AFAICS this restricts the part of the handler that constructs the stream processor to shared access to its closure — is there a reason for this? If not I’d prefer FnMut.

No particular reason, I guess this could be useful if you want to track connection-local state across all inbound/outbound streams?

That would only be shared across one closure though, meaning you wouldn't be able to share state between the inbound and outbound factory function.

I think we might be able to change this interface to allow for a single factory function that gets called for both inbound and outbound streams which would allow you to share state. Is that what you would be after?

Sharing state is not my concern because doing that always proceeds via shared references (of which Arc is a special case). Fn does not allow captured values to be modified (more precisely: mutably borrowed), even if they are owned.

Fn does not allow captured values to be modified (more precisely: mutably borrowed), even if they are owned.

If you want to mutate a captured value, you are effectively sharing the state within a connection across all stream handler factory invocations.

I think we are saying the same thing and I agree that this capability is useful :)

Ah, you mean “sharing along the timeline”, not “sharing in different places”. Yes, now we both know that we are in agreement :-)

rkuhn · 2022-11-16T17:16:08Z

swarm/src/handler/from_fn.rs

+/// state changes.
+///
+/// [`NetworkBehaviour`]: crate::NetworkBehaviour
+pub fn from_fn(protocol: &'static str) -> Builder<WantState> {


please allow multiple protocols, like

pub fn from_fn<I>(protocols: I) -> Builder<I, WantState> where I: IntoIterator + Clone + Send + 'static I::Item: ProtocolName { ... }

Yeah that is doable!

rkuhn · 2022-11-16T17:19:40Z

swarm/src/handler/from_fn.rs

+        {
+            if self.keep_alive.is_yes() {
+                // TODO: Make timeout configurable
+                self.keep_alive = KeepAlive::Until(Instant::now() + Duration::from_secs(10))


It feels weird that this grace period is implemented on the level of ConnectionHandler — shouldn’t it be a NetworkBehaviour setting that applies whenever KeepAlive toggles from Yes to No?

I am not happy with this either but I couldn't think of a better way to handle this. keep_alive is per connection. My initial implementation would have been a with_keep_alive_timeout configuration parameter on the builder that defaults to 30s or something.

rkuhn · 2022-11-16T17:25:45Z

swarm/src/handler/from_fn.rs

+                    PeerId,
+                    ConnectedPoint,
+                    Arc<TState>,
+                ) -> BoxStream<'static, TInbound>


I’d prefer type aliases for these function types

What do you mean exactly? As a user, you shouldn't need to come in contact this this BoxStream per se. Where would you like to see a type alias?

I only mean the readability of this source module by itself.

rkuhn · 2022-11-16T17:31:28Z

swarm/src/handler/from_fn.rs

+                .connections
+                .iter()
+                .map(|(peer_id, conn_id)| (*peer_id, *conn_id, self.shared.clone()))
+                .collect();


This erases previously undelivered wake-ups — which is fine unless it happens in a tight loop.

The old events are useless though because they contain stale state. I am not sure if there is a better way?

Right, old events are useless. Perhaps I am overthinking this, my worry is that there may be a situation where only the first (few) connections are ever notified because updates keep coming in — a fairness concern. This would be solved by randomising the notification order, or by always finishing one batch of notifications (with values updated during the process!) before starting the next. The second is deterministic but more work to implement.

Getting these things wrong usually has low chance of severe impact, but when the impact happens it is really difficult for downstream code to find the bug.

I think that we currently drain events from the NetworkBehaviour before we inject new ones into it so I don't think that this would happen but I'd have to double check that.

mergify · 2022-11-17T09:30:04Z

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

mergify · 2022-12-14T00:51:37Z

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

thomaseizinger · 2022-12-14T04:29:47Z

I think we can close this as won't-merge. It was an interesting exploration though!

thomaseizinger added 3 commits August 24, 2022 12:56

Add ReadyUpgrade

2c580d9

Add basic FromFn ConnectionHandler

b2d1c05

Add TState abstraction

230fabb

thomaseizinger requested review from mxinden and elenaf9 August 28, 2022 20:12

thomaseizinger added 2 commits August 28, 2022 22:16

Publicly expose types

58fe45b

Fix docs

ff1db42

thomaseizinger mentioned this pull request Aug 30, 2022

core/upgrade/: Add ReadyUpgrade #2855

Merged

3 tasks

thomaseizinger added 2 commits August 30, 2022 11:33

Implement limit for max inbound streams and pending dials

1b175d2

Implement std::error::Error for OpenError

69fa94f

mxinden reviewed Sep 2, 2022

View reviewed changes

swarm/src/handler/from_fn.rs Outdated Show resolved Hide resolved

swarm/src/handler/from_fn.rs Outdated Show resolved Hide resolved

thomaseizinger mentioned this pull request Sep 2, 2022

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

Open

elenaf9 reviewed Sep 2, 2022

View reviewed changes

swarm/src/handler/from_fn.rs Outdated Show resolved Hide resolved

elenaf9 reviewed Sep 2, 2022

View reviewed changes

swarm/src/handler/from_fn.rs Show resolved Hide resolved

swarm/src/handler/from_fn.rs Outdated Show resolved Hide resolved

swarm/src/handler/from_fn.rs Outdated Show resolved Hide resolved

thomaseizinger mentioned this pull request Sep 2, 2022

swarm: Split off "keep alive" functionality from DummyConnectionHandler #2859

Merged

3 tasks

thomaseizinger changed the title ~~Add FromFn ConnectionHandler~~ swarm: Add FromFn ConnectionHandler Sep 4, 2022

thomaseizinger mentioned this pull request Sep 4, 2022

swarm/src/behaviour: Add KeepAlive Behaviour #2778

Closed

thomaseizinger added 5 commits September 5, 2022 18:57

Avoid dial terminology for streams

54e6437

Remove idle_waker

635730a

Make TState configurable

b37fa5c

Finish local work before producing new work

d4079bf

Introduce FromFnProto

bc361ec

This allows us to capture the `remote_peer_id` and `connection_point` of the connection.

Make inbound streams impossible when no handler is configured

f84388f

thomaseizinger added 5 commits November 11, 2022 23:05

Allow for streaming protocols

6d1add5

Pass ConnectedPoint by value

913caf3

This clone is not very expensive and offers a huge ergonomic improvement.

Allow regular async functions to be used as handlers

e9cd5fc

Flatten errors

5801576

Implement libp2p-ping with from_fn abstraction

c15433f

thomaseizinger force-pushed the from-fn-connection-handler branch from b70272d to c15433f Compare November 12, 2022 01:35

thomaseizinger mentioned this pull request Nov 12, 2022

protocols/stream: Add a streaming-response protocol to rust-libp2p #2657

Closed

thomaseizinger commented Nov 12, 2022

View reviewed changes

thomaseizinger added 3 commits November 16, 2022 22:44

Merge branch 'master' into from-fn-connection-handler

a418d2f

Fully migrate rendezvous

628e9ab

Minimise diff

7bdd953

thomaseizinger commented Nov 16, 2022

View reviewed changes

thomaseizinger marked this pull request as ready for review November 16, 2022 13:43

thomaseizinger requested review from elenaf9 and mxinden November 16, 2022 13:52

rkuhn reviewed Nov 16, 2022

View reviewed changes

thomaseizinger added 2 commits November 18, 2022 16:29

Merge branch 'master' into from-fn-connection-handler

4485c48

Update to new behaviour interface

6f00703

This was referenced Nov 18, 2022

protocols/kad/: Replace manual procedural state machines with async/await #3130

Closed

kad: Change RecordStore trait interface - add results to methods. #3076

Closed

thomaseizinger closed this Dec 14, 2022

thomaseizinger deleted the from-fn-connection-handler branch February 24, 2023 14:46

swarm: Add FromFn ConnectionHandler #2852

swarm: Add FromFn ConnectionHandler #2852

Conversation

thomaseizinger commented Aug 28, 2022 • edited

Description

Links to any relevant issues

Open Questions

Open tasks

Change checklist

thomaseizinger commented Aug 29, 2022

thomaseizinger commented Aug 30, 2022 • edited

mxinden commented Sep 2, 2022

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger commented Sep 2, 2022

elenaf9 left a comment • edited

Choose a reason for hiding this comment

thomaseizinger commented Sep 4, 2022

thomaseizinger commented Sep 4, 2022

mxinden commented Sep 5, 2022

thomaseizinger commented Sep 5, 2022

thomaseizinger commented Nov 11, 2022

thomaseizinger commented Nov 12, 2022

Choose a reason for hiding this comment

thomaseizinger commented Nov 16, 2022

Choose a reason for hiding this comment

thomaseizinger commented Nov 16, 2022 • edited

rkuhn left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Nov 17, 2022 • edited

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

thomaseizinger Nov 18, 2022 • edited

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

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

mergify bot commented Nov 17, 2022

mergify bot commented Dec 14, 2022

thomaseizinger commented Dec 14, 2022

swarm: Add `FromFn` `ConnectionHandler` #2852

swarm: Add `FromFn` `ConnectionHandler` #2852

thomaseizinger commented Aug 28, 2022 •

edited

thomaseizinger commented Aug 30, 2022 •

edited

elenaf9 left a comment •

edited

thomaseizinger commented Nov 16, 2022 •

edited

rkuhn left a comment •

edited

thomaseizinger Nov 17, 2022 •

edited

thomaseizinger Nov 18, 2022 •

edited