grpc-go
grpc-go copied to clipboard
grpc
transport: send trailers-only response if no messages are sent, even after SetHeader
What version of gRPC are you using?
Master / v1.24.0
What did you do?
For grpc-go code:
https://github.com/grpc/grpc-go/blob/4ec516e58900fb35115553b525e0fb7eb29fd6e3/internal/transport/http2_server.go#L830-L834
When gRPC server impl code return error and using grpc.SetHeader(ctx,md) combination. The separated header frame cause istio/envoy retry strategy not work.
Why using separated header?
Example of server impl code:
func (s *ExperimentalService) Ping(ctx context.Context, req *hack.Empty) (*hack.ExperimentalPayload, error) {
grpc.SetHeader(ctx, metadata.Paris("x-request-id", "mock-uuid"))
// rand gen gRPC status 14 Unavailable
if rand.Intn(5) == 3 {
return nil, status.Error(14, "")
}
return &hack.ExperimentalPayload{}, nil
}
What did you expect to see?
What did you see instead?
@elvizlai can you be a little more specific? Why does istio/envoy retry strategy not work?
gRPC retry does not work if the server sends headers. This is by design. Please see this section of the gRFC that implements it: https://github.com/grpc/proposal/blob/master/A6-client-retries.md#when-retries-are-valid
@dfawley Sorry for the late reply.
As you said that this is design purpose: gRPC retry does not work if the server sends headers.
But this it is looks miserable when using interceptors.
May be I should check if next middleware returns error, then using Trailer instead of Header?
Because when using grpc.SetHeader, even if error occurs, the first frame(contains :status) will be send.
if len(s.header) > 0 { // Send a separate header frame.
if err := t.writeHeaderLocked(s); err != nil {
s.hdrMu.Unlock()
return err
}
the grpc-status is in the following(separated) frame.
I tried to hack the grpc-go, using append, the retry strategy works:
headerFields = append(headerFields, hpack.HeaderField{Name: ":status", Value: "200"})
headerFields = append(headerFields, hpack.HeaderField{Name: "content-type", Value: contentType(s.contentSubtype)})
headerFields = appendHeaderFieldsFromMD(headerFields, s.header)
instead of
if err := t.writeHeaderLocked(s); err != nil {
s.hdrMu.Unlock()
return err
}
May be I should check if next middleware returns error, then using Trailer instead of Header?
I believe the gRPC library should be (but isn't) automatically doing what you described. Also from the same gRFC:
gRPC servers should delay the Response-Headers until the first response message or until the application code chooses to send headers. If the application code closes the stream with an error before sending headers or any response messages, gRPC servers should send the error in Trailers-Only.
I'm not sure about your specific patch, but if you'd like to send a PR to implement the behavior, please feel free to do so.
Hi @easwars. I'd like to work on this. Is there anything I should know (except what's already here) before I start the actual work?
If the server application sets headers explicitly by calling the SetHeader API, then they should get sent out as in a separate headers frame. They should not be combined into a trailers-only response when the RPC is terminated with an error before any messages are sent on it. See: https://github.com/grpc/grpc-go/pull/5909#issuecomment-1386255311
So, our current behavior is what we want, and hence we are closing this issue.
This is my fault, and I think I misinterpreted the following, actually:
gRPC servers should delay the Response-Headers until the first response message or until the application code chooses to send headers. If the application code closes the stream with an error before sending headers or any response messages, gRPC servers should send the error in Trailers-Only.
Specifically the part: "until the application code chooses to send headers". In C++ and Java, there is no API to "set" headers, only to send them. So that distinction wasn't important to them. I, however, took this to mean that if headers were set by the application, but not sent, that we should send them in a trailers-only response instead.
However, this would be a breaking API change. If we shift headers into trailers, then the application won't know how to receive them. And it will change depending on whether an error occurs.
Instead, what this sentence in the gRFC actually means is that gRPC should not send its default headers frame until necessary, and if it wasn't necessary when the stream ends, then send a trailers-only response.
So sorry for the confusion.