search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

Fix debug assert with reordered ACKs #1893

Closed Ralith closed 2 months ago

Ralith commented 3 months ago

Fixes https://github.com/quinn-rs/quinn/issues/1889.

On careful examination, I think @tthebst's instinct was correct: the assert was fundamentally wrong, and our logic works as intended without it. I've hence removed the assert and attempted to better document why this is okay, but please double-check my thinking.

rumenov commented 3 months ago

Hello folks, when can we have this MR reviewed?

The MR fixes our turmoil based unit tests of a library that uses QUINN. Fixing the unit tests will unblock the QUINN and RUSTLS upgrades on our side.

djc commented 3 months ago

Sorry, I want to spend a little time contemplating this change but haven't gotten to it. Hopefully in the next few days. Definitely haven't forgotten about it.

rumenov commented 3 months ago

Sorry, I want to spend a little time contemplating this change but haven't gotten to it. Hopefully in the next few days. Definitely haven't forgotten about it.

Thank you @djc!

I was just worried it fell through the cracks because removing a debug_assert may seem unimportant.