libp2p-ping sometimes panics in WASM #5442

Activity

1. dariusc93 commented on Jun 4, 2024

   

   dariusc93

   on Jun 4, 2024

   Member

   More actions

   Thanks for the report and good catch. I havent been able to reproduce this myself though during my testing, so out of curiosity was there any specific conditions in which a panic would happen?
2. oblique commented on Jun 4, 2024

   

   oblique

   on Jun 4, 2024

   Last edited by oblique

   ContributorAuthor

   More actions

   It is a bit hard to reproduce it. In our case sometimes takes 30 minute to reproduce. The following flow triggers it:

   1. The first ping request is send (code)
   2. A timeout or IO error is produced (code). These can happen only because of network issues.
   3. In this line only self.outbound is reset.
   4. Because of (3), this is called for a second time and will panic.

   More info:

   • gloo-timers panic if an expired TimeoutFuture is reused (code).
   • futures-timer's native implementation does not panic, but they didn't apply this behavior for WASM (code)

   The following fixes the issue in libp2p-ping, however we need to recheck all the places that futures_timer::Delay is used in libp2p to make sure that no other places are affected.

   diff --git a/protocols/ping/src/handler.rs b/protocols/ping/src/handler.rs
   index b9bd25c52..af7a55a13 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 => {
3. dariusc93 commented on Jun 4, 2024

   

   dariusc93

   on Jun 4, 2024

   Member

   More actions

   Thanks for the info. I will see what i can do on my end to reproduce it just out of my curiosity. In the meantime, maybe we should work on getting a patch done here in libp2p-ping since it might be a while before somebody review your fix in futures-timer (going based on the previous history there).

   👍React with 👍1Reacted by João Oliveira
4. oblique

   added a commit that references this issue on Jun 6, 2024

   fix(ping): Fixes panic in WASM caused by retrying on dail errors

   ...

   This issue will close once commit 85fdf92 is merged into the 'master' branch.

   85fdf92
5. oblique commented on Jun 6, 2024

   

   oblique

   on Jun 6, 2024

   Last edited by oblique

   ContributorAuthor

   More actions

   I created a repository with reproduction: https://github.com/oblique/libp2p-issue-5442
   Please read README.md for more info.

   👍React with 👍1Reacted by Darius Clark
6. oblique

   added a commit that references this issue on Jun 6, 2024

   fix(ping): Fixes panic in WASM caused by retrying on dail errors

   ...

   This issue will close once commit 7b8185a is merged into the 'master' branch.

   7b8185a
7. mergify

   closed this as completedin #5447on Jun 10, 2024
8. mergify

   added a commit that references this issue on Jun 10, 2024

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

   ...

   This issue will close once commit 60e32c9 is merged into the 'master' branch.

   60e32c9
9. TimTinkers

   added a commit that references this issue on Sep 14, 2024

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

   ...

   This issue will close once commit 6a9c333 is merged into the 'master' branch.

   6a9c333
10. AdetsGithub

    added a commit that references this issue on Oct 16, 2026

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

    ...

    This issue will close once commit dc01f1f is merged into the 'master' branch.

    dc01f1f