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
transport: return http-status when grpc-status is missing #4606
Changes from 1 commit
3e948ab
008beeb
fbb727b
64e41a7
ae2fd9c
e9a576e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1290,7 +1290,7 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { | |
statusGen *status.Status | ||
httpStatusCode *int | ||
httpStatusErr string | ||
rawStatusCode = codes.Unknown | ||
rawStatusCode *codes.Code | ||
// headerError is set if an error is encountered while parsing the headers | ||
headerError string | ||
) | ||
|
@@ -1318,7 +1318,8 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { | |
t.closeStream(s, se.Err(), true, http2.ErrCodeProtocol, se, nil, endStream) | ||
return | ||
} | ||
rawStatusCode = codes.Code(uint32(code)) | ||
rsc := codes.Code(uint32(code)) | ||
rawStatusCode = &rsc | ||
case "grpc-message": | ||
grpcMessage = decodeGrpcMessage(hf.Value) | ||
case "grpc-status-details-bin": | ||
|
@@ -1366,7 +1367,9 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { | |
if !isGRPC || httpStatusErr != "" { | ||
var code = codes.Internal // when header does not include HTTP status, return INTERNAL | ||
|
||
if httpStatusCode != nil { | ||
if rawStatusCode != nil { | ||
code = *rawStatusCode | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So now here is the big question: what if If you read the spec literally, we should ignore the HTTP status in this case. This appears to be what the other languages do as well, so I suppose we should do that, even though to me it appears to be covering up errors in the system. (@ejona86 @markdroth FYI) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you mentioned - this results in potentially covering up an error. Here's an example in tests in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this part of the change is right. A missing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took a stab at this. I'm not 100% sure it's what you meant but I think it is. If we receive a grpc-status but have encountered another error, we'll take the http-status over the grpc-status. Otherwise, we'll default to the http-status. |
||
} else if httpStatusCode != nil { | ||
var ok bool | ||
code, ok = HTTPStatusConvTab[*httpStatusCode] | ||
if !ok { | ||
|
@@ -1438,7 +1441,11 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) { | |
} | ||
|
||
if statusGen == nil { | ||
statusGen = status.New(rawStatusCode, grpcMessage) | ||
if rawStatusCode == nil { | ||
rsc := codes.Unknown | ||
rawStatusCode = &rsc | ||
} | ||
statusGen = status.New(*rawStatusCode, grpcMessage) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's do this the other way around to avoid an allocation (even if it hopefully doesn't escape the stack): if statusGen == nil {
rsc := codes.Unknown
if rawStatusCode != nil {
rsc = *rawStatusCode
}
statusGen = status.New(rsc, grpcMessage)
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
} | ||
|
||
// if client received END_STREAM from server while stream was still active, send RST_STREAM | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2031,7 +2031,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { | |
}, | ||
}, | ||
wantStatus: status.New( | ||
codes.Unknown, | ||
codes.OK, | ||
"malformed header: missing HTTP content-type", | ||
), | ||
}, | ||
|
@@ -2090,7 +2090,7 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) { | |
metaHeaderFrame: &http2.MetaHeadersFrame{ | ||
Fields: []hpack.HeaderField{ | ||
{Name: "content-type", Value: "application/grpc"}, | ||
{Name: "grpc-status", Value: "0"}, | ||
{Name: "grpc-status", Value: "14"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is a Maybe set this error code to a different number and change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say a grpc-message should go along with it from a "we don't hate ourselves" perspective. If there isn't a message, I wouldn't use an empty string; I'd leave the header missing. I wouldn't suggest a useless message like "UNAVAILABLE"; again, I'd prefer just not having it. But that's a bit neither here nor there. In this case, there is a grpc-status, so you should use that and the grpc-message if it isn't present. I don't think this test is correct: the status shouldn't have a status description. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is client-side. If the header is missing, the client would use "" as the message string presented to the user. (We have to give them some string, since it's returned as a string value; we can't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, then yeah, empty string seems appropriate for your API. Java's API lets the description (which we call grpc-message) be null. Makes sense that you wouldn't have such a distinction in Go. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ping @JNProtzman, I think this is the last issue: to not use the generated http error message along with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dfawley sorry for the delay here. I'm currently in Iceland and have plans to travel through Europe until Nov. 1st. I won't be able to pick this up until then... sorry! |
||
{Name: ":status", Value: "504"}, | ||
}, | ||
}, | ||
|
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.
Shouldn't
ParseInt
above beParseUint
?And then we need a bounds check on this as well.
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.
I adjusted this to
ParseUint
. What kind of bounds check should I do?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, nevermind the bounds checks. If we were to add a new status code, I believe we'd want to allow it to be propagated by a gRPC proxy without requiring recompilation of the proxy. So any unsigned value is fine here.
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.
OK great. I think this is ready for re-review then!