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.

Sign up for GitHub

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

Jump to bottom

fix(ping): Fix panic in WASM caused by retrying on dial upgrade errors #5447

Merged
merged 11 commits into from
Jun 10, 2024
Merged

fix(ping): Fix panic in WASM caused by retrying on dial upgrade errors #5447

merged 11 commits into from
Jun 10, 2024

Conversation

oblique
Copy link
Contributor

@oblique oblique commented Jun 6, 2024 •
edited
Loading

Description

This PR workarounds async-rs/futures-timer#74 issue.

Fixes #5442

Notes & open questions

It is hard to create a reproduction as a unit test, however I created a reproduction repository. Please read README.md for more info.

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

@oblique oblique mentioned this pull request Jun 7, 2024
Closed
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 8, 2024

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

@oblique oblique requested a review from dariusc93 June 8, 2024 09:24
jxs
jxs reviewed Jun 8, 2024
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks for working on this Yianis, left a comment

protocols/ping/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/ping/src/handler.rs Outdated Show resolved Hide resolved
protocols/ping/src/handler.rs Show resolved Hide resolved
oblique and others added 2 commits June 8, 2024 21:43
@jxs jxs changed the title fix(ping): Fixes panic in WASM caused by retrying on dail errors fix(ping): Fix panic in WASM caused by retrying on dail errors Jun 8, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 9, 2024

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

@jxs jxs changed the title fix(ping): Fix panic in WASM caused by retrying on dail errors fix(ping): Fix panic in WASM caused by retrying on dial upgrade errors Jun 10, 2024
jxs
jxs approved these changes Jun 10, 2024
View reviewed changes
protocols/ping/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/ping/src/handler.rs Outdated Show resolved Hide resolved
@jxs jxs added the send-it label Jun 10, 2024
@mergify mergify bot merged commit 60e32c9 into libp2p:master Jun 10, 2024
72 checks passed
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
@oblique @TimTinkers
fix(ping): Fix panic in WASM caused by retrying on dial upgrade errors
6a9c333 
This PR workarounds async-rs/futures-timer#74 issue.

Fixes libp2p#5442

Pull-Request: libp2p#5447.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jxs jxs jxs approved these changes

@dariusc93 dariusc93 Awaiting requested review from dariusc93

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

libp2p-ping sometimes panics in WASM
3 participants
@oblique @jxs @dariusc93