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

ACKs are delivered in order panic when packets are reordered #1889

Closed
tthebst opened this issue Jun 6, 2024 · 0 comments · Fixed by #1893
Closed

ACKs are delivered in order panic when packets are reordered #1889

tthebst opened this issue Jun 6, 2024 · 0 comments · Fixed by #1893

Comments

@tthebst
Copy link
Contributor

tthebst commented Jun 6, 2024

I was trying to upgrade to quinn 0.11.2 and I have some tests where I implement AsyncUdpSocket with turmoil and my tests panic with ACKs are delivered in order (line 392 in mtud.rs) which originates from the BlackHoleDetector . After some experimentation I found that this happens because in our test with turmoil we have reordering of packets.

The issue can be reproduced if you locally add some reordering sudo tc qdisc add dev lo root netem delay 50ms 100ms and send some messages between a client and a server.

thread 'tokio-runtime-worker' panicked at quinn-proto/src/connection/mtud.rs:392:9:
ACKs are delivered in order
stack backtrace:
hello
   0: rust_begin_unwind
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:72:14
   2: quinn_proto::connection::mtud::BlackHoleDetector::on_non_probe_acked
             at ./quinn-proto/src/connection/mtud.rs:392:9
   3: quinn_proto::connection::mtud::MtuDiscovery::on_acked
             at ./quinn-proto/src/connection/mtud.rs:105:13
   4: quinn_proto::connection::Connection::on_ack_received
             at ./quinn-proto/src/connection/mod.rs:1387:35
   5: quinn_proto::connection::Connection::process_payload
             at ./quinn-proto/src/connection/mod.rs:2670:21
   6: quinn_proto::connection::Connection::process_decrypted_packet
             at ./quinn-proto/src/connection/mod.rs:2299:38
   7: quinn_proto::connection::Connection::handle_packet
             at ./quinn-proto/src/connection/mod.rs:2237:21
   8: quinn_proto::connection::Connection::handle_decode
             at ./quinn-proto/src/connection/mod.rs:2133:13
   9: quinn_proto::connection::Connection::handle_event
             at ./quinn-proto/src/connection/mod.rs:1069:17
  10: quinn::connection::State::process_conn_events
             at ./quinn/src/connection.rs:1026:21
  11: <quinn::connection::ConnectionDriver as core::future::future::Future>::poll
             at ./quinn/src/connection.rs:230:25
  12: quinn::connection::Connecting::new::{{closure}}
             at ./quinn/src/connection.rs:66:40
  13: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll

This PR should fix the issue. It removes the ACK ordering requirement for non mtu probe acks.

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