Skip to content
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

codec: support for half-open H2 streams #22748

Closed
kyessenov opened this issue Aug 17, 2022 · 2 comments · Fixed by #22802
Closed

codec: support for half-open H2 streams #22748

kyessenov opened this issue Aug 17, 2022 · 2 comments · Fixed by #22802
Assignees
Labels
enhancement Feature requests. Not bugs or questions.

Comments

@kyessenov
Copy link
Contributor

Current implementation does not handle half-closed HTTP streams well. When a response is received, the stream is assumed to be destroyed (in particular, the underlying connection can be closed if it's drained/idle). This is especially problematic for HTTP CONNECT streams: when tcp_proxy attempts to write end_stream after response is complete for a draining cluster (e.g. via CDS update), a crash occurs.

This is a feature request to support an explicit closure of request stream and delay stream destruction until then.

@kyessenov kyessenov added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Aug 17, 2022
@buffalu
Copy link

buffalu commented Aug 19, 2022

we (mainly @segfaultdoc) have been testing envoy with GRPC trying to make sure it'll work for our application. we have the following service file:

service ExampleService {
  rpc Echo (Message) returns (Message) {}
  rpc StreamToClient (Message) returns (stream Message) {}
  rpc StreamToServer (stream Message) returns (Message) {}
  rpc EchoStream (stream Message) returns (stream Message) {}
}

all of them work except StreamToServer. is this issue and #22754 supposed to address the issues we see with StreamToServer getting dropped on envoy but not directly?

@kyessenov
Copy link
Contributor Author

Yes, the issue is that Envoy considers the response completion as the stream completion, so if the client continues sending data after the upstream server ends its stream, the data is not sent upstream. This happens at the codec level, so there may be some variation in how gRPC SDKs encode streaming semantics in HTTP2.

@lizan lizan removed the triage Issue requires triage label Aug 24, 2022
kyessenov added a commit that referenced this issue Sep 1, 2022
Signed-off-by: Kuat Yessenov kuat@google.com

Commit Message: Hold ActiveRequest destruction until a full close instead of doing immediately on response decode.
Additional Description: This is needed to support half closed tcp_proxy CONNECT streams. Closing immediately on response triggers TCP connection close on the draining upstream which breaks tcp_proxy continuing to send data. Fixes #22748. See H2 state transition diagram here: https://httpwg.org/specs/rfc9113.html#StreamStates.

HTTP/1.1 is particularly difficult to change because message completion is triggered by the parser. This PR keeps the behavior unchanged for HTTP/1.1 codec.

QUIC requires adding STOP_SENDING support on client side, otherwise, the underlying stream is destroyed before the callbacks kick in. The test in protocol integration is updated to ensure that test processes client STOP_SENDING before closing the connection.

Risk Level: medium / high
Testing: manually verified the crash is fixed, reproducing test case added
Docs Changes: none
Release Notes: yes
Runtime feature: envoy.reloadable_features.http_response_half_close
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants