-
Notifications
You must be signed in to change notification settings - Fork 961
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
Labels
Comments
This was referenced Jun 26, 2023
4 tasks
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
Description
Original discussion thread: #3844
We want an
idle_connection_timeout
config on theSwarmBuilder
that triggers once a connection reportsKeepAlive::No
. The current logic is here:rust-libp2p/swarm/src/connection.rs
Lines 339 to 356 in 524bfc2
I think what we can do here is utilize the already existing
Shutdown::Later
variant and simply set it to the configured timeout in theKeepAlive::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"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.
The text was updated successfully, but these errors were encountered: