From 7b8185aec034a4af3ac538db2c15df3cb98f9275 Mon Sep 17 00:00:00 2001 From: Yiannis Marangos Date: Thu, 6 Jun 2024 15:10:14 +0300 Subject: [PATCH 1/7] fix(ping): Fixes panic in WASM caused by retrying on dail errors This PR workarounds async-rs/futures-timer#74 issue. Fixes #5442 --- protocols/ping/src/handler.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/protocols/ping/src/handler.rs b/protocols/ping/src/handler.rs index b9bd25c52ea..af7a55a13bd 100644 --- a/protocols/ping/src/handler.rs +++ b/protocols/ping/src/handler.rs @@ -183,6 +183,7 @@ impl Handler { >, ) { self.outbound = None; // Request a new substream on the next `poll`. + self.interval.reset(Duration::new(0, 0)); let error = match error { StreamUpgradeError::NegotiationFailed => { From fbb12117a81eccc4b408a354901d1d438ac4c7dc Mon Sep 17 00:00:00 2001 From: Yiannis Marangos Date: Thu, 6 Jun 2024 18:21:55 +0300 Subject: [PATCH 2/7] update changelog and version --- Cargo.toml | 2 +- protocols/ping/CHANGELOG.md | 8 +++++++- protocols/ping/Cargo.toml | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6c6dc114983..b2fb3193026 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -93,7 +93,7 @@ libp2p-mplex = { version = "0.41.0", path = "muxers/mplex" } libp2p-muxer-test-harness = { path = "muxers/test-harness" } libp2p-noise = { version = "0.44.0", path = "transports/noise" } libp2p-perf = { version = "0.3.0", path = "protocols/perf" } -libp2p-ping = { version = "0.44.1", path = "protocols/ping" } +libp2p-ping = { version = "0.44.2", path = "protocols/ping" } libp2p-plaintext = { version = "0.41.0", path = "transports/plaintext" } libp2p-pnet = { version = "0.24.0", path = "transports/pnet" } libp2p-quic = { version = "0.10.3", path = "transports/quic" } diff --git a/protocols/ping/CHANGELOG.md b/protocols/ping/CHANGELOG.md index 17338a4ba9a..7163dff7399 100644 --- a/protocols/ping/CHANGELOG.md +++ b/protocols/ping/CHANGELOG.md @@ -1,9 +1,15 @@ -## 0.44.1 - unreleased +## 0.44.2 + +- Fixes panic in WASM caused by retrying on dail errors. + See [PR 5447](https://github.com/libp2p/rust-libp2p/pull/5447). + +## 0.44.1 - Impose `Sync` on `ping::Failure::Other`. `ping::Event` can now be shared between threads. See [PR 5250] + [PR 5250]: https://github.com/libp2p/rust-libp2p/pull/5250 ## 0.44.0 diff --git a/protocols/ping/Cargo.toml b/protocols/ping/Cargo.toml index c436478668c..9c85bd6934e 100644 --- a/protocols/ping/Cargo.toml +++ b/protocols/ping/Cargo.toml @@ -3,7 +3,7 @@ name = "libp2p-ping" edition = "2021" rust-version = { workspace = true } description = "Ping protocol for libp2p" -version = "0.44.1" +version = "0.44.2" authors = ["Parity Technologies "] license = "MIT" repository = "https://github.com/libp2p/rust-libp2p" From 24826ea00819fdda04fc55ef5e3b0b4ef2d661c8 Mon Sep 17 00:00:00 2001 From: Yiannis Marangos Date: Thu, 6 Jun 2024 18:31:20 +0300 Subject: [PATCH 3/7] update cargo.lock --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a3e98555eca..f9106f44d81 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3113,7 +3113,7 @@ dependencies = [ [[package]] name = "libp2p-ping" -version = "0.44.1" +version = "0.44.2" dependencies = [ "async-std", "either", From d8cb21214eaf879c2a9fefee56009001381a2e06 Mon Sep 17 00:00:00 2001 From: Yiannis Marangos Date: Sat, 8 Jun 2024 12:15:51 +0300 Subject: [PATCH 4/7] add comments --- protocols/ping/src/handler.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/protocols/ping/src/handler.rs b/protocols/ping/src/handler.rs index af7a55a13bd..6ab6e2db71d 100644 --- a/protocols/ping/src/handler.rs +++ b/protocols/ping/src/handler.rs @@ -183,6 +183,17 @@ impl Handler { >, ) { self.outbound = None; // Request a new substream on the next `poll`. + + // Timer is already polled and expired before substream request is initiated + // and will be polled again later on in our `poll` because we reset `self.outbound`. + // + // `futures-timer` allows an expired timer to be polled again and returns + // immidietly a `Poll::Ready`. However in its WASM implementation there is + // a bug that causes the expired timer to panic. + // + // This is a workaround until a proper fix is merged and released. + // + // See async-rs/futures-timer#74 and libp2p/rust-libp2p#5447 for more info. self.interval.reset(Duration::new(0, 0)); let error = match error { From f4fb1d5fd06f117ef37657875205d4322aa93a41 Mon Sep 17 00:00:00 2001 From: Yiannis Marangos Date: Sat, 8 Jun 2024 21:43:16 +0300 Subject: [PATCH 5/7] Update protocols/ping/CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: João Oliveira --- protocols/ping/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/ping/CHANGELOG.md b/protocols/ping/CHANGELOG.md index 0c1f856b380..b6acd704aaf 100644 --- a/protocols/ping/CHANGELOG.md +++ b/protocols/ping/CHANGELOG.md @@ -3,7 +3,7 @@ - Use `web-time` instead of `instant`. See [PR 5347](https://github.com/libp2p/rust-libp2p/pull/5347). -- Fixes panic in WASM caused by retrying on dail errors. +- Fix panic in WASM caused by retrying on dail errors. See [PR 5447](https://github.com/libp2p/rust-libp2p/pull/5447). ## 0.44.1 From 2ea753ceafdd289e7934394fc044165912e9d4e6 Mon Sep 17 00:00:00 2001 From: Yiannis Marangos Date: Sat, 8 Jun 2024 21:43:25 +0300 Subject: [PATCH 6/7] Update protocols/ping/src/handler.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: João Oliveira --- protocols/ping/src/handler.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/ping/src/handler.rs b/protocols/ping/src/handler.rs index 6ab6e2db71d..7ab3e8bcd2e 100644 --- a/protocols/ping/src/handler.rs +++ b/protocols/ping/src/handler.rs @@ -188,7 +188,7 @@ impl Handler { // and will be polled again later on in our `poll` because we reset `self.outbound`. // // `futures-timer` allows an expired timer to be polled again and returns - // immidietly a `Poll::Ready`. However in its WASM implementation there is + // immediately `Poll::Ready`. However in its WASM implementation there is // a bug that causes the expired timer to panic. // // This is a workaround until a proper fix is merged and released. From abb41978fb8be68ffd8eab62faac78ca2a2a8100 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Oliveira?= Date: Mon, 10 Jun 2024 23:01:38 +0100 Subject: [PATCH 7/7] Apply suggestions from code review --- protocols/ping/CHANGELOG.md | 2 +- protocols/ping/src/handler.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/protocols/ping/CHANGELOG.md b/protocols/ping/CHANGELOG.md index 5330d10bc54..7c7d7f3a54f 100644 --- a/protocols/ping/CHANGELOG.md +++ b/protocols/ping/CHANGELOG.md @@ -3,7 +3,7 @@ - Use `web-time` instead of `instant`. See [PR 5347](https://github.com/libp2p/rust-libp2p/pull/5347). -- Fix panic in WASM caused by retrying on dail errors. +- Fix panic in WASM caused by retrying on dial upgrade errors. See [PR 5447](https://github.com/libp2p/rust-libp2p/pull/5447). ## 0.44.1 diff --git a/protocols/ping/src/handler.rs b/protocols/ping/src/handler.rs index 7ab3e8bcd2e..2816cdc4048 100644 --- a/protocols/ping/src/handler.rs +++ b/protocols/ping/src/handler.rs @@ -190,10 +190,10 @@ impl Handler { // `futures-timer` allows an expired timer to be polled again and returns // immediately `Poll::Ready`. However in its WASM implementation there is // a bug that causes the expired timer to panic. - // // This is a workaround until a proper fix is merged and released. + // See libp2p/rust-libp2p#5447 for more info. // - // See async-rs/futures-timer#74 and libp2p/rust-libp2p#5447 for more info. + // TODO: remove when async-rs/futures-timer#74 gets merged. self.interval.reset(Duration::new(0, 0)); let error = match error {