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

Suppress ACK-only packet sanity-check when datagrams are queued #1854

Merged
merged 4 commits into from
May 8, 2024

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented May 8, 2024

Fixes #1850.

This bug caused occasional panics in debug builds in applications which send large application datagrams. The debug_assert did not account for the possibility of ACK-only packets arising due to queued non-ACK being too large to fit into the same packet, which became a problem when 6a8758a introduced the possibility of ACK frames being sent alongside application data (such as large application datagrams) at times when it would be illegal to send an ACK frame spontaneously on its own.

@djc
Copy link
Collaborator

djc commented May 8, 2024

How hard is it to add a test for this? (No need to block a release on this.) Do you want to add a version bump?

@Ralith
Copy link
Collaborator Author

Ralith commented May 8, 2024

How hard is it to add a test for this?

Should be feasible.

Do you want to add a version bump?

Sure, soon as I finish diagnosing some related funny behavior...

@Ralith Ralith requested a review from djc May 8, 2024 20:45
The prior logic could even produce panics if segment size falls below
MIN_PACKET_SPACE.
@Ralith
Copy link
Collaborator Author

Ralith commented May 8, 2024

Fixed and tested a related issue (stealing a variation on the unit test that was previously in #1851), still need a test for the original issue, but manually running the repro from #1850 now succeeds so I think we're okay to proceed with a point release.

@djc djc merged commit a35ad73 into main May 8, 2024
8 checks passed
@djc djc deleted the fix-sendable-frames branch May 8, 2024 21:21
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.

"SendableFrames was SendableFrames { acks: false, other: true }, but only ACKs have been written"
2 participants