[+] Fix: Support more specific idletimeout calculation#535
[+] Fix: Support more specific idletimeout calculation#535
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts QUIC connection idle-timeout handling to better align with RFC 9000 semantics, and tightens stateless reset handling.
Changes:
- Reset the connection idle timer on sending ACK-eliciting packets (with new
conn_last_ack_eliciting_send_timetracking). - Refine effective idle-timeout computation to consider both local and remote advertised values.
- Fix stateless reset minimum-length parsing and update stateless reset token association to the server’s CID.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/transport/xqc_send_ctl.c | Updates connection idle timer when sending ACK-eliciting packets and tracks last ACK-eliciting send time. |
| src/transport/xqc_packet_parser.c | Corrects stateless reset minimum-length check (< vs <=). |
| src/transport/xqc_conn.h | Adds conn_last_ack_eliciting_send_time to the connection struct. |
| src/transport/xqc_conn.c | Initializes conn_paths_list earlier, changes stateless reset token generation CID, and updates idle-timeout calculation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| xqc_usec_t pto = xqc_conn_get_max_pto(conn); | ||
| if (pto > 0) { | ||
| xqc_msec_t min_idle_timeout = (xqc_msec_t)((3 * pto + 999) / 1000); | ||
| idle_timeout = xqc_max(idle_timeout, min_idle_timeout); | ||
| } | ||
|
|
There was a problem hiding this comment.
xqc_conn_get_idle_timeout() computes the effective idle timeout as min(local, remote) but then increases it to at least 3PTO. This can make the actual idle timeout exceed the negotiated/advertised max_idle_timeout, which is contrary to RFC 9000’s definition of the effective value (minimum of the two advertised values). If you need a minimum duration of 3PTO, that should be applied to the closing/draining timers (or a separate timeout) rather than the negotiated idle timeout.
| xqc_usec_t pto = xqc_conn_get_max_pto(conn); | |
| if (pto > 0) { | |
| xqc_msec_t min_idle_timeout = (xqc_msec_t)((3 * pto + 999) / 1000); | |
| idle_timeout = xqc_max(idle_timeout, min_idle_timeout); | |
| } |
| xqc_msec_t local = conn->local_settings.max_idle_timeout == 0 | ||
| ? XQC_CONN_DEFAULT_IDLE_TIMEOUT : conn->local_settings.max_idle_timeout; | ||
|
|
||
| xqc_msec_t remote_idle_timeout = (xqc_msec_t)conn->remote_settings.max_idle_timeout; | ||
| xqc_msec_t idle_timeout = (remote_idle_timeout > 0) ? xqc_min(local, remote_idle_timeout) : local; |
There was a problem hiding this comment.
The changes in xqc_conn_get_idle_timeout() alter timeout semantics (using both local and remote advertised values). There are no unit tests covering the effective idle-timeout calculation; please add targeted cases in tests/unittest/xqc_conn_test.c to prevent regressions.
As RFC 9000 wrote here :