transport: return http-status when grpc-status is missing#4606
transport: return http-status when grpc-status is missing#4606JNProtzman wants to merge 6 commits intogrpc:masterfrom
Conversation
| return | ||
| } | ||
| rawStatusCode = codes.Code(uint32(code)) | ||
| rsc := codes.Code(uint32(code)) |
There was a problem hiding this comment.
Shouldn't ParseInt above be ParseUint?
And then we need a bounds check on this as well.
There was a problem hiding this comment.
I adjusted this to ParseUint. What kind of bounds check should I do?
There was a problem hiding this comment.
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.
OK great. I think this is ready for re-review then!
|
|
||
| if httpStatusCode != nil { | ||
| if rawStatusCode != nil { | ||
| code = *rawStatusCode |
There was a problem hiding this comment.
So now here is the big question: what if rawStatusCode is OK?
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.
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.
I don't think this part of the change is right. A missing content-type error should continue to take priority over a grpc-status of OK (though that's admittedly a gray area and also unlikely to happen). We should still use that precedence, but only change it so we take grpc-status over http-status if everything higher priority is okay.
There was a problem hiding this comment.
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.
internal/transport/http2_client.go
Outdated
| if rawStatusCode == nil { | ||
| rsc := codes.Unknown | ||
| rawStatusCode = &rsc | ||
| } | ||
| statusGen = status.New(*rawStatusCode, grpcMessage) |
There was a problem hiding this comment.
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)
}
|
Friendly ping on this PR as well - my comment 7d ago is hidden in a thread above, so I also wanted to make sure it's more obvious that this PR isn't blocked on me. |
|
I know the tests are failing but it seems like those might be unrelated. @dfawley, I think this is ready for your review again. |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
| 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.
If there is a grpc-status, then there should be a grpc-message to go along with it, or we should use the empty string like we do normally -- we should not be using the generated "unexpected HTTP status code" error message. At least, that's what it seems like it should be doing, though it's not mentioned in our docs. Does that seem right to you, @ejona86 / @markdroth?
Maybe set this error code to a different number and change the wantStatus code, so it is clear that's the source of the error, not the 504->Unavailable conversion?
There was a problem hiding this comment.
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.
I'd leave the header missing
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 nil.)
There was a problem hiding this comment.
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.
Ping @JNProtzman, I think this is the last issue: to not use the generated http error message along with the grpc-status header value in the status returned to the user.
There was a problem hiding this comment.
@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!
dfawley
added
Type: Bug
Status: Requires Reporter Clarification
and removed
stale
labels
|
Ping @JNProtzman - hope all is well. Have you returned, and are you able to pick this PR up again? |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
4 participants
This (hopefully) addresses @dfawley's comment.
RELEASE NOTES: n/a