Fixing bug with END_STREAM if header has continuations #22626
Conversation
|
|
bocon13
force-pushed
the
end-stream-fix
branch
5 times, most recently
from
9d30088 to
9eb4b13
Compare
ericgribkoff
added
kokoro:run
lang/core
release notes: yes
|
I've finished all of the expected changes on my side. Can you please rerun the whole test suite so that I can see if there are any other issues to fix? Thanks! |
- The HEADER frame should get the END_STREAM flag per the HTTP/2 spec; the old code put END_STREAM on the last CONTINUATION frame. - Removing deprecated parameter, is_last_in_stream, from finish_frame(); it has been superseded by the is_end_of_stream member of the framer_state struct. - Adding some gRPC frame validation tests to hpack_encoder_test.cc, and explicting checking that the END_STREAM flag is not on the CONTINUATION frame. fixes grpc#21436
|
@yashykt no problem and thanks for the review! Regarding the failing checks, it looks to me to the ObjC iOS tests are unrelated timeouts, and the Interop ALTS tests are unrelated Docker build errors. One of the RBE ASAN C/C++ errors was related, and should be solved by removing the code you requested. I'm not really sure about the other one: My most recent commit has squashed everything into 1 commit and rebased to latest master. |
|
@yashykt @ericgribkoff thanks for all of the help and reviews! Can we also backport this to 1.28? I'm new here, and couldn't find the criteria for being included in the support branch. Please let me know if there's anything that you need me to do. Thanks! |
|
Issues : #18892 |
|
Thanks for the fix! @bocon13 if you can open a similar PR for v1.28.x, we can merge it for you |
|
Awesome! Thanks, @yashykt, for merging it! I've submitted two PRs to backport the fix to the 1.28 and the newly created 1.29 branches. |
The HEADER frame should get the END_STREAM flag per the HTTP/2 spec.
fixes #21436