swarm: Split off "keep alive" functionality from `DummyConnectionHandler` by thomaseizinger · Pull Request #2859 · libp2p/rust-libp2p

thomaseizinger · 2022-08-31T10:00:38Z

Description

Previously, the DummyConnectionHandler offered a "keep alive" functionality, i.e. it allowed users to set the value of what is returned from ConnectionHandler::keep_alive. This handler is primarily used in tests or NetworkBehaviours that don't open any connections (like mDNS). In all of these cases, it is statically known whether we want to keep connections alive. As such, this functionality is better represented by a static KeepAliveConnectionHandler that always returns KeepAlive::Yes and a DummyConnectionHandler that always returns KeepAlive::No.

To follow the naming conventions described in #2217, we introduce a top-level keep_alive and dummy behaviour in libp2p-swarm that contains both the NetworkBehaviour and ConnectionHandler implementation for either case.

Links to any relevant issues

swarm: Add FromFn ConnectionHandler #2852: To actually replace "Ping"'s ConnectionHandler with a FromFn ConnectionHandler, we need to decouple the "Keep Alive" functionality from the Ping protocol. If this PR is accepted, I'd send a follow-up to remove the keep-alive functionality from ping in favor of using the KeepAliveBehaviour.

Open Questions

~~Implementations on () are not as easy to discover. Should we retain DummyBehaviour and DummyConnectionHandler but strip the keep-alive logic from them?~~
~~Should we rename the KeepAlive to KeepAlivePolicy (or something else) to avoid ambiguity with behaviour::KeepAlive?~~
~~Should we strip the keep alive functionality from libp2p-ping as part of this PR or in a follow-up?~~

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

() is the default associated type when creating implementations of traits with IDEs like IntelliJ Rust or rust-analyzer. Providing a `ConnectionHandler` implementation for those should make it easier for users to get started.

mxinden · 2022-09-09T08:45:31Z

Implementations on () are not as easy to discover.

I have to give this more thought. I agree that it makes it harder to discover. Also I don't find the fact that () implements a dummy NetworkBehaviour intuitive.

Should we retain DummyBehaviour and DummyConnectionHandler but strip the keep-alive logic from them?

I think this is a good idea.

thomaseizinger · 2022-09-12T01:59:21Z

Should we retain DummyBehaviour and DummyConnectionHandler but strip the keep-alive logic from them?

I think this is a good idea.

I'll progress in this direction then!

thomaseizinger · 2022-09-12T02:36:04Z

This is ready for another review @mxinden !

mxinden

Thanks for cleaning this up. Small suggestions on the naming of the two NetworkBehaviour implementations.

swarm/src/lib.rs

mxinden · 2022-09-15T14:07:49Z

Can you update the pull request description?

thomaseizinger · 2022-09-16T01:35:36Z

Can you update the pull request description?

Updated. Also added new questions.

It is better for these to be grouped together rather than by what interface they are implementing.

thomaseizinger · 2022-09-27T02:07:01Z

This is ready to merge from my end. Feel free to merge in case you want to keep the naming as is or want that to happen in a follow-up @thomaseizinger.

I ended up introducing top-level dummy and keep_alive modules in libp2p_swarm that export both, the ConnectionHandler and the corresponding NetworkBehaviour.

thomaseizinger · 2022-09-27T02:10:16Z

Changelog and PR description are up to date. This is ready to be merged from my end.

mxinden · 2022-09-29T15:06:56Z

@thomaseizinger can you take a look at the clippy failure?

error: unused import: `NetworkBehaviour`
Error:   --> examples/ping.rs:44:21
   |
44 | use libp2p::swarm::{NetworkBehaviour, Swarm, SwarmEvent};
   |                     ^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

thomaseizinger · 2022-09-29T23:16:44Z

@thomaseizinger can you take a look at the clippy failure?

error: unused import: `NetworkBehaviour`
Error:   --> examples/ping.rs:44:21
   |
44 | use libp2p::swarm::{NetworkBehaviour, Swarm, SwarmEvent};
   |                     ^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

Ah yes, I expected that one to happen eventually. I originally got the warning because #2932 wasn't yet fixed!

…2p into unit-connection-handler

mxinden

Ready to merge from my end otherwise.

mxinden · 2022-10-04T16:52:46Z

swarm/src/dummy.rs

+use std::task::{Context, Poll};
+use void::Void;
+
+/// Implementation of [`NetworkBehaviour`] that doesn't do anything other than keep all connections alive.


It does not keep connections alive, right?

Absolutely correct. Thanks for spotting the typo!

…haviour See: libp2p/rust-libp2p#2859

thomaseizinger added 3 commits August 30, 2022 12:46

Add implementation of ConnectionHandler for ()

05fa3c7

() is the default associated type when creating implementations of traits with IDEs like IntelliJ Rust or rust-analyzer. Providing a `ConnectionHandler` implementation for those should make it easier for users to get started.

Replace usages of DummyConnectionHandler with ()

907f025

Split DummyBehaviour into KeepAliveBehaviour and unit-impl

ec843b0

thomaseizinger requested review from elenaf9 and mxinden August 31, 2022 10:00

thomaseizinger added 2 commits September 2, 2022 20:02

Fix doc test by implementing Default

237b867

Merge branch 'master' into unit-connection-handler

48c0e36

thomaseizinger changed the title ~~RFC: Implement NetworkBehaviour and ConnectionHandler for ()~~ swarm: Implement NetworkBehaviour and ConnectionHandler for () Sep 2, 2022

thomaseizinger marked this pull request as ready for review September 2, 2022 18:08

This was referenced Sep 4, 2022

swarm: Add FromFn ConnectionHandler #2852

Closed

swarm/src/behaviour: Add KeepAlive Behaviour #2778

Closed

thomaseizinger added 9 commits September 12, 2022 12:09

Re-introduce Dummy{ConnectionHandler,NetworkBehaviour}

ba06870

Restore DummyNetworkBehaviour where it was used before

ce7efb8

Remove license text

6c93d6d

Apply same member order as declaration

6db3dee

Use void::unreachable instead of panicking

5aa920d

Merge branch 'master' into unit-connection-handler

805d1c3

Add changelog entry and bump version

3797dcf

Use correct type in tests

b236e7f

Bump versions

729d57e

thomaseizinger changed the title ~~swarm: Implement NetworkBehaviour and ConnectionHandler for ()~~ swarm: Split off "keep alive" functionality from DummyConnectionHandler Sep 12, 2022

mxinden reviewed Sep 15, 2022

View reviewed changes

swarm/src/lib.rs Outdated Show resolved Hide resolved

swarm/src/lib.rs Outdated Show resolved Hide resolved

swarm/src/lib.rs Show resolved Hide resolved

thomaseizinger added 2 commits September 16, 2022 11:30

Rename network behaviours to remove Behaviour postfix

ea3fa2a

Merge branch 'master' into unit-connection-handler

ca4adbe

thomaseizinger requested a review from mxinden September 16, 2022 01:34

thomaseizinger added 3 commits September 27, 2022 11:41

Merge branch 'master' into unit-connection-handler

8a2f14a

Have dummy and keep-alive Behaviour adhere to naming convention

cd7bd6d

Merge Behaviour and ConnectionHandler into dummy and keep_alive modules

d847b47

It is better for these to be grouped together rather than by what interface they are implementing.

thomaseizinger mentioned this pull request Sep 27, 2022

All crates should have a consistent API & naming #2217

Open

18 tasks

Update changelog

3b8f1b0

thomaseizinger requested a review from mxinden September 27, 2022 02:08

thomaseizinger linked an issue Sep 27, 2022 that may be closed by this pull request

swarm/src/behaviour: Add KeepAlive Behaviour #2778

Closed

Merge branch 'master' into unit-connection-handler

feb4703

thomaseizinger added 9 commits September 30, 2022 09:17

Merge branch 'master' into unit-connection-handler

ad2f597

Merge branch 'unit-connection-handler' of github.com:libp2p/rust-libp…

780460f

…2p into unit-connection-handler

Fix docs

a750b07

Merge branch 'master' into unit-connection-handler

0a167a1

Fix changelog

b84857f

Fix ping changelog

add20ce

Fix imports

e6f3c5a

Fix empty module

212b787

Merge branch 'master' into unit-connection-handler

8a83545

mxinden approved these changes Oct 4, 2022

View reviewed changes

thomaseizinger added 3 commits October 5, 2022 08:23

Merge branch 'master' into unit-connection-handler

d50c0d2

Fix typo

54ff6f7

Merge branch 'master' into unit-connection-handler

1f98636

mxinden merged commit bdf9209 into master Oct 5, 2022

thomaseizinger deleted the unit-connection-handler branch October 5, 2022 20:44

thomaseizinger mentioned this pull request Oct 10, 2022

transports/onion: Add dial-only implementation of Transport #2899

Closed

4 tasks

thomaseizinger mentioned this pull request Oct 21, 2022

Update for rust-libp2p 0.49 release libp2p/test-plans#60

Merged

binarybaron added a commit to eigenwallet/core that referenced this pull request Oct 24, 2024

fix(libp2p): Swarm keeps connections alive forever instead of ping be…

dfd6180

…haviour See: libp2p/rust-libp2p#2859

Conversation

thomaseizinger commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links to any relevant issues

Open Questions

Change checklist

Uh oh!

mxinden commented Sep 9, 2022

Uh oh!

thomaseizinger commented Sep 12, 2022

Uh oh!

thomaseizinger commented Sep 12, 2022

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxinden commented Sep 15, 2022

Uh oh!

thomaseizinger commented Sep 16, 2022

Uh oh!

thomaseizinger commented Sep 27, 2022

Uh oh!

thomaseizinger commented Sep 27, 2022

Uh oh!

mxinden commented Sep 29, 2022

Uh oh!

thomaseizinger commented Sep 29, 2022

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thomaseizinger commented Aug 31, 2022 •

edited

Loading