fix(swarm): gracefully disable oneshot handler on dial upgrade errors #3577

vnermolaev · 2023-03-09T13:07:14Z

Description

Resolves #3269.

Notes

This PR implement changes discussed in #3269

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.

Links to any relevant issues

#3269

Change checklist

The changes are trivial. Not sure whether the usual checklist applies.

thomaseizinger

Thanks!

thomaseizinger · 2023-03-10T05:17:25Z

swarm/src/handler/one_shot.rs

@@ -213,7 +213,9 @@ where
            }
            ConnectionEvent::DialUpgradeError(DialUpgradeError { error, .. }) => {
                if self.pending_error.is_none() {
+                    log::debug!("DialUpgradeError: {error:?}");
                    self.pending_error = Some(error);


I think we should remove this, otherwise we will still close the connection later!

Pardon me, remove self.pending_error = Some(error)?
I am not sure what implications are, so I kept the changes aligned with the approach outlined in the respective issue.

Anything within pending_error will be returned via ProtocolsHandlerEvent::Close which effectively closes the connection. In this case, we don't want that but set keep_alive to No which, if not other handler needs the connection will result in closing it.

thomaseizinger · 2023-03-10T05:20:02Z

swarm/src/handler/one_shot.rs

@@ -213,7 +213,9 @@ where
            }
            ConnectionEvent::DialUpgradeError(DialUpgradeError { error, .. }) => {
                if self.pending_error.is_none() {
+                    log::debug!("DialUpgradeError: {error:?}");


Suggested change

log::debug!("DialUpgradeError: {error:?}");

log::debug!("DialUpgradeError: {error}");

Errors can be display formatted but at the moment, that does not necessarily print the entire chain of errors.

@mxinden Should we depend on anyhow internally here to print all errors?

There is a function print_error_chain in swarm/src/lib.rs. I could use that one. Is this an option?

Happy to use that one yeah as we are within the same crate!

swarm/src/handler/one_shot.rs

thomaseizinger · 2023-03-10T14:01:40Z

swarm/src/handler.rs

@@ -467,7 +467,7 @@ where
            ConnectionHandlerUpgrErr::Timer => {
                write!(f, "Timer error while opening a substream")
            }
-            ConnectionHandlerUpgrErr::Upgrade(err) => write!(f, "{err}"),
+            err @ ConnectionHandlerUpgrErr::Upgrade(_) => crate::print_error_chain(f, err),


I don't think this is what we want, instead, this should probably just print "upgrade error" without printing the inner error. That is what print_error_chain then does because the inner error is returned from source.

If you use print_error_chain here, we might end up with duplicate printing if somebody uses print_error_chain on ConnectionHandlerUpgrErr.

You're right.
Please see my other comment.
It'd be possible to mimic impl fmt::Display for DialError.

vnermolaev · 2023-03-13T17:54:48Z

One test is failing but this is due some docker issues.

thomaseizinger

Thanks!

thomaseizinger · 2023-03-13T19:18:14Z

One test is failing but this is due some docker issues.

Yes that is unfortunately a flaky test.

Can you add a small changelog entry to libp2p-swarm please?

vnermolaev · 2023-03-13T19:36:22Z

Is it really necessary?
Last time I did a larger refactoring and no changelog has been required because it was not a user-facing feature.
This one does not seem to be user-facing. I can ofc add a change log, but what should I even write? :)

thomaseizinger · 2023-03-13T19:44:42Z

This one does not seem to be user-facing.

I'd argue that this one is definitely a change in behaviour that is observable.

I can ofc add a change log, but what should I even write? :)

Feel free to copy the PR title (modulo formatting adjustments)!

vnermolaev · 2023-03-13T19:48:24Z

done

mergify · 2023-03-13T19:53:25Z

This pull request has merge conflicts. Could you please resolve them @vnermolaev? 🙏

thomaseizinger · 2023-03-14T06:34:56Z

Thank you!

I've opened #3591 as a spiritual follow-up because we currently have this "problem" in many crates actually.

mxinden

Thanks for the work here. @thomaseizinger or @vnermolaev can either of you address the comments below?

I hope that this is not a breaking change. In other words, I hope that no downstream user depends on the fact that the OneShotHandler closes a connection if a remote does not support its protocol.

mxinden · 2023-03-14T13:30:32Z

swarm/CHANGELOG.md

@@ -90,6 +90,8 @@
 - Deprecate methods `Swarm::with_executor`, `Swarm::with_*_executor`, `Swarm::without_executor`.
  Introduce similar methods in `SwarmBuilder`. See [PR 3588].

+- Gracefully disable oneshot handler on dial upgrade errors. See [PR 3577].


Under the assumption that this is not a breaking change, this pull request should have bumped libp2p-swarm to v0.42.1 and this changelog entry should have been under the v0.42.1 section.

Not sure how to address this because it's been merged.

We can just fix it in a new PR.

mxinden · 2023-03-14T13:36:01Z

swarm/src/handler/one_shot.rs

@@ -213,7 +213,8 @@ where
            }
            ConnectionEvent::DialUpgradeError(DialUpgradeError { error, .. }) => {
                if self.pending_error.is_none() {
-                    self.pending_error = Some(error);


As far as I can tell, this was the only occurrence of setting self.pending_error. In other words, I don't think pending_error is used anywhere after this change. Is that correct?

Almost; there is one more place

if let Some(err) = self.pending_error.take() { return Poll::Ready(ConnectionHandlerEvent::Close(err)); }

The resolution of this usage is related to #3591.

But if it is never set, then this is dead code so we can delete it straight-away!

Sorry for missing that @mxinden.

thomaseizinger · 2023-03-14T16:38:04Z

I hope that this is not a breaking change. In other words, I hope that no downstream user depends on the fact that the OneShotHandler closes a connection if a remote does not support its protocol.

I'd be very surprised if it breaks something. I consider this a bugfix to be honest.

vnermolaev changed the title ~~fix(swarm) Log error. Set keep_alive to false.~~ fix(swarm): Log error. Set keep_alive to false. Mar 9, 2023

thomaseizinger changed the title ~~fix(swarm): Log error. Set keep_alive to false.~~ fix(swarm): gracefully disable oneshot handler on dial upgrade errors Mar 10, 2023

thomaseizinger reviewed Mar 10, 2023

View reviewed changes

vnermolaev force-pushed the bug/swallowe-error branch from 1ba4e1a to 1060619 Compare March 10, 2023 12:48

vnermolaev requested a review from thomaseizinger March 10, 2023 12:48

vnermolaev force-pushed the bug/swallowe-error branch from 1060619 to ad03390 Compare March 10, 2023 13:02

thomaseizinger requested changes Mar 10, 2023

View reviewed changes

vnermolaev force-pushed the bug/swallowe-error branch 3 times, most recently from 6ec12e4 to 8b4c290 Compare March 13, 2023 17:31

thomaseizinger approved these changes Mar 13, 2023

View reviewed changes

Victor Ermolaev added 3 commits March 13, 2023 20:55

Log error. Set keep_alive to false.

48bb92d

Print error chain for ConnectionHandlerUpgrErr

4c9fde3

Add a changelog entry.

2e468f2

vnermolaev force-pushed the bug/swallowe-error branch from 7f6fc65 to 2e468f2 Compare March 13, 2023 19:57

thomaseizinger approved these changes Mar 13, 2023

View reviewed changes

thomaseizinger added the send-it label Mar 13, 2023

mergify bot merged commit 2a18f7a into libp2p:master Mar 13, 2023

mxinden reviewed Mar 14, 2023

View reviewed changes

vnermolaev mentioned this pull request Mar 14, 2023

swarm: Remove ConnectionHandler::Error #3591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(swarm): gracefully disable oneshot handler on dial upgrade errors #3577

fix(swarm): gracefully disable oneshot handler on dial upgrade errors #3577

vnermolaev commented Mar 9, 2023 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

thomaseizinger Mar 10, 2023

vnermolaev Mar 10, 2023

thomaseizinger Mar 10, 2023

thomaseizinger Mar 10, 2023

vnermolaev Mar 10, 2023

thomaseizinger Mar 10, 2023

thomaseizinger Mar 10, 2023

thomaseizinger Mar 10, 2023

vnermolaev Mar 10, 2023

vnermolaev commented Mar 13, 2023

thomaseizinger left a comment

thomaseizinger commented Mar 13, 2023

vnermolaev commented Mar 13, 2023

thomaseizinger commented Mar 13, 2023

vnermolaev commented Mar 13, 2023

mergify bot commented Mar 13, 2023

thomaseizinger commented Mar 14, 2023

mxinden left a comment

mxinden Mar 14, 2023

vnermolaev Mar 14, 2023

thomaseizinger Mar 14, 2023

mxinden Mar 14, 2023

vnermolaev Mar 14, 2023

thomaseizinger Mar 14, 2023

thomaseizinger commented Mar 14, 2023

	log::debug!("DialUpgradeError: {error:?}");
	log::debug!("DialUpgradeError: {error}");

fix(swarm): gracefully disable oneshot handler on dial upgrade errors #3577

fix(swarm): gracefully disable oneshot handler on dial upgrade errors #3577

Conversation

vnermolaev commented Mar 9, 2023 • edited by thomaseizinger Loading

Description

Notes

Links to any relevant issues

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnermolaev commented Mar 13, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Mar 13, 2023

vnermolaev commented Mar 13, 2023

thomaseizinger commented Mar 13, 2023

vnermolaev commented Mar 13, 2023

mergify bot commented Mar 13, 2023

thomaseizinger commented Mar 14, 2023

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Mar 14, 2023

vnermolaev commented Mar 9, 2023 •

edited by thomaseizinger

Loading