-
Notifications
You must be signed in to change notification settings - Fork 941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: don't report inbound stream upgrade errors to handler #3605
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 I am very much looking forward to this pull request landing! This has been a thorn in my eye for long.
@thomaseizinger let me know if you want another review here. |
Go for it. It is only a draft because it is a breaking change. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…-libp2p into feat/no-report-inbound-error
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to merge from my end. Great work!
})); | ||
} | ||
} | ||
self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are on the same page when I say that libp2p-relay
should not be closing the connection based on pending_error
via ConnectionHandlerEvent::Close
further below in poll
. That said, let's not fix it here but along with #3591.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be a much easier change once this lands too!
Description
When an inbound stream upgrade fails, there isn't a whole lot we can do about that in the handler. In fact, for several errors, we wouldn't even know which specific handler to target, for example,
NegotiationFailed
. Similiarly, in case of an IO error during the upgrade, we don't know which handler the stream was eventually meant to be for.Notes & open questions
This is a breaking change, thus draft for now.Change checklist