feat(swarm): allow configuration to idle connection timeout #4161

startup-dreamer · 2023-07-08T12:15:54Z

Description

Previously, a connection would be shut down immediately as soon as its ConnectionHandler reports KeepAlive::No. As we have gained experience with libp2p, it turned out that this isn't ideal.

For one, tests often need to keep connections alive longer than the configured protocols require. Plus, some usecases require connections to be kept alive in general.

Both of these needs are currently served by the keep_alive::Behaviour. That one does essentially nothing other than statically returning KeepAlive::Yes from its ConnectionHandler.

It makes much more sense to deprecate keep_alive::Behaviour and instead allow users to globally configure an idle_conncetion_timeout on the Swarm. This timeout comes into effect once a ConnectionHandler reports KeepAlive::No. To start with, this timeout is 0. Together with #3844, this will allow us to move towards a much more aggressive closing of idle connections, together with a more ergonomic way of opting out of this behaviour.

Fixes #4121.

Notes & open questions

~~am I in the right direction also we might add tests for this feature.~~

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

Thanks! Looks like a good start!

I'd like us to use this directly in libp2p-swarm-test and see which tests we can remove keep_alive::Behaviour from, should be all of them but maybe something else turns up!

swarm/src/connection.rs

thomaseizinger · 2023-07-11T15:32:41Z

Hy @thomaseizinger can you guide me through how I am gonna remove keep_alive::Behaviour tests can't wrap my head around this.

Sure!

So the context is that with the current code, a connection will be immediately shut down as soon as a ConnectionHandler reports KeepAlive::No.

In many tests, we only use a single NetworkBehaviour which is quite an artificial scenario and as such, we often run into the problem that connections get shut down too quickly despite us still wanting to use them in the test.

With this new feature, we can delay that shutdown which in turn allows us to remove keep_alive::Behaviour from many tests.

I'd take the following approach:

Put a #[deprecated] attribute on keep_alive::Behaviour
Run cargo check / cargo clippy with --all-targets
Go through each usage that is a test and remove it. Most of the time, this will be as simple as deleting a field from a struct
Run the test. Likely, it will fail.
Check where the Swarm in this particular test gets constructed.
- For the usages coming from Swarm::new_ephemeral, you can extend the SwarmBuilder in there to configure a 10 sec keep alive timeout. That should be more than plenty for each test. This will likely fix a lot of tests because we extensively use that test harness in our codebase.
- For any other usage, simply extend that SwarmBuilder with a 10 second timeout.

Does that make sense? You can try yourself on one or two tests and then push a commit and we can look at it together :)

swarm/src/connection.rs

swarm/src/lib.rs

mxinden

Thank you for the help here.

swarm/src/connection.rs

startup-dreamer · 2023-07-20T20:26:32Z

Thank you for the help here.

I still wan't to work on this however I am busy in some other work so if anyone wanna take this he can.
If not I will complete this as soon as possible.

thomaseizinger · 2023-07-21T09:17:53Z

Thank you for the help here.

I still wan't to work on this however I am busy in some other work so if anyone wanna take this he can.
If not I will complete this as soon as possible.

Thank you! No pressure from our end, your help is much appreciated :)

…ing tests

thomaseizinger · 2023-07-31T12:39:56Z

@startup-dreamer I've pushed some commits that should help move this forward. In particular, I've added the deprecation attribute and converted the first test. Most of the others should be similar :)

swarm/src/lib.rs

startup-dreamer · 2023-08-01T01:24:05Z

@startup-dreamer I've pushed some commits that should help move this forward. In particular, I've added the deprecation attribute and converted the first test. Most of the others should be similar :)

graet help sir will try to move forward from here.

mergify · 2023-08-04T18:43:11Z

This pull request has merge conflicts. Could you please resolve them @startup-dreamer? 🙏

thomaseizinger

Looks good so far :)

thomaseizinger · 2023-08-05T12:35:04Z

@thomaseizinger should i wait for you to review each refactor I does or do a bunch of refactoring and then take the review

Looks good so far, you are on the right track :)

examples/metrics/src/main.rs

thomaseizinger · 2023-09-12T05:40:31Z

@mxinden I spent some cycles on this to push it over the finish line as there wasn't much missing. Please review!

mxinden

One comment, otherwise this looks good to me.

Let's have this run on am6-rust (IPFS bootstrap node staging server) once merged into master before cutting a release. Will be deployed automatically via our nightly job once merged.

swarm/src/connection.rs

thomaseizinger · 2023-09-17T21:33:03Z

Let's have this run on am6-rust (IPFS bootstrap node staging server) once merged into master before cutting a release. Will be deployed automatically via our nightly job once merged.

With or without setting a custom timeout?

mergify · 2023-09-18T09:11:06Z

This pull request has merge conflicts. Could you please resolve them @startup-dreamer? 🙏

swarm/Cargo.toml

mxinden · 2023-09-19T15:35:03Z

Let's have this run on am6-rust (IPFS bootstrap node staging server) once merged into master before cutting a release. Will be deployed automatically via our nightly job once merged.

With or without setting a custom timeout?

I suggest without, to validate that the status-quo doesn't change.

mxinden

Cool. Thank you for the tests!

mxinden · 2023-09-19T15:40:26Z

@thomaseizinger can you address the CI failures? Otherwise this is ready to merge from my side.

thomaseizinger · 2023-09-19T22:19:46Z

@thomaseizinger can you address the CI failures? Otherwise this is ready to merge from my side.

Done! I'll send it then :)

Previously, a connection would be shut down immediately as soon as its `ConnectionHandler` reports `KeepAlive::No`. As we have gained experience with libp2p, it turned out that this isn't ideal. For one, tests often need to keep connections alive longer than the configured protocols require. Plus, some usecases require connections to be kept alive in general. Both of these needs are currently served by the `keep_alive::Behaviour`. That one does essentially nothing other than statically returning `KeepAlive::Yes` from its `ConnectionHandler`. It makes much more sense to deprecate `keep_alive::Behaviour` and instead allow users to globally configure an `idle_conncetion_timeout` on the `Swarm`. This timeout comes into effect once a `ConnectionHandler` reports `KeepAlive::No`. To start with, this timeout is 0. Together with libp2p#3844, this will allow us to move towards a much more aggressive closing of idle connections, together with a more ergonomic way of opting out of this behaviour. Fixes libp2p#4121. Pull-Request: libp2p#4161.

Previously, a connection would be shut down immediately as soon as its `ConnectionHandler` reports `KeepAlive::No`. As we have gained experience with libp2p, it turned out that this isn't ideal. For one, tests often need to keep connections alive longer than the configured protocols require. Plus, some usecases require connections to be kept alive in general. Both of these needs are currently served by the `keep_alive::Behaviour`. That one does essentially nothing other than statically returning `KeepAlive::Yes` from its `ConnectionHandler`. It makes much more sense to deprecate `keep_alive::Behaviour` and instead allow users to globally configure an `idle_conncetion_timeout` on the `Swarm`. This timeout comes into effect once a `ConnectionHandler` reports `KeepAlive::No`. To start with, this timeout is 0. Together with #3844, this will allow us to move towards a much more aggressive closing of idle connections, together with a more ergonomic way of opting out of this behaviour. Fixes #4121. Pull-Request: #4161.

startup-dreamer changed the title ~~added idle_connection_timeout for swarm builder~~ feat(SwarmBuilder): added idle_connection_timeout for swarm builder Jul 8, 2023

thomaseizinger reviewed Jul 9, 2023

View reviewed changes

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

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

thomaseizinger changed the title ~~feat(SwarmBuilder): added idle_connection_timeout for swarm builder~~ feat(swarm): allow configuration of idle connection timeout Jul 9, 2023

added idle_timeout and test for swarm builder

073cee1

startup-dreamer force-pushed the feat/idle_connection_timeout branch from a0a5d44 to 073cee1 Compare July 11, 2023 13:50

This comment was marked as resolved.

Sign in to view

thomaseizinger reviewed Jul 11, 2023

View reviewed changes

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

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

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

mxinden reviewed Jul 20, 2023

View reviewed changes

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

thomaseizinger added 2 commits July 31, 2023 14:29

Allow configuration of idle timeout via SwarmBuilder

459cb4d

Set idle_connection_timeout in Swarm::new_ephemeral and rewrite p…

6134c56

…ing tests

thomaseizinger reviewed Jul 31, 2023

View reviewed changes

swarm/src/lib.rs Show resolved Hide resolved

thomaseizinger added 2 commits July 31, 2023 14:50

Fix deprecation message

837a038

Move deprecation attribute to module declaration instead

647e273

refactor: deprecated keep_alive in transports tls smoke test

3dcec14

This comment was marked as outdated.

Sign in to view

refactor: deprecated keep_alive in interop-test suite

fdc555b

refactor: deprecated keep_alive in transport webrtc example

d4eea12

startup-dreamer changed the title ~~feat(swarm): allow configuration of idle connection timeout~~ feat(swarm): allow configuration to idle connection timeout Aug 5, 2023

thomaseizinger reviewed Aug 5, 2023

View reviewed changes

fix: idle timeout test case and added zero constant in comparison

42b9e3b

thomaseizinger reviewed Aug 6, 2023

View reviewed changes

examples/metrics/src/main.rs Outdated Show resolved Hide resolved

refactor: deprecated keep_alive from swarm lib tests

f91d242

startup-dreamer force-pushed the feat/idle_connection_timeout branch from 27f2345 to f91d242 Compare August 6, 2023 08:26

thomaseizinger added 3 commits September 12, 2023 15:02

Fix tests and bug in impl!

f86240e

Fix missing import in tutorial

0b73c5e

Remove deprecated imports

e338178

thomaseizinger requested a review from mxinden September 12, 2023 05:12

thomaseizinger added 2 commits September 12, 2023 15:12

Merge branch 'master' into feat/idle_connection_timeout

1ae9af9

Add changelog entry

88f1f1b

thomaseizinger added 3 commits September 12, 2023 15:40

Merge branch 'master' into feat/idle_connection_timeout

23311ef

Merge branch 'master' into feat/idle_connection_timeout

98aa6b0

Merge branch 'master' into feat/idle_connection_timeout

3737483

mxinden reviewed Sep 16, 2023

View reviewed changes

swarm/src/connection.rs Show resolved Hide resolved

Shutdown ConnectionHandler after max(KeepAlive::Until, idle_timeout)

f566a48

thomaseizinger requested a review from mxinden September 17, 2023 23:46

thomaseizinger added this to the Simplify idle connection management milestone Sep 17, 2023

Merge branch 'master' into feat/idle_connection_timeout

91705f9

thomaseizinger reviewed Sep 18, 2023

View reviewed changes

swarm/Cargo.toml Outdated Show resolved Hide resolved

Update swarm/Cargo.toml

21d4d17

mxinden approved these changes Sep 19, 2023

View reviewed changes

thomaseizinger added 2 commits September 20, 2023 08:18

Update webrtc example to remove keep-alive

4287c50

Merge branch 'master' into feat/idle_connection_timeout

ba4cb8d

thomaseizinger added the send-it label Sep 19, 2023

mergify bot merged commit c52a2fc into libp2p:master Sep 19, 2023
69 checks passed

startup-dreamer deleted the feat/idle_connection_timeout branch September 20, 2023 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(swarm): allow configuration to idle connection timeout #4161

feat(swarm): allow configuration to idle connection timeout #4161

startup-dreamer commented Jul 8, 2023 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

This comment was marked as resolved.

thomaseizinger commented Jul 11, 2023

mxinden left a comment

startup-dreamer commented Jul 20, 2023

thomaseizinger commented Jul 21, 2023

thomaseizinger commented Jul 31, 2023

startup-dreamer commented Aug 1, 2023

This comment was marked as outdated.

mergify bot commented Aug 4, 2023

thomaseizinger left a comment

thomaseizinger commented Aug 5, 2023

thomaseizinger commented Sep 12, 2023

mxinden left a comment

thomaseizinger commented Sep 17, 2023

mergify bot commented Sep 18, 2023

mxinden commented Sep 19, 2023

mxinden left a comment

mxinden commented Sep 19, 2023

thomaseizinger commented Sep 19, 2023

feat(swarm): allow configuration to idle connection timeout #4161

feat(swarm): allow configuration to idle connection timeout #4161

Conversation

startup-dreamer commented Jul 8, 2023 • edited by thomaseizinger Loading

Description

Notes & open questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

thomaseizinger commented Jul 11, 2023

mxinden left a comment

Choose a reason for hiding this comment

startup-dreamer commented Jul 20, 2023

thomaseizinger commented Jul 21, 2023

thomaseizinger commented Jul 31, 2023

startup-dreamer commented Aug 1, 2023

This comment was marked as outdated.

mergify bot commented Aug 4, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Aug 5, 2023

thomaseizinger commented Sep 12, 2023

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger commented Sep 17, 2023

mergify bot commented Sep 18, 2023

mxinden commented Sep 19, 2023

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Sep 19, 2023

thomaseizinger commented Sep 19, 2023

startup-dreamer commented Jul 8, 2023 •

edited by thomaseizinger

Loading