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

#2008: Make max_idle_timeout negotiation commutative. #2009

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented Oct 9, 2024

Fixes #2008.

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.

These changes make sense to me, thanks!

quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
mstyura added a commit to mstyura/quinn that referenced this pull request Oct 9, 2024
mstyura added a commit to mstyura/quinn that referenced this pull request Oct 9, 2024
mstyura added a commit to mstyura/quinn that referenced this pull request Oct 9, 2024
# This is the 1st commit message:

quinn-rs#2008: Make max_idle_timeout negotiation commutative.

# This is the commit message quinn-rs#2:

quinn-rs#2008: Store negotiated idle_timeout as Duration.

# This is the commit message quinn-rs#3:

quinn-rs#2009: Address code review feedback.
@mstyura mstyura force-pushed the gh-issue-2008 branch 2 times, most recently from c5882f4 to 11e6de0 Compare October 9, 2024 14:17
Ensure that both endpoints can agree on the same idle_timeout value,
regardless of the announced max_idle_timeout parameters.
This fix addresses a corner case where a locally configured idle_timeout
of 0 was used as the negotiated value instead of the non-zero
max_idle_timeout provided by the remote side.
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

The original intent here was that users pass None to TransportConfig::max_idle_timeout, rather than Some(0), if no maximum timeout is desired. I wonder if we should revisit that interface to make this less of a footgun? For example, we could make IdleTimeout require a nonzero number of milliseconds, or round up to 1, or something to that effect.

@djc
Copy link
Member

djc commented Oct 11, 2024

I wonder if we should revisit that interface to make this less of a footgun? For example, we could make IdleTimeout require a nonzero number of milliseconds, or round up to 1, or something to that effect.

Thinking about how we would explain that interface in documentation makes me feel that it would make things more complex rather than easier? We could potentially add some explicit documentation about None vs Some(0).

@Ralith Ralith added this pull request to the merge queue Oct 11, 2024
@Ralith
Copy link
Collaborator

Ralith commented Oct 11, 2024

This is a strict non-breaking improvement, anyway.

Merged via the queue into quinn-rs:main with commit 31a95ee Oct 11, 2024
14 checks passed
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.

Incorrect max_idle_timeout negotiation.
3 participants