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

Send ACK_FREQUENCY frames intelligently rather than spamming IMMEDIATE_ACK #1656

Merged
merged 7 commits into from
Sep 9, 2023

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Sep 2, 2023

Alternative to #1654. Reduces the amount of communication required compared to the status quo when the RTT is not varying wildly.

This preserves the pre-existing behavior where max_ack_delay is effectively clamped below the current RTT. It might be useful to also offer an escape hatch to force it higher, although such a configuration is contrary to the "SHOULD" recommendation of the ack frequency draft.

@Ralith Ralith force-pushed the intelligent-ack-frequency branch 3 times, most recently from bd03260 to 65a5aee Compare September 2, 2023 23:13
quinn-proto/src/tests/util.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/ack_frequency.rs Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Sep 8, 2023

I'm honestly not completely confident I understand all the nuance here, but it seems like a nice simplification and looks like we got decent test coverage of what's going on.

@Ralith Ralith force-pushed the intelligent-ack-frequency branch from 65a5aee to be696c0 Compare September 9, 2023 17:16
@Ralith Ralith enabled auto-merge (rebase) September 9, 2023 17:17
@Ralith Ralith merged commit 0af891e into main Sep 9, 2023
8 checks passed
@Ralith Ralith deleted the intelligent-ack-frequency branch September 9, 2023 17: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.

2 participants