-
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(ping): don't close connections upon failures #3947
Conversation
Mostly adds noise
This simplifies the API and reduces code. The hypothesis is that nobody is really interested in `Pong` events.
Do I understand correctly, that we expect all If yes, fine with me to proceed with this pull request. If not, I would expect all users to want some mechanism along the lines of what |
Define malfunctioning? I don't think
I tried to already make a point that equating a working ping with a working connection is a policy and I believe that users should be in charge of policy. Do you not agree with that? For example, another policy could be to disconnect all peers with a latency higher than 500ms or all that don't have a latency in the 95th percentile of all currently active connections.
If they can reliably detect a malfunctioning connection, then yes absolutely. |
I am happy to add some more docs to |
This comment was marked as resolved.
This comment was marked as resolved.
…ibp2p/rust-libp2p into feat/no-close-connection-ping-failures
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.
Fine with proceeding here. Thanks for the details. Just one thing on how the user should close a specific connection.
…ibp2p/rust-libp2p into feat/no-close-connection-ping-failures
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.
Good to merge from my end.
// Note: For backward-compatibility the first failure is always "free" | ||
// and silent. This allows peers who use a new substream |
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.
Is this backwards compatibility still relevant? This implementation should be compatible with any recent other implementation adhering to the specification, right?
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.
We could reword it but the functionality still needs to be there. JS for example uses a new stream per ping and we should not report that as an error.
Description
Previously, the
libp2p-ping
module came with a policy to close a connection after X failed pings. This is only one of many possible policies on how users would want to do connection management.We remove this policy without a replacement. If users wish to restore this functionality, they can easily implement such policy themselves: The default value of
max_failures
was 1. To restore the previous functionality users can simply close the connection upon the first received ping error.In this same patch, we also simplify the API of
ping::Event
by removing the layer ofping::Success
and instead reporting the RTT to the peer directly.Related: #3591.
Notes & open questions
Patch-by-patch review is recommended.
Change checklist