swallowed errors, or escalating too quickly

[rkuhn]

rust-libp2p/swarm/src/handler/one_shot.rs

Lines 214 to 219 in 13a59a3

	ConnectionEvent::DialUpgradeError(DialUpgradeError { error, .. }) => {
	if self.pending_error.is_none() {
	self.pending_error = Some(error);
	}
	}
	ConnectionEvent::AddressChange(_) | ConnectionEvent::ListenUpgradeError(_) => {}

@mxinden This is where incoming stream upgrade errors are lost, as mentioned in today’s meeting. #3128 somewhat intentionally did not fix this since the error is returned from Apply. Since there is no obvious need for more reaction, I’d propose to log inbound upgrade errors on debug level in the above spot.

In the above code snippet, outbound upgrade errors lead to closing the connection. I’m not sure whether that is the best possible response given that many protocols may be composed and thus the escalation’s scope seems too large.

[thomaseizinger]

Agree with all your points!

Related: #2894

[mxinden]

I’d propose to log inbound upgrade errors on debug level in the above spot.

Sounds good to me.

In the above code snippet, outbound upgrade errors lead to closing the connection. I’m not sure whether that is the best possible response given that many protocols may be composed and thus the escalation’s scope seems too large.

Agreed. Setting its keep_alive to False should be enough.

In my eyes #2894 would reduce some of the error handling complexity in most ConnectionHandler implementations.