client: fix ClientStream.Header() behavior #6557
Conversation
This change causes us to reliably report the stream status from ClientStream.Header, including io.EOF in case the stream ended successfully with a trailers-only response. To do this required moving the binary logging of trailers into clientStream.finish. By doing this, retry is also fixed in cases where an error is encountered on the stream before calling Header(). Previously, if Header() was called first, we would report ErrNoHeaders from the transport which would be converted into a non-error by clientStream.Header, which would then commit the RPC and prevent retries. A test case was added to cover this scenario.
| if !endStream { | ||
| // HEADERS frame block carries a Response-Headers. | ||
| isHeader = true | ||
| if !endStream { |
There was a problem hiding this comment.
Optional (use some/all/none): mention this is headers flow, and that for trailers flow, header chan gets closed in closeStream() which writes status before unblocking the header chan, letting client stream read the status after it's block on header chan (in csAttempt).
| if atomic.CompareAndSwapUint32(&s.headerChanClosed, 0, 1) { | ||
| s.headerValid = true | ||
| if !endStream { | ||
| // HEADERS frame block carries a Response-Headers. |
There was a problem hiding this comment.
Optional: keep this comment?
stream.go
Outdated
| cs.mu.Unlock() | ||
| // For binary logging. only log cancel in finish (could be caused by RPC ctx | ||
| // canceled or ClientConn closed). Trailer will be logged in RecvMsg. | ||
| // canceled or ClientConn closed). | ||
| // | ||
| // Only one of cancel or trailer needs to be logged. In the cases where | ||
| // users don't call RecvMsg, users must have already canceled the RPC. |
There was a problem hiding this comment.
Optional: delete this comment, no longer applies since we don't log both events in different places: "In the cases where users don't call RecvMsg, users must have already canceled the RPC."
stream.go
Outdated
| @@ -1002,18 +985,33 @@ func (cs *clientStream) finish(err error) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| cs.mu.Unlock() | |||
| // For binary logging. only log cancel in finish (could be caused by RPC ctx | |||
There was a problem hiding this comment.
Now it logs cancel and server trailers :).
There was a problem hiding this comment.
Hmm, I think the logic here is a bit wrong, since the server could return a Canceled status with trailers which is distinct from the client initiating cancelation. I'll have to work on this and add a test since I think it worked properly before.
test/end2end_test.go
Outdated
| } | ||
| return status.Errorf(codes.Unknown, "expected client to CloseSend") |
There was a problem hiding this comment.
Optional: s/CloseSend/call CloseSend().
| if last.Type != binlogpb.GrpcLogEntry_EVENT_TYPE_SERVER_TRAILER || | ||
| last.GetTrailer().GetStatusCode() != uint32(codes.Canceled) || | ||
| last.GetTrailer().GetStatusMessage() != statusMsgWant || | ||
| len(last.GetTrailer().GetMetadata().GetEntry()) != 1 || | ||
| last.GetTrailer().GetMetadata().GetEntry()[0].GetKey() != "key" || | ||
| string(last.GetTrailer().GetMetadata().GetEntry()[0].GetValue()) != "value" { |
There was a problem hiding this comment.
Just a thought, could you use the eventual proto.Equal call in this helper as other parts of the test:
grpc-go/binarylog/binarylog_end2end_test.go
Line 843 in 11a7cd2
There was a problem hiding this comment.
The purpose of this test is to check the trailer log entry. I don't want to have to build & validate everything just for that, which equalLogEntry requires. I think this is fine and I'm not a big fan of the style of tests in this file that have a lot of innate knowledge about different parts of the code spread throughout.
This fixes a regression introduced in #5763. Fixes #6524
This change causes us to reliably report the stream status from
ClientStream.Header(), includingio.EOFin case the stream ended successfully with a trailers-only response. To do this required moving the binary logging of trailers intoclientStream.finish().By doing this, retry is also fixed in cases where an error is encountered on the stream before calling
Header(). Previously, ifHeader()was called first, we would reportErrNoHeadersfrom the transport which would be converted into a non-error byclientStream.Header(), which would then commit the RPC and prevent retries. A test case was added to cover this scenario.RELEASE NOTES:
ClientStream.Headerfrom returning an error for trailers-only RPC responses, and as a side-effect, prevented retry of the RPC.