search

connectrpc / connect-go

The Go implementation of Connect: Protobuf RPC that works.
https://connectrpc.com
Apache License 2.0
2.76k stars 90 forks source link

Why not return a Trailers-only response other than gRPC-Web? #724

Closed castaneai closed 2 months ago

castaneai commented 2 months ago

Describe the bug

Connect's gRPC Protocol returns a Trailers-only response only when gRPC-Web.

https://github.com/connectrpc/connect-go/blob/59f77e784908ddab78a262ed44bdd0515733d487/protocol_grpc.go#L534-L535

However, according to the specification, Trailers-only reponse would work for gRPC as well as gRPC-Web.

https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses

For example, grpc-go also returns Trailers-only reponse. I posted this question because some libraries that rely on Trailers-only behaved differently in Connect-Go.

Thanks!

To Reproduce

For example, gRPC retries by grpc-dotnet do not work well with Connect.

https://learn.microsoft.com/en-us/aspnet/core/grpc/retries?view=aspnetcore-8.0

Environment (please complete the following information):

jhump commented 2 months ago

@castaneai, this library is built on top of net/http, which does not provide enough control over the HTTP/2 response stream to emit a correct "trailers-only" response for gRPC.

In particular, a "trailers-only" response for the gRPC protocol must emit the response headers and set the "end stream" flag on the HTTP/2 header frame. However, with net/http, this isn't possible. It emits the response headers but does not set the "end stream" flag on the frames. And then, upon realizing there is no response body, it sends an empty data frame with the "end stream" flag set. This is an invalid "trailers-only" response and will be interpreted as an erroneous response by some gRPC client implementations. So to maximize interop (and conform to gRPC's protocol spec), this library never attempts to send responses that way.

It may be worth filing a bug to net/http to have some affordance for this: like if the handler returns without ever calling Write or WriteHeader on the http.ResponseWriter, then it has yet to write the header frames and also knows there will be no data, so it could set the "end stream" flag when it sends the headers. But that is definitely not implemented today, and it's questionable if the Go team would be okay with something that HTTP/2-specific being added to the spec for http.ResponseWriter.

castaneai commented 2 months ago

@jhump Thank you! Is it correct that the client library side should handle both responses?

jhump commented 2 months ago

Is it correct that the client library side should handle both responses?

@castaneai, yes. Other servers do use trailers-only responses in various scenarios, so clients must handle them, in addition to normal responses (with separate headers and trailers, and possibly non-empty body).

Again due to limitations in net/http, the client implementation in this module is lenient with regards to "trailers-only" responses in gRPC. Since it cannot tell if the response was a single headers HTTP/2 frame with the "end stream" bit set (net/http doesn't provide this level of information about the on-the-wire form of the request), it allows any response that has an empty body and no trailers to be interpreted as a trailers-only response.

castaneai commented 2 months ago

@jhump Thank you! I will try to fix the client library side.