-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
if !endStream { | ||
// HEADERS frame block carries a Response-Headers. | ||
isHeader = true | ||
if !endStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it logs cancel and server trailers :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: s/CloseSend/call CloseSend().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL at the recent commit. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nits. I approved previously I think.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
func equalLogEntry(entries ...*binlogpb.GrpcLogEntry) (equal bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Looks like vet proto is failing, but seems unrelated to this change.
This fixes a regression introduced in #5763. Fixes #6524
This change causes us to reliably report the stream status from
ClientStream.Header()
, includingio.EOF
in 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 reportErrNoHeaders
from 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.Header
from returning an error for trailers-only RPC responses, and as a side-effect, prevented retry of the RPC.