Proposed fix to the no-progress timeout logic #574
Conversation
The two timeouts -- the idle timeout specified in the RFCs and the "user progress" timeout that we ourlselves defined -- should use different alarms. This makes for cleaner code that is easier to reason about. Closes #531
There was a problem hiding this comment.
Pull Request Overview
This PR refactors timeout logic to separate idle timeout (RFC-defined) from user progress timeout (custom implementation) by introducing a dedicated alarm for user stream progress tracking. The change eliminates shared state between these two timeout mechanisms, making the code more maintainable and easier to reason about.
Key changes:
- Adds
AL_USER_STREAMalarm to track user progress timeout separately from idle timeout - Removes shared timeout state from
lsquic_conn_publicstructure - Introduces
ci_user_stream_progresscallback interface for stream progress reporting
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/liblsquic/lsquic_alarmset.h | Adds new AL_USER_STREAM alarm type and corresponding bit flag |
| src/liblsquic/lsquic_alarmset.c | Adds string representation for new alarm type |
| src/liblsquic/lsquic_conn.h | Defines new ci_user_stream_progress callback interface |
| src/liblsquic/lsquic_conn_public.h | Removes shared last_tick and last_prog fields |
| src/liblsquic/lsquic_stream.h | Removes debug-only sm_last_prog field |
| src/liblsquic/lsquic_stream.c | Refactors progress tracking to use new callback mechanism |
| src/liblsquic/lsquic_full_conn_ietf.c | Implements separate alarm handling for user progress timeout |
| src/liblsquic/lsquic_full_conn.c | Implements separate alarm handling for user progress timeout |
| src/liblsquic/lsquic_send_ctl.c | Removes progress update logic from ACK handling |
| tests/test_stream.c | Adds stub implementation of new callback interface |
| tests/test_send_headers.c | Adds stub implementation of new callback interface |
| tests/test_h3_framing.c | Adds stub implementation of new callback interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (lsquic_alarmset_is_set(&conn->ifc_alset, AL_USER_STREAM) | ||
| && expiry > conn->ifc_alset.as_expiry[AL_USER_STREAM]) |
There was a problem hiding this comment.
The condition only extends the alarm if it's already set and the new expiry is later. This means the first call to ci_user_stream_progress won't set the alarm if it hasn't been initialized yet. Consider handling the case where the alarm is not yet set, or document why this behavior is intentional.
| if (lsquic_alarmset_is_set(&conn->ifc_alset, AL_USER_STREAM) | |
| && expiry > conn->ifc_alset.as_expiry[AL_USER_STREAM]) | |
| if (!lsquic_alarmset_is_set(&conn->ifc_alset, AL_USER_STREAM) | |
| || expiry > conn->ifc_alset.as_expiry[AL_USER_STREAM]) |
There was a problem hiding this comment.
This code is intentional: The check for alarm being set is effectively checking whether the no-progress timeout is enabled. This is in lieu of the removed flag.
| if (lsquic_alarmset_is_set(&conn->fc_alset, AL_USER_STREAM) | ||
| && expiry > conn->fc_alset.as_expiry[AL_USER_STREAM]) |
There was a problem hiding this comment.
The condition only extends the alarm if it's already set and the new expiry is later. This means the first call to ci_user_stream_progress won't set the alarm if it hasn't been initialized yet. Consider handling the case where the alarm is not yet set, or document why this behavior is intentional.
| if (lsquic_alarmset_is_set(&conn->fc_alset, AL_USER_STREAM) | |
| && expiry > conn->fc_alset.as_expiry[AL_USER_STREAM]) | |
| if (!lsquic_alarmset_is_set(&conn->fc_alset, AL_USER_STREAM) | |
| || expiry > conn->fc_alset.as_expiry[AL_USER_STREAM]) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
The two timeouts -- the idle timeout specified in the RFCs and the "user progress" timeout that we ourlselves defined -- should use different alarms. This makes for cleaner code that is easier to reason about.
Closes #531