-
Notifications
You must be signed in to change notification settings - Fork 4.6k
transport: make the client send a RST_STREAM when it receives an END_STREAM from the server #8832
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: master
Are you sure you want to change the base?
Conversation
…ilers if client is not half-closed
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8832 +/- ##
==========================================
- Coverage 83.29% 83.20% -0.09%
==========================================
Files 414 414
Lines 32753 32721 -32
==========================================
- Hits 27280 27226 -54
- Misses 4070 4079 +9
- Partials 1403 1416 +13
🚀 New features to boost your workflow:
|
arjan-bal
left a comment
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.
IIUC, the situation mentioned in #835 is not covered. Specifically, the server sends trailers with the EOS flag but does not send an RST_STREAM frame. The client is expected to send an RST_STREAM to inform proxies about the stream closure.
easwars
left a comment
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.
IIUC, the situation mentioned in #835 is not covered. Specifically, the server sends trailers with the EOS flag but does not send an RST_STREAM frame. The client is expected to send an RST_STREAM to inform proxies about the stream closure.
Added a test for it, and combined all three scenarios for the client into a single table driven test.
arjan-bal
left a comment
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.
LGTM!
dfawley
left a comment
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.
It would probably be a good idea to write a test that ensures unread messages can still be read when this happens. I.e.:
- Start stream; client sits idle for now
- Server sends a few messages and a status
- After the server has done this and we know the client has finished calling
closeStream(somehow...maybe the server closes the connection (gracefully?) and we check the connectivity state?), client then reads the messages out of the stream.
Done. I added a test to to verify that the client can still read unread messages after sending a RST_STREAM upon receiving an EOS from the server. I also refactored the existing table-driven test into individual, clearer test functions using a shared helper as the test logic was getting more and more complicated. |
Fixes #835
This PR fixes the behavior of the client to send a RST_STREAM when it receives an END_STREAM from the server when the client-side of the stream is still open. It also adds tests for both the client and server side behaviors of sending RST_STREAM when they receive an END_STREAM from their peer.
RELEASE NOTES: