On closing of connections

[thomaseizinger]

Context: This started in #3201.

Currently, connections can be closed in a variety of ways:

• Users of Swarm can close a connection
• A NetworkBehaviour can close a connection
• A ConnectionHandler can close a connection

The first one is definitely legit. A user should be able to close one or all connections to a particular peer.
The second is legit too. Esp. with #2824 coming, a NetworkBehaviour needs to be able to close a connection to a peer.

What I'd like to discuss are two things:

1. Should a single ConnectionHandler be able to close an entire connection? On closing of connections #3353 (comment)
2. Which errors should we report to a ConnectionHandler? On closing of connections #3353 (comment)

[thomaseizinger]

Should a single ConnectionHandler be able to close an entire connection?

I'd like to put the idea forward of removing this feature. ConnectionHandlers can already "disable" themselves by returning KeepAlive::No. If all handlers return No, then the connection will be closed. I don't see why a single ConnectionHandler should close an entire connection.

There is an argument that a protocol implemented within one ConnectionHandler encounters a peer with bad behaviour and thus the entire connection to this peer should be closed. The question is, should it really? Or should we just refuse to speak that one protocol. Even if we wanted to close the connection, users could still implement that by sending an event to the NetworkBehaviour which can then close the connection.

[mxinden]

Allowing users to bubble up errors via ? is neat. That said, what should the upper layer do with the error? Provide it to the NetworkBehaviour? Pass it to the user via Swarm? Close the connection?

[thomaseizinger]

The latter is more complex. Thus I am tending towards keeping it as is, unless removing the feature in ConnectionHandler significantly simplifies related design.

It simplifies the design a fair bit. You'd pretty much be looking at #3201 without the whole CloseReason concept. A connection closed by a NetworkBehaviour currently does not yield a reason / error to the user. Should it?

I am leaning towards removing Close for this reason. A NetworkBehaviour also doesn't have a complete view of the application (it only sees the same vertical slice as its associated handlers) so one could argue that really, only the user should be able to close connections via the Swarm.

But, a NetworkBehaviour has the ability to create connections so closing them seems to also be in scope.

[dariusc93]

Though i am a bit late to responding, I do like the idea of removing the feature (if it not already) since i was going to question where it would make sense to have a handler close a whole connection if we have KeepAlive, which indicate if the connection can be kept alive or not, or if we would be able to allow it to close a connection, would it be possible to track other handlers to allow one to determine if it can close a connection to prevent interrupting other states in different behaviours.

[thomaseizinger]

The consensus is to remove it, see #3591 :)

We've done it for gossipsub already but several other protocols (like relay) still actively close connections.

[dariusc93]

Thanks! Hopefully that will change in time since one faulty handler shouldnt mean that the whole connection would be faulty and thus warrant to be disconnected.

[thomaseizinger]

Which errors should we report to a ConnectionHandler?

We currently report all errors that happen during a stream upgrade to the ConnectionHandler:

rust-libp2p/swarm/src/connection.rs

Lines 265 to 268 in d82c2a1

	Poll::Ready(Some((info, Err(error)))) => {
	handler.on_connection_event(ConnectionEvent::ListenUpgradeError(
	ListenUpgradeError { info, error },
	));

In other words, we pretend as if those errors were ephemeral. In practice however, this error contains ProtocolError from multistream-select:

rust-libp2p/misc/multistream-select/src/protocol.rs

Lines 399 to 411 in d82c2a1

	pub enum ProtocolError {
	/// I/O error.
	IoError(io::Error),
	/// Received an invalid message from the remote.
	InvalidMessage,
	/// A protocol (name) is invalid.
	InvalidProtocol,
	/// Too many protocols have been returned by the remote.
	TooManyProtocols,
	}

Any of these are either permanent (io::Error) or are a protocol violation. In the former case, the connection is already broken so trying to close it will likely not work. In the latter case, I think we should close the connection for the user and report that.

I've already outlined this idea in #3307 (comment) but I wanted to discuss this in a more visible place.

[thomaseizinger]

I wonder if we should try to do this as a non-breaking change? It is useless for the io::Error variant and I'd be very surprised if any user somehow depends on any of the other errors, thus I'd say it is safe to just remove as a breaking change?

[thomaseizinger]

It is also reasonable easy to migrate because users just need to delete code, i.e. remove match-arms. Basically, the breaking change would be to remove the Upgrade variant from ConnectionHandlerUpgrErr and replace it with Failed.

[thomaseizinger]

It is also reasonable easy to migrate because users just need to delete code, i.e. remove match-arms. Basically, the breaking change would be to remove the Upgrade variant from ConnectionHandlerUpgrErr and replace it with Failed.

Actually, until #2863 is implemented, there would also still be a variant Apply or whatever we want to call it.

[thomaseizinger]

In #3482, it came up that closing the connection is actually not a good idea because those errors might be local to a stream, i.e. one stream could get dropped during protocol negotiation. Closing the entire connection is overkill here and not the right strategy.

I wonder if we can still simplify error handling here.

From the ConnectionHandlers perspective, a new stream was requested. But these streams don't have an identity, instead they are interchangeable.

Would it make sense to retry opening a new stream for the same request upon these errors? The question would be, when do we stop and do we eventually report an error to the user?

[mxinden]

Would it make sense to retry opening a new stream for the same request upon these errors?

What if the remote dropped the stream because it wants to deny that exact protocol. I don't think retrying should be implicit, i.e. done by libp2p-swarm.

[mxinden]

Meta level: Thanks for the great write-up with lots of context @thomaseizinger. Allows everyone to join in easily.

[pieterdd]

I'm not sure if this is the correct place to bring it up, but there may be another angle to this.

I'm using request_response::Behaviour to implement my application's network logic. My working assumption is that this automatically rules out most options since I don't have my own NetworkBehaviour or ConnectionHandler implementation. I do have a Codec implementation.

One solution I came across is the usage of io.close() in Codec::write_request and/or Codec::write_response. When trying this out though, I noticed that a SwarmEvent::ConnectionClosed event doesn't trigger right away.

Alternatively, I suppose I could hook in to libp2p::request_response::Event::Message to call Swarm::disconnect_peer_id when receiving a final response. The documentation seems to recommend instead using ConnectionHandler, which I'm not sure is possible in my case.

What are the recommendations for request_response?

[thomaseizinger]

The connection should be closed automatically once it is idle in your case.

Closing the stream doesn't immediately close the connection.

Is there a reason you'd want to actively close the connection?

[pieterdd]

My use case is a CLI program that waits for a peer to advertise its presence, connects to it, synchronizes state both ways and exits when the exchange is finished. Waiting until the connection notices the other end hung up would cause an exit delay for the user.

Since Rust's trait bounds aren't satisfied when closing the IO stream in Codec::read_response upon reading the final message, I can't fully mirror the closing behavior I added to Codec::write_response. However, a Swarm::disconnect_peer_id call in the message event of the final response does eliminate the exit delay for me. It triggers a ConnectionClosed event, after which the program exits as expected.

Does this approach have any caveats I should be mindful of?

[thomaseizinger]

It seems unnecessary to manually disconnect the peer and then exit the application, you could just exit upon receiving https://docs.rs/libp2p/0.51.3/libp2p/request_response/enum.Event.html#variant.ResponseSent. There isn't really any benefit in closing the connection because we currently don't have graceful shutdown of connections.

[mxinden]

There isn't really any benefit in closing the connection because we currently don't have graceful shutdown of connections.

I am confused. Don't we @thomaseizinger?

rust-libp2p/swarm/src/connection.rs

Lines 193 to 197 in 53e5370

	/// Begins an orderly shutdown of the connection, returning the connection
	/// handler and a `Future` that resolves when connection shutdown is complete.
	pub(crate) fn close(self) -> (THandler, impl Future<Output = io::Result<()>>) {
	(self.handler, self.muxing.close())
	}

[thomaseizinger]

You are right, we do actually close them gracefully but we don't wait for it if you are already shutting down the Swarm anyway.

[thomaseizinger]

An idea regarding idle connections:

With #3844, we will remove KeepAlive::Until in favor ConnectionHandlers having to implement such a mechanism themselves if they want it.

Whilst implementing this, it occurred to me that an idle connection timeout is a useful feature that we could provide as a general setting on the Swarm with a default of 0 seconds.

Moreover, we could automatically determine that a ConnectionHandler is idle if:

• There are no active streams (incl. no in-progress upgrades)
• The ConnectionHandler emitted Poll::Pending

This would completely remove the need for:

• ConnectionHandler::connection_keep_alive
• keep_alive::Behaviour

Instead, users would configure an idle timeout of X seconds that kicks in once the connection is idle.

Never closing idle connections could be achieved by setting an absurdly high value for the idle timeout.

I think this would be a great simplification. Opinions welcome.

[thomaseizinger]

There are no active streams

How would the connection task know that there are no active streams? Once it hands a stream to a ConnectionHandler the connection task has no way to track the stream's lifecycle today, right?

Not today but we can track it using an Arc and the strong_count function.

[mxinden]

That would work. Though I guess, with the above mentioned ping example, the idea of keeping a connection alive based on active streams in obsolete.

[thomaseizinger]

Just brainstorming here: We could still say that this is the default but add a function on Stream that opts out of that?

// swarm/src/stream.rs
impl Stream {
    /// Opt-out this stream from [`Swarm`]s keep-alive tracking.
    ///
    /// By default, streams prevents a connection from becoming idle.
    /// This function allows you to opt-out this particular stream from this behaviour.
    /// 
    /// By setting `no_keep_alive`, a connection will be treated as idle as soon as all other streams are de-allocated.
    pub fn no_keep_alive(&mut self) { }
}

Naming subject to being improved but you get the idea :)

I think this would work quite well. Most protocols want to keep connections alive whilst they have streams open on it and protocols that are okay with their streams being aborted can set this flag on them. We could also do that with a type-parameter on Stream such that it is obvious that you turned a stream from "strong" to "weak".

[mxinden]

Just checking my understanding, we would still need some mechanism for e.g. libp2p-relay to be able to keep a connection alive even though it does not have any streams open.

[thomaseizinger]

Just checking my understanding, we would still need some mechanism for e.g. libp2p-relay to be able to keep a connection alive even though it does not have any streams open.

Yes, there would still be connection_keep_alive but it would be optional, default to No and almost no protocol would have to override it.

In other words, the standard case would be easy and protocols like relay can override it with Yes. Protocols like ping could opt on a per stream basis.

[thomaseizinger]

Whilst implementing this, it occurred to me that an idle connection timeout is a useful feature that we could provide as a general setting on the Swarm with a default of 0 seconds.

What do you think of this @mxinden?

[mxinden]

This makes sense to me 👍

[thomaseizinger]

This makes sense to me 👍

And deprecate keep_alive::Behaviour as a result?

[mxinden]

Yes, that makes sense. Of course as long as one can set the idle connection timeout VERY high.

[thomaseizinger]

Yes, that makes sense. Of course as long as one can set the idle connection timeout VERY high.

Yes, I was thinking of making it a u64 representing seconds. 584942 million years (u64::MAX) seems sufficient.

[thomaseizinger]

I am closing this now that we have issues for all discussed points here!