-
Notifications
You must be signed in to change notification settings - Fork 185
Respect FLAG_FIN & FLAG_RST in TYPE_WINDOW_UPDATE frames #932
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
base: main
Are you sure you want to change the base?
Conversation
Hi @vegas503, The fix looks correct overall. Would you be able to add some unit tests for this? Specifically testing WINDOW_UPDATE frames with FLAG_FIN and FLAG_RST. |
Thanks @vegas503 for identifying and fixing this important spec compliance issue, and @paschal533 for the thoughtful review and feedback 🙏 The added handling of FIN & RST flags in WINDOW_UPDATE frames is a solid improvement, and the suggestions around unit testing and validation against real yamux implementations will help ensure correctness and stability. Great collaboration here — once the tests and cleanup are in place, this will be a strong addition to the codebase. CCing @acul71 , @lla-dane and @sumanjeet0012 for peer review and feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Review: Respect FLAG_FIN & FLAG_RST in TYPE_WINDOW_UPDATE frames (#932)
Summary
This PR addresses a spec compliance issue in the yamux implementation where FLAG_FIN
and FLAG_RST
flags in TYPE_WINDOW_UPDATE
frames were not being properly handled.
Spec Verification: The yamux specification explicitly states:
0x4 FIN - Performs a half-close of a stream. May be sent with a data message or window update.
0x8 RST - Reset a stream immediately. May be sent with a data or window update message.
- "To close a stream, either side sends a data or window update frame along with the FIN flag."
Changes Made
- Added handling for
FLAG_FIN
inTYPE_WINDOW_UPDATE
frames (lines 664-672) - Added handling for
FLAG_RST
inTYPE_WINDOW_UPDATE
frames (line 579) - Both flags now properly trigger stream state changes (recv_closed for FIN, reset for RST)
Code Quality
✅ Good: The fix follows existing patterns in the codebase
✅ Good: Proper logging and stream state management
✅ Good: Thread-safe implementation with appropriate locks
Concerns
Recommendation
Approve with conditions: The fix appears correct and addresses a real spec compliance issue. However, unit tests should be added before merging to ensure the behavior works as expected and to prevent regressions.
Follow-up Actions
- Add unit tests for
WINDOW_UPDATE
frames withFLAG_FIN
andFLAG_RST
- Test against real yamux implementations for validation
- Update relevant documentation
- Clean up commit history as mentioned in the PR description
@vegas503 what's the status of this PR? |
What was wrong?
Issue #931
How was it fixed?
Added handling of FIN & RST flags in WINDOW_UPDATE frames.
To-Do