Skip to content
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

fix(ping): honor ping interval in case of errors #4423

Merged
merged 11 commits into from
Sep 9, 2023

Conversation

dariusc93
Copy link
Contributor

@dariusc93 dariusc93 commented Sep 1, 2023

Description

This PR adds a delay to ping connection handler after exceeding the failure rate to prevent opening a outbound stream right after an error.

Resolves #4410.

Notes & open questions

  1. Would it be better to switch failures to a boolean instead?

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

protocols/ping/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/ping/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

  1. Would it be better to switch failures to a boolean instead?

I don't think we can make it a boolean. The behaviour we need to retain is that the first error is "free", i.e. doesn't cause any problems. We want to be compatible with an implementation that uses one stream per ping. (Do we actually have a test for that?)

I am open to changing the impl here but I'd prefer if that happens in a different PR and we focus this one on the fix for the stream spam.

protocols/ping/CHANGELOG.md Show resolved Hide resolved
protocols/ping/CHANGELOG.md Outdated Show resolved Hide resolved
dariusc93 and others added 2 commits September 3, 2023 00:02
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@dariusc93 dariusc93 marked this pull request as ready for review September 4, 2023 00:18
protocols/ping/src/handler.rs Outdated Show resolved Hide resolved
protocols/ping/src/handler.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title fix(ping): Add delay after exceeding failure rate fix(ping): honor ping interval in case of errors. Sep 4, 2023
@thomaseizinger thomaseizinger changed the title fix(ping): honor ping interval in case of errors. fix(ping): honor ping interval in case of errors Sep 6, 2023
@thomaseizinger thomaseizinger dismissed their stale review September 6, 2023 21:27

Made them myself.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but deferring to @thomaseizinger. Thank you for the work here.

protocols/ping/src/handler.rs Outdated Show resolved Hide resolved
@mergify mergify bot merged commit cd32435 into libp2p:master Sep 9, 2023
67 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ping: Repeated errors emitted from ping behaviour
3 participants