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

Add pointers about idle timeout in documentation #1875

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ruuda
Copy link

@ruuda ruuda commented Dec 1, 2024

I spent an evening debugging an application, where both ends of the connection were timing out at roughly the same time. I mistakenly assumed that on_timeout would cause ping packets to be sent to keep the connection alive, but after reading through the RFC, it dawned on me that applications are responsible for keeping the connection alive, and Quiche merely detects the timeout. This design does make sense, but a remark like the one I'm adding here would have saved me a lot of time, so I hope it can be helpful to others in the future.

I spent an evening debugging an application, where both ends of the
connection were timing out at roughly the same time. I mistakenly
assumed that 'on_timeout' would cause ping packets to be sent to keep
the connection alive, but after reading through the RFC, it dawned on
me that applications are responsible for keeping the connection alive,
and Quiche merely detects the timeout. This design does make sense, but
a remark like the one I'm adding here would have saved me a lot of time,
so I hope it can be helpful to others in the future.
@ruuda ruuda requested a review from a team as a code owner December 1, 2024 21:14
/// if there is no data to send. See also the [Deferring Idle Timeout
/// section of RFC 9000][idle].
///
/// [idle]: https://www.rfc-editor.org/rfc/rfc9000.html#name-deferring-idle-timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a note to help users seems sensible but I don't think the proposal covers the considerations with enough nuance.

The QUIC idle timeout is negotiated between endpoints, so it doesn't necessarily matter what the local endpoint configured, since the peer can affect it. This suggests that we might want to expose the value of the idle timeout. timeout() and timeout_instant() can return values unrelated to the idle timeout, so aren't necessarily correct.

Also, idle timeout is a useful feature of QUIC, so we shouldn't unilaterally recommend that applications keep a connection alive when there may be no good reason to. The RFC text seems sufficient in terms of guidance. Pointing to the functions that reset the idle timer seems helpful though.

@hawkinsw
Copy link
Contributor

I spent an evening debugging an application, where both ends of the connection were timing out at roughly the same time. I mistakenly assumed that on_timeout would cause ping packets to be sent to keep the connection alive, but after reading through the RFC, it dawned on me that applications are responsible for keeping the connection alive, and Quiche merely detects the timeout. This design does make sense, but a remark like the one I'm adding here would have saved me a lot of time, so I hope it can be helpful to others in the future.

Thank you for taking the initiative on adding documentation for on_timeout. In an application I am writing I, too, struggled with using this function correctly. I will defer to the experts on the best way to improve the documentation (e.g., you and @LPardue ), but I know that more documentation will really help users like me!

Thank you, again!

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.

3 participants