-
Notifications
You must be signed in to change notification settings - Fork 996
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
refactor(swarm)!: refactor ListenUpgradeError and DialUpgradeError #3307
Closed
jxs
wants to merge
18
commits into
libp2p:3264-remove-inject-calls
from
jxs:split-listen-upgrade-error
Closed
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
e63d5c7
swarm: remove unconstructed Timer variant
jxs fbc6b82
swarm: make ListenUpgradeError::error an UpgradeError
jxs 11db523
swarm: make DialUpgradeError::error an UpgradeError
jxs a793fb0
dcutr: update to libp2p-swarm ListenUpgradeError
jxs 05c5fd0
identify: update to libp2p-swarm ListenUpgradeError
jxs 6758ccd
relay: update to libp2p-swarm ListenUpgradeError
jxs 24d4c9d
metrics: update to libp2p-swarm ListenUpgradeError
jxs fb51e22
kad: update to libp2p-swarm ListenUpgradeError
jxs a03e26c
rendezvous: update to libp2p-swarm ListenUpgradeError
jxs a403407
ping: update to libp2p-swarm ListenUpgradeError
jxs bc2e6bb
gossipsub: update to libp2p-swarm ListenUpgradeError
jxs 4a0e5ee
request-response: update to libp2p-swarm ListenUpgradeError
jxs 021502c
examples: update file-sharing example to libp2p-swarm
jxs ec1cb53
dcutr: report Timeout as a variant of Error
jxs 1bc39e2
identify: introduce Error enum
jxs d70280a
metrics: fix removal of identify Event::Timeout
jxs 3a0db97
relay: introduce behaviour::OutboundError enum
jxs c2eda37
relay: remove unneeded inbound_hop::UpgradeError.
jxs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Previously, we emitted an error (and thus closed the connection) when we ran into a timeout. Why are we changing this behaviour now?
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.
Isn't the behaviour the same? In the sense that the connection was closed because of a timeout and we are bubbling it up? But after reading and replying on #3307 (comment) I now see that Timeout makes sense being a variant on the
Error
itself ofDirectConnectionUpgradeFailed
rather than a variant of Event.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.
Unless I am mistaken, the timeout refers to a stream upgrade. The connection can/should still be intact at this stage. Something like
PendingUpgrade
should trigger this timeout on the remote but the connection underneath works perfectly fine.It does raise the question whether we should close an entire connection at all just because one protocol failed to upgrade a stream. Just disabling that protocol (and handler) and letting the collaborative keep-alive eventually close the connection seems like a better idea at first sight.
We may want to do this in a separate PR though, either before or after we merge this.