swarm: deprecate KeepAlive::Until
#3844
Comments
|
cc @nathanielc |
|
Identify doesn't use |
|
#3876 is queued for merging now. In a first step towards deprecating Once all protocols have been ported, we can deprecate the enum variant itself and have our users to the same. |
Currently, the `connection_keep_alive` function of identify does not compute anything but its return value is set through the handler state machine. This is hard to understand and causes surprising behaviour because at the moment, we set `KeepAlive::No` as soon as the remote has answered our identify request. Depending on what else is happening on the connection, this might close the connection before we have successfully answered the remote's identify request. To fix this, we now compute `connection_keep_alive` based on whether we are still using the connection. Related: #3844. Pull-Request: #3876.
Has this part already been implemented? I had assumed this was a blocker to make many changes here. Was the |
Yes the identify protocol was simple in this regard because no messages are generated in the behaviour. Help in migrating other protocols away from |
|
After |
|
It is already |
|
@mxinden Do you think it makes sense to add a general When trying to transition One could even ask whether we shouldn't deprecate |
|
@thomaseizinger In the // Check keep alive status.
if self.reservation_request_future.is_none()
&& self.circuit_accept_futures.is_empty()
&& self.circuit_deny_futures.is_empty()
&& self.alive_lend_out_substreams.is_empty()
&& self.circuits.is_empty()
&& self.active_reservation.is_none()
{
match self.keep_alive {
KeepAlive::Yes => {
self.keep_alive = KeepAlive::Until(Instant::now() + Duration::from_secs(10));
}
KeepAlive::Until(_) => {}
KeepAlive::No => panic!("Handler never sets KeepAlive::No."),
}
} else {
self.keep_alive = KeepAlive::Yes;
}If I'm correct, the goal of this snippet is to keep the connection alive few more seconds, even if the condition is false. By removing the |
|
Yes, although in general, I'd like to move away from implicitly keeping connections open for longer than they are needed. For the relay protocol, what happens if we just remove the |
|
Proposal above, namely to add an At first I wondered whether a |
Yeah I think that is very hard to teach and is a bit too specialized for my taste. Also, with the strategy of initialising On the other hand, having connections not close immediately when they go idle after doing some work is definitely a problem:
The benchmarks in particular are a special-case of applications that aren't strictly p2p but more client-server style, i.e. you want to connect to very particular nodes. I opened an issue here: #4121 |
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.
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.
|
@mxinden Shall we put a |
Works for me @thomaseizinger. |
In preparation for removing this variant, we are issuing a deprecation notice. The linked issue describes why and guides users in migrating away from it. This deprecation is backwards-compatible and allows us to remove the variant in the next breaking release. We haven't migrated all internal protocols yet but that isn't strictly necessary to issue the deprecation. Related: #3844. Pull-Request: #4656.
Previously, the relay client was applying a 10 second timeout to idle connections. Instead, we now compute the connection keep alive based on whether we are still using the connection. This makes all tests pass apart from 1: `reuse_connection`. This makes sense as that connection is immediately idle once dialed. To make that test pass, we configure a global idle connection timeout of 1 second. In a real-world scenario, reusing a connection only applies if the connection is still alive due to other protocols being active. Related: #3844. Pull-Request: #4696.
|
This is all done! Many thanks to @leonzchang for all the help! |
@thomaseizinger Thanks for your guidance too! |
…p_setup/alice.rs See: libp2p/rust-libp2p#3844
Description
Deprecate the
KeepAlive::Untilvariant. Users can achieve the same thing by having a timer internally.Motivation
Understanding how to properly use
KeepAlive::Untilis difficult. It requires keeping aroundKeepAliveas a field in theConnectionHandlerotherwise it will be updated forever with a new timestamp. HavingKeepAliveas a field thus makes it more difficult to accurately compute it. It could be set fromNotoYesand back etcIdeally, a
ConnectionHandlercan just compute itsKeepAlivevalue from its internal state. There should be no reason to keep an idle connection around. If aConnectionHandleras nothing to do, it should immediately returnKeepAlive::No.To avoid connections from being shut-down directly after it is established,
NetworkBehaviours should pass initial state inhandle_established_outbound_connectionandhandle_established_inbound_connection. This way, theconnection_keep_alivefunction can correctly returnKeepAlive::Yes.Whether or not completely idle connections should be shut down or not is an application-wide decision and cannot reasonably be computed by a single
ConnectionHandler. Users should useswarm::Config::with_idle_connection_timeoutto configure that. See #4121.Tasks
Config::set_connection_keep_alive#4029Config::idle_timeout#4648KeepAlive::Until#4656Config::set_connection_idle_timeout#4675KeepAlive::Until#4642KeepAlive::Until#4676KeepAlive::UntilfromOneshotHandler#4677Config::set_connection_keep_alive#4679Config::set_connection_idle_timeout#4659The text was updated successfully, but these errors were encountered: