swarm: Remove ConnectionHandler::Error

[thomaseizinger]

Based on #3353 (comment), I am opening this issue to track the work around removing ConnectionHandlerEvent::Close. Closing a connection is the only usage of ConnectionHandler::Error but we've agree that ConnectionHandlers themselves should not be allowed to close connections because they don't fully own them and might disrupt other protocols.

Tasks

Beta Give feedback

1. fix(gossipsub): gracefully disable handler on stream errors #3625
   send-it +1
2. feat(request-response): don't close connection on stream errors #3913
   send-it +1
3. feat(ping): don't close connections upon failures #3947
   send-it +1
4. feat(swarm): deprecate ConnectionHandlerEvent::Close #4714
   internal-change +1
5. feat(swarm): don't close connection in OneShotHandler #4715
   internal-change send-it +2
6. feat(relay): don't close connections upon errors in relay server #4718
   internal-change send-it +2
7. feat(relay): propagate errors to Transport::{listen_on,dial} #4745
   send-it +1
8. refactor(dcutr): simplify public API #4749
   send-it +1
9. feat(swarm): don't have ConnectionHandlers close connections #4755
   internal-change send-it +2

[mxinden]

Audit the codebase for usages of Close and port all protocols to set KeepAlive::No instead of closing the connection directly.

Based on intuition, with no particular instance in mind, I am not sure this is possible, i.e. I expect us to find a use-case for ConnectionHandlerEvent::Close. That said, I would like to be proven wrong and thus I am in favor of proceeding on this GitHub issue.

[thomaseizinger]

Audit the codebase for usages of Close and port all protocols to set KeepAlive::No instead of closing the connection directly.

Based on intuition, with no particular instance in mind, I am not sure this is possible, i.e. I expect us to find a use-case for ConnectionHandlerEvent::Close. That said, I would like to be proven wrong and thus I am in favor of proceeding on this GitHub issue.

Currently, we tend to close connection upon certain errors. That is wrong in my opinion. Just because one or more streams on one handler fail doesn't mean the connection is faulty or that the other peer is necessarily misbehaving. Do we agree that in those instances, setting KeepAlive::No is more appropriate than closing the connection?

[mxinden]

Do we agree that in those instances, setting KeepAlive::No is more appropriate than closing the connection?

Agreed.

[vnermolaev]

I think I can pick up this issue as "a spiritual follow-up" of #3577.

I scouted the code, and it seems that ConnectionHandlerEvent::Close is only used in two cases

• as a result of polling behaviour, i.e.,return Poll::Ready(ConnectionHandlerEvent::Close(err));
  The inner error can contain different values, e.g.,

  return Poll::Ready(ConnectionHandlerEvent::Close(
      HandlerError::MaxInboundSubstreams,
  ));

  return Poll::Ready(ConnectionHandlerEvent::Close(
      ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(Either::Left(e))),
  ))

  This information will be lost (at best printed out). Is this a desired behaviour? In such cases, I can set self.keep_alive = KeepAlive::No. Should Poll::Pending be returned (it seems the only choice, though)?

• in rewrapping actions, e.g., ConnectionHandlerEvent::Close(val) => ConnectionHandlerEvent::Close(map(val)),. This can be deprecated for consequent removal.

[thomaseizinger]

Sure!

Let me give you some more context so you can work through this properly. It is a slightly more advanced issue!

First, I'd recommend you to read through #3353 (comment).

Then have a look at this section in case you haven't: https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md#hierarchical-state-machines

Each NetworkBehaviour is given one ConnectionHandler per physical connection. All ConnectionHandlers of a single connection are composed together. What is issue is essentially about is the fact that one of these ConnectionHandlers can close the underlying physical connection which obviously affect all other protocols that run in parallel.

Currently, we often do that upon what is termed "fatal" errors. However, those errors are usually only fatal for this one ConnectionHandler and not for all handlers that are running on the same connection. As you said correctly, if we stop returning the errors, this information is "lost". I think a log message on info level is good enough here. info is typically used for state changes and the state change we are making here is that we "disable" the handler by setting KeepAlive::No.

Poll::Pending should only be returned if there is no work to do within the handler, i.e. all queues are empty etc. Most likely, the pattern you will see is either that we check for an Option<Error> to be Some, in which case the statement can be removed entirely, or that a VecDeque of events is collected and we push a ConnectionHandlerEvent::Close into it in which case we only need to remove the code that pushes the Close event into it.

Does this make sense?

In regards to implementing this, I'd prefer if we can have very small PRs, ideally one per protocol / crate.

[vnermolaev]

Then have a look at this section in case you haven't: https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md#hierarchical-state-machines

OMG, where has this gem been all my Rust life? I've googled a lot on SMs, but this was never among the results.

In regards to implementing this, I'd prefer if we can have very small PRs, ideally one per protocol / crate.

Agreed, let me draft a small PR for some protocol, work out all the quirks and then attempt to apply the same procedure to the rest.

[thomaseizinger]

Then have a look at this section in case you haven't: master/docs/coding-guidelines.md#hierarchical-state-machines

OMG, where has this gem been all my Rust life? I've googled a lot on SMs, but this was never among the results.

Our connection state machine is one of my favourites:

rust-libp2p/swarm/src/connection.rs

Line 95 in 1b09b8c

pub struct Connection<THandler>

In regards to implementing this, I'd prefer if we can have very small PRs, ideally one per protocol / crate.

Agreed, let me draft a small PR for some protocol, work out all the quirks and then attempt to apply the same procedure to the rest.

Looking forward to it! Thank you for your help :)

[vnermolaev]

Look, there is an approach i found in gossipsub

rust-libp2p/protocols/gossipsub/src/handler.rs

Lines 303 to 314 in 1b09b8c

	NegotiationError::Failed => {
	// The protocol is not supported
	self.protocol_unsupported = true;
	if !self.peer_kind_sent {
	self.peer_kind_sent = true;
	// clear all substreams so the keep alive returns false
	self.inbound_substream = None;
	self.outbound_substream = None;
	self.keep_alive = KeepAlive::No;
	return Poll::Ready(ConnectionHandlerEvent::Custom(
	HandlerEvent::PeerKind(PeerKind::NotSupported),
	));

compared to the following piece

rust-libp2p/protocols/gossipsub/src/handler.rs

Lines 327 to 329 in 1b09b8c

	if let Some(error) = reported_error {
	return Poll::Ready(ConnectionHandlerEvent::Close(error));
	}

In the former case, this particular handler will be closed, while the error is still reported to the parent.
In the latter case, the full connection will be closed (and such behaviour is undesiarable).
It is worth noting that both inner types of ::Custom and ::Close are the same HandlerEvent.

Would the former approach be acceptable for the latter case? It seems to reach the set goals: the handler will be closed, but also the error will be propagated to the parent, where it can be appropriately reported.

[thomaseizinger]

Would the former approach be acceptable for the latter case? It seems to reach the set goals: the handler will be closed, but also the error will be propagated to the parent, where it can be appropriately reported.

I don't think it is worth reporting these errors to the parent. They might be transient, i.e. only a single stream might have failed but otherwise the connection is intact. This actually makes me think, it could be that we have other streams that are still operating (i.e. inbound_stream or outbound_stream might still be Some). Thus I am inclined to say:

• Upon errors on a single stream, all we really should be doing is log the error
• KeepAlive should only depend on whether we are currently doing work

I think the above applies to all protocols. This is a slight deviation from what I previously thought we should do! Setting KeepAlive::No on any error seems to be too aggressive. As @mxinden noted in #3482 (comment), a remote might just drop one particular stream for whatever reason. We should be resilient to that!

The combination of the above means that in case all our streams fail, we will eventually close the connection because they are set to None. Also note that gossipsub in particular has a limit of creating a maximum of 5 streams. Once we hit that limit, we should set KeepAlive::No instead of closing the connection. That applies to these lines here:

rust-libp2p/protocols/gossipsub/src/handler.rs

Lines 341 to 346 in 1b09b8c

	if self.inbound_substreams_created > MAX_SUBSTREAM_CREATION {
	// Too many inbound substreams have been created, end the connection.
	return Poll::Ready(ConnectionHandlerEvent::Close(
	HandlerError::MaxInboundSubstreams,
	));
	}

[AgeManning]

I just read through this. I'm one of the people that have been closing connections on errors (in gossipsub) :p.

I'm interested in the negotiation errors. And have a few questions about some logic to handle.

I see these often. I think a good portion of the time, is when trying to connect to a go-libp2p client and for one reason or another (usually they have too many peers) they drop the initial connection. From our side, it can look like a negotiation error. In the earlier days, I think they were not responding, but not dropping the connection. It was up to us to drop the connection when negotiation failed. In the new version, we may keep these stale connections alive until the KeepAlive timeout ends?

There were also attacks where an attacker would consume all of the local file descriptors by constantly trying to open many many new connections and keeping them alive. We mitigated this somewhat by capping the number of inbound connections, but also dropping the connections pretty fast when negotiations or connections failed. Could we be at risk if we now leave these connections alive for longer?

When a node goes offline, we can identify it via Pings or various other negotiation timeouts. In the past I've found it useful to drop connections faster so that we can run discovery and search for new peers, rather than keeping stale old connections alive.

I think all of these can be mitigated I guess by the keep alive timeout? Curious about your thoughts on these.

[thomaseizinger]

In the new version, we may keep these stale connections alive until the KeepAlive timeout ends?

Unless a ConnectionHandler sets KeepAlive::Until, terminating the connection is instant with KeepAlive::No. My approach would be that any protocol needs to disable its handler instantly if it receives a negotiation error. If we go forward with an approach such as #3651 for #2680, then a ConnectionHandler could even eagerly disable itself once we receive the "supported protocols" (via identify f.e.) and the current one is not present. A quick sketch could be (Disclaimer: I only came up with this idea yesterday, there might still be flaws):

• Connection is established
• Some protocols, like identify, run upon connection establishment
• They receive a response and emit ConnectionHandlerEvent::ReportRemoteProtocols
• The list of protocols is immediately handed back to the ConnectionHandler via ConnectionEvent::RemoteProtocolsChange
• Each ConnectionHandler could instantly set themselves to KeepAlive::No if their own protocol is not present in the list

Curious about your thoughts on these.

The longer discussion around this is in #3353 (comment). I think eagerly closing connections on unsupported protocols is a reasonable policy. I also think that policy does not belong into rust-libp2p. It is something each application has to decide for themselves. My recommendation would be that protocols should emit an event if they disable themselves (perhaps we can even provide something standard for this?). This event can be captured by the behaviour and emitted to the Swarm. At the application-level, you can then close connections to peers that e.g. don't support gossipsub in case your application relies on that.

I don't want to derail this too much but the entire KeepAlive mechanism may also need some more thought. For example, should a protocol like identify keep the connection alive? It is more of an "auxiliary" protocol in my eyes but if no other protocols need the connection, it could be closed.

[AgeManning]

Yeah I agree.

We probably need a standardized way of letting the users decide which protocols should allow the connection to be kept alive. Maybe in the derive macro when composing behaviours we could tell the macro which sub-behaviours we want to keep the connection alive.

[rkuhn]

I’d think that it depends on the protocol if KeepAlive::No makes sense. If the user (of the derived behaviour) wants to keep connections around then that should probably be done by offering a NetworkBehaviour that does just that — previously this was allowed by the ping protocol, but the functionality has — correctly, I think — been decoupled from ping.

While I do agree with @mxinden’s proposal above, I’m not sure whether it addresses the issue @AgeManning raised: if we need to interact with a broken peer (like go-ipfs sounds to be) who “signals” the desire that we should close the connection by reporting a nondescript stream negotiation failure, then we might need to keep the ability for a stream protocol handler to close the connection. Alternatively, go-ipfs could be fixed …

---

Regarding the overall topic: it might well be reasonable to run a protocol on a stream that performs admin coordination. Such a handler may then want to shut down the connection when agreed with the peer, independent of other stream handlers setting KeepAlive::Yes. If the connection handler event goes away, then this will need to be done by emitting an event into the corresponding admin NetworkBehaviour and closing the connection from there, right?

[thomaseizinger]

We probably need a standardized way of letting the users decide which protocols should allow the connection to be kept alive. Maybe in the derive macro when composing behaviours we could tell the macro which sub-behaviours we want to keep the connection alive.

Configuration is one option (pun intended) but adding configuration options is a fight we are not going to win as more usecases come up. I am a fan of thinking in policies and mechanism. rust-libp2p should provide a mechanism for closing connections and users should bring the policy on when to use that.

If the connection handler event goes away, then this will need to be done by emitting an event into the corresponding admin NetworkBehaviour and closing the connection from there, right?

Yes, that is the idea.

---

I am starting to wonder whether the entire connection_keep_alive functionality is a good idea. Wouldn't everyone be served better if we don't close connections automatically?

1. ConnectionHandlers wouldn't need to use KeepAlive::Until as the default value to avoid connections being shut down instantly after they are established
2. Our examples and tests wouldn't need to add keep_alive::Behaviour to work

Even determining whether a remote supports any of our protocols is difficult. We can only learn that through identify and if that works, a naive intersection of the protocols will at least yield that one.

[AgeManning]

From a selfish perspective (gossipsub), it would seem to make sense that each behaviour has the ability to "disable" itself independently of other behaviours (such that for all intents and purposes a "DisableBehaviour" event looks the same as a "CloseConnection" at the behaviour level). I guess this is what the KeepAlive is attempting to do.

If a peer does something the behaviour doesn't like, it has the ability to drop all substreams and prevent new ones from being created. Other behaviours work independently until all become disabled and the connection gets closed altogether. Its been a while since I looked at the KeepAlive, but from memory it does this, but without the ability of disabling re-established substreams?

[thomaseizinger]

We are going to land #3605 soon which simplifies error handling already a fair bit.

Plus, @mxinden had a good idea regarding keep-alive: Deprecate the Until mechanism. I've captured this here: #3844

[thomaseizinger]

@mxinden I added this to the milestone but I'd also like to make another patch-release of libp2p-swarm where we deprecate ConnectionHandlerEvent::Close to give users a heads-up that we are going to remove this.