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.

- [x] https://github.com/libp2p/rust-libp2p/pull/3625
- [x] https://github.com/libp2p/rust-libp2p/pull/3913
- [x] https://github.com/libp2p/rust-libp2p/pull/3947
- [ ] https://github.com/libp2p/rust-libp2p/pull/4714
- [ ] https://github.com/libp2p/rust-libp2p/pull/4715
- [ ] https://github.com/libp2p/rust-libp2p/pull/4718
- [ ] https://github.com/libp2p/rust-libp2p/pull/4745
- [ ] https://github.com/libp2p/rust-libp2p/pull/4749
- [ ] https://github.com/libp2p/rust-libp2p/pull/4755

[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,
	));
	}