Fix 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.
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.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
"register progress" and "update last progress" carry the same meaning
dtikhonov
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 14 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