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

Fix debug assert with reordered ACKs #1893

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Fix debug assert with reordered ACKs #1893

merged 1 commit into from
Jul 3, 2024

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Jun 7, 2024

Fixes #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
Copy link

rumenov commented Jun 17, 2024

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
Copy link
Member

djc commented Jun 17, 2024

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
Copy link

rumenov commented Jun 25, 2024

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.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, this makes sense to me.

@djc djc enabled auto-merge (rebase) July 3, 2024 13:06
@djc djc merged commit fb63829 into main Jul 3, 2024
8 checks passed
@djc djc deleted the fix-ack-panic branch July 3, 2024 13:11
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 this pull request may close these issues.

ACKs are delivered in order panic when packets are reordered
3 participants