libp2p-ping sometimes panics in WASM

[oblique]

Summary

While researching our eigerco/lumina#256 bug we found one of the causes were bacause libp2p-ping was panicking.

This was due to inconsistent behavior in futures-timer when it was used in WASM.

Expected behavior

Not to panic

Actual behavior

Panics

Relevant log output

No response

Possible Solution

We fixed the issue with async-rs/futures-timer#74 PR and we are waiting to be reviewed and get merged.

If futures-timer rejects or do not merge our fix, then another fix can be applied directly in libp2p-ping.

Version

No response

Would you like to work on fixing this bug ?

Yes

[dariusc93]

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?

[oblique]

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 => {

[dariusc93]

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).

[oblique]

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