quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.57k stars 364 forks source link

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

Open tthebst opened 3 weeks ago

tthebst commented 3 weeks ago

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.