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: Default to keeping connections alive whilst there are active streams #4520

Closed
Tracked by #4306
thomaseizinger opened this issue Sep 19, 2023 · 2 comments · Fixed by #4595
Closed
Tracked by #4306

swarm: Default to keeping connections alive whilst there are active streams #4520

thomaseizinger opened this issue Sep 19, 2023 · 2 comments · Fixed by #4595

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Sep 19, 2023

Currently, whether or not a connection has active streams does not affect its keep-alive. However, for most protocols, that is the default. The only exception that we know of here is the ping protocol.

As a first step, we want to incorporate "counting" of active streams in the keep-alive algorithm of a connection. This code lives here:

// Ask the handler whether it wants the connection (and the handler itself)
// to be kept alive, which determines the planned shutdown, if any.
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(new_duration) = deadline.checked_duration_since(Instant::now())
{
let effective_keep_alive = max(new_duration, *idle_timeout);
timer.reset(effective_keep_alive)
}
}
}
(_, KeepAlive::Until(earliest_shutdown)) => {
if let Some(requested_keep_alive) =
earliest_shutdown.checked_duration_since(Instant::now())
{
let effective_keep_alive = max(requested_keep_alive, *idle_timeout);
// Important: We store the _original_ `Instant` given by the `ConnectionHandler` in the `Later` instance to ensure we can compare it in the above branch.
// This is quite subtle but will hopefully become simpler soon once `KeepAlive::Until` is fully deprecated. See <https://github.com/libp2p/rust-libp2p/issues/3844>/
*shutdown =
Shutdown::Later(Delay::new(effective_keep_alive), earliest_shutdown)
}
}
(_, KeepAlive::No) if idle_timeout == &Duration::ZERO => {
*shutdown = Shutdown::Asap;
}
(Shutdown::Later(_, _), KeepAlive::No) => {
// Do nothing, i.e. let the shutdown timer continue to tick.
}
(_, KeepAlive::No) => {
let deadline = Instant::now() + *idle_timeout;
*shutdown = Shutdown::Later(Delay::new(*idle_timeout), deadline);
}
(_, KeepAlive::Yes) => *shutdown = Shutdown::None,
};

To count the number of active streams on a connection, we embed an Arc<()> in each stream. This is possible in a backwards-compatible way because Stream is instantiated within this module.

  • At the start of the connection, we construct an Arc<()> as a stream counter and store it in Connection.
  • Every time we make a new stream, be in inbound or outbound, we clone this Arc and store it in the Stream
  • At any point, we can check how many streams there are by calling Arc::strong_count.

If this count reaches 1, we know that there are no active streams on this connection. This should be a pre-condition to asking the ConnectionHandler via ConnectionHandler::connection_keep_alive whether or not the connection is still needed.

Overall, this will allow protocols to not having to do any tracking on their own in regards to whether to keep the connection alive because there are still streams running on it.

As a second step, we need to introduce an "opt-out" functionality for this behaviour:

impl Stream {
	pub fn no_keep_alive(&mut self);
}

Internally, this needs to downgrade the Arc<()> to a Weak<()> which automatically reduces the strong_count and thus "ignores" this stream. The ping protocol needs to call this function on every Stream received to not keep the connection alive if the only remaining streams are ping-streams.

Within the same PR, we should remove any book-keeping from protocols around active streams to make sure that this mechanism actually works.

@thomaseizinger
Copy link
Contributor Author

@leonzchang I see you opened a draft PR for this, exciting! :)
Mind commenting on her so I can assign you to this issue?

@leonzchang
Copy link
Contributor

Sure please☺️

@mergify mergify bot closed this as completed in #4595 Oct 25, 2023
mergify bot pushed a commit that referenced this issue Oct 25, 2023
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.

2 participants