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

Clarify Connection::space_can_send logic #1851

Merged
merged 5 commits into from
May 12, 2024
Merged

Clarify Connection::space_can_send logic #1851

merged 5 commits into from
May 12, 2024

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented May 8, 2024

No description provided.

@Ralith Ralith force-pushed the can-send-acks branch 2 times, most recently from b007489 to c56bac4 Compare May 8, 2024 05:32
@Ralith Ralith changed the title Don't assert on ACK-only packets when application data is pending Clarify Connection::space_can_send logic May 8, 2024
@Ralith
Copy link
Collaborator Author

Ralith commented May 8, 2024

Testing and re-examination reveals that this doesn't actually change any behavior, because if ACKs were sendable then SendableFrames::acks would have been set by the first PacketSpace::can_send call. We only reach the case that hard-codes that to false if we already know it would have returned false. Cleaned up the logic a bunch to avoid future confusion; please review carefully for absence of semantic changes.

quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Show resolved Hide resolved
Ralith added 3 commits May 9, 2024 18:58
When this check fails, indicating that `can_send ==
SendableFrames::empty()`, we return `SendableFrames::empty()` anyway.
@djc djc merged commit d613895 into main May 12, 2024
8 checks passed
@djc djc deleted the can-send-acks branch May 12, 2024 09:13
@djc
Copy link
Collaborator

djc commented May 12, 2024

Much easier to review, thanks!

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.

None yet

2 participants