swarm: Add `FromFn` `ConnectionHandler`

[thomaseizinger]

Description

This is a PoC of an idea to make it easier for users to write their own NetworkBehaviour without having to write their own ConnectionHandler. The usecases we could be serving with this are custom protocols that don't require much connection-specific handling and can be expressed as an async function, like request-response. Writing your own NetworkBehaviour is actually fairly easy. It is the ConnectionHandler that is typically cumbersome to write.

Links to any relevant issues

Open Questions

• Is the TState abstraction useful? It requires users to manually keep the state in the connection handler updated. As explained in the docs, this introduced eventual consistency which isn't always bad, one just has to be aware of it. For example, 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.
• Is the name any good?
• Is the documentation good enough?
• Is the keep_alive implementation sufficient?

Open tasks

• [x] Propose ReadyUpgrade as separate PR: https://github.com/libp2p/rust-libp2p/pull/2855
• [ ] Add a utility for notifying all handlers
• [x] Implement limit for max incoming substreams (const generic vs regular number?)
• [x] Implement limit for max pending dials?
• [ ] Fix TODOs in code
• [ ] Experiment with this handler in other protocols (ping? rendezvous? request-response?)

Change checklist

• [x] 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]

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]

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]

I think overriding keep_alive is orthogonal to the ping behaviour.

Agreed. See https://github.com/libp2p/rust-libp2p/issues/2778.

[thomaseizinger]

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.

[thomaseizinger]

@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]

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 https://github.com/libp2p/rust-libp2p/pull/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]

@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 https://github.com/libp2p/rust-libp2p/pull/1880.

[thomaseizinger]

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

[thomaseizinger]

@mxinden @elenaf9 Please have another look at this! I also added a dirty patch that converts libp2p-rendezous to use the new FromFn handler.

I also added a Shared abstraction that will "automatically" push state updates to the handler any time the state is modified. We also have a passing test now wthin the from_fn module.

Looking forward to your feedback. I think we might be onto something here. Pushing state to the connection handlers makes a many things a lot easier!

[thomaseizinger]

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.

[thomaseizinger]

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]

Rendezvous is now also fully migrated and functional.

[thomaseizinger]

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

I've updated the PR description.

[mergify[bot]]

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

[mergify[bot]]

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

[thomaseizinger]

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