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

Ping protocol tests fail when mplex is used instead of yamux. #1758

Closed
artemii235 opened this issue Sep 15, 2020 · 3 comments · Fixed by #1770
Closed

Ping protocol tests fail when mplex is used instead of yamux. #1758

artemii235 opened this issue Sep 15, 2020 · 3 comments · Fixed by #1770
Assignees

Comments

@artemii235
Copy link

Some background

Our team is using Ping protocol as part of bigger combined behaviour that derives NetworkBehaviour. We had few issues with Yamux so switched to Mplex. We've noticed ping timeouts while there's no actual reason for this to happen so I started digging and discovered that Ping doesn't seem to work properly when Mplex is used.

How to reproduce:

  1. Rust toolchain used: nightly-2020-02-01-x86_64-apple-darwin
  2. Use the latest master state.
  3. Use Mplex instead of Yamux at https://github.com/libp2p/rust-libp2p/blob/master/protocols/ping/tests/ping.rs#L209
  4. Run Ping protocol tests.

Expected behavior:

Tests pass

Actual behavior

max_failures fails and ping_pong runs indefinitely.

@romanb
Copy link
Contributor

romanb commented Sep 15, 2020

We had few issues with Yamux so switched to Mplex.

It would be great if you could report any issues you encountered at https://github.com/paritytech/yamux, if you think there is a problem in the Yamux implementation.

@romanb romanb self-assigned this Sep 15, 2020
@artemii235
Copy link
Author

artemii235 commented Sep 16, 2020

It would be great if you could report any issues you encountered at https://github.com/paritytech/yamux

We did our tests a couple of months ago, things might have changed as Yamux version was upgraded. I'm going to retest the Yamux today and check whether problems we encountered are solved. If not I will create an issue in Yamux repo.

@romanb
Copy link
Contributor

romanb commented Sep 28, 2020

This should now be fixed in master and there will probably be new releases this week. Thanks a lot for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants