Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

swarm: Introduce an idle_connection_timeout config option #4121

Closed
Tracked by #4306
thomaseizinger opened this issue Jun 26, 2023 · 0 comments · Fixed by #4161
Closed
Tracked by #4306

swarm: Introduce an idle_connection_timeout config option #4121

thomaseizinger opened this issue Jun 26, 2023 · 0 comments · Fixed by #4161

Comments

@thomaseizinger
Copy link
Contributor

Description

Original discussion thread: #3844

We want an idle_connection_timeout config on the SwarmBuilder that triggers once a connection reports KeepAlive::No. The current logic is here:

let keep_alive = handler.connection_keep_alive();
match (&mut *shutdown, keep_alive) {
(Shutdown::Later(timer, deadline), KeepAlive::Until(t)) => {
if *deadline != t {
*deadline = t;
if let Some(dur) = deadline.checked_duration_since(Instant::now()) {
timer.reset(dur)
}
}
}
(_, KeepAlive::Until(t)) => {
if let Some(dur) = t.checked_duration_since(Instant::now()) {
*shutdown = Shutdown::Later(Delay::new(dur), t)
}
}
(_, KeepAlive::No) => *shutdown = Shutdown::Asap,
(_, KeepAlive::Yes) => *shutdown = Shutdown::None,
};

I think what we can do here is utilize the already existing Shutdown::Later variant and simply set it to the configured timeout in the KeepAlive::No branch.

By default, the timeout should be 0 seconds.

Motivation

Several usecases like tests, examples, benchmarks and more client-server style applications often want to keep connections alive even if there currently isn't any work happening on the connection.

This will unblock #4029. At the same time with introducing this feature, we should deprecate keep_alive::Behaviour and recommend users to simply set the new configuration option to:

  • u64::MAX if they want to keep the connection alive "forever"
  • a value that makes sense in their context

For tests for example, we would probably be fine to set the value to something like 10 seconds. We can do that here: https://github.com/libp2p/rust-libp2p/blob/master/swarm-test/src/lib.rs#L221

As a result, we can likely remove the usage of keep_alive::Behaviour from several tests.

Are you planning to do it yourself in a pull request?

Maybe but would appreciate help.

@thomaseizinger thomaseizinger added this to the Simplify idle connection management milestone Sep 17, 2023
@mergify mergify bot closed this as completed in #4161 Sep 19, 2023
mergify bot pushed a commit that referenced this issue Sep 19, 2023
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.
thomaseizinger pushed a commit to dgarus/rust-libp2p that referenced this issue Sep 20, 2023
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.
thomaseizinger pushed a commit that referenced this issue Sep 21, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant