transport: Reject non-positive timeout values in server #8290
Conversation
Negative timeouts should be forbidden. The previous implementation called ParseInt and allowed negative timeouts. Change it to call ParseUint to reject these values. This is also slightly faster, since ParseInt eventually calles ParseUint. Extend the test to cover more cases.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8290 +/- ##
==========================================
+ Coverage 82.15% 82.27% +0.11%
==========================================
Files 419 419
Lines 41904 41949 +45
==========================================
+ Hits 34426 34513 +87
+ Misses 6013 5983 -30
+ Partials 1465 1453 -12
🚀 New features to boost your workflow:
|
internal/transport/http_util_test.go
Outdated
| {"2562047H", time.Hour * 2562047, nil}, | ||
| {"2562048H", time.Duration(math.MaxInt64), nil}, | ||
| {"99999999H", time.Duration(math.MaxInt64), nil}, | ||
| {"-1S", 0, fmt.Errorf("strconv.ParseUint: parsing %q: invalid syntax", "-1")}, |
There was a problem hiding this comment.
This error string is not part of gRPC and can cause the test to break when run with a different version of golang. You can instead use cmp.Equal() with cmpopts.EquateErrors(). You can use cmpopts.AnyError as the expected value.
https://google.github.io/styleguide/go/decisions#test-error-semantics
There was a problem hiding this comment.
Good suggestion! However, since this function is returning fmt.Errorf, none of the expected errors can be compared, since I changed the error check to just be for "has an error". We could change the function to return some other typed error instead, like maybe a status error? I think the test checking for existence of an error is probably okay.
arjan-bal
added
Type: Bug
Area: Server
* forbid timeout == 0 * change test to check for err != nil and not compare messages
|
Thanks for the suggestion! This caused me to add a check for |
arjan-bal
changed the title
|
I've been investigating grpc overhead in services at Datadog that do a lot of requests/second, which means they spend a lot of time in grpc-go. I noticed this function and wondered why So ... Maybe not worth our time discussing and reviewing the change, but I thought it wasn't a total waste. |
No, that's fine. I was just wondering if there was some other significant event at play here, like one of our grpc libraries was actually sending negative timeouts. Thanks for the change! |
|
In b/427338711 at Google we're seeing a regression caused by this and grpc-java sending timeouts of This makes me believe that no gRPC implementation was actually requiring the timeout to be positive, only non-negative. Makes me wonder if we should change the spec. |
|
Wow oops thanks for the update and sorry for breaking stuff :(. A request with a 0 timeout would previously fail with deadline exceeded, and now I think this error causes the entire http2 connection to be closed IIRC, is that right? (I want to take a quick look to see if we might be running into this error at Datadog ...) |
PROTOCOL-HTTP2.md specifies "TimeoutValue → {positive integer as ASCII
string of at most 8 digits}". Zero is not positive, so it should be
avoided. So make sure timeouts are at least 1 nanosecond instead of 0
nanoseconds.
grpc-go recently began disallowing zero timeouts in
grpc/grpc-go#8290 which caused a regression as
grpc-java can generate such timeouts. Apparently no gRPC implementation
had previously been checking for zero timeouts.
Instead of changing the max(0) to max(1) everywhere, just move the max
handling into TimeoutMarshaller, since every caller of TIMEOUT_KEY was
doing the same max() handling.
b/427338711
PROTOCOL-HTTP2.md specifies "TimeoutValue → {positive integer as ASCII
string of at most 8 digits}". Zero is not positive, so it should be
avoided. So make sure timeouts are at least 1 nanosecond instead of 0
nanoseconds.
grpc-go recently began disallowing zero timeouts in
grpc/grpc-go#8290 which caused a regression as
grpc-java can generate such timeouts. Apparently no gRPC implementation
had previously been checking for zero timeouts.
Instead of changing the max(0) to max(1) everywhere, just move the max
handling into TimeoutMarshaller, since every caller of TIMEOUT_KEY was
doing the same max() handling.
b/427338711
PROTOCOL-HTTP2.md specifies "TimeoutValue → {positive integer as ASCII
string of at most 8 digits}". Zero is not positive, so it should be
avoided. So make sure timeouts are at least 1 nanosecond instead of 0
nanoseconds.
grpc-go recently began disallowing zero timeouts in
grpc/grpc-go#8290 which caused a regression as
grpc-java can generate such timeouts. Apparently no gRPC implementation
had previously been checking for zero timeouts.
Instead of changing the max(0) to max(1) everywhere, just move the max
handling into TimeoutMarshaller, since every caller of TIMEOUT_KEY was
doing the same max() handling.
Before fd8fd51 (in 2016!), grpc-java actually behaved correctly, as it
failed RPCs with timeouts "<= 0". The commit changed the handling to the
max(0) handling we see now.
b/427338711
PROTOCOL-HTTP2.md specifies "TimeoutValue → {positive integer as ASCII
string of at most 8 digits}". Zero is not positive, so it should be
avoided. So make sure timeouts are at least 1 nanosecond instead of 0
nanoseconds.
grpc-go recently began disallowing zero timeouts in
grpc/grpc-go#8290 which caused a regression as
grpc-java can generate such timeouts. Apparently no gRPC implementation
had previously been checking for zero timeouts.
Instead of changing the max(0) to max(1) everywhere, just move the max
handling into TimeoutMarshaller, since every caller of TIMEOUT_KEY was
doing the same max() handling.
Before fd8fd51 (in 2016!), grpc-java actually behaved correctly, as it
failed RPCs with timeouts "<= 0". The commit changed the handling to the
max(0) handling we see now.
b/427338711
This should not cause connection closure. Just an RPC failing with a different status. I sent #8439 to allow a 0s timeout again. |
Negative timeouts should be forbidden. The previous implementation called ParseInt and allowed negative timeouts. Change it to call ParseUint to reject these values. This is also slightly faster, since ParseInt eventually calles ParseUint. Extend the test to cover more cases.
RELEASE NOTES:
grpc-timeoutheader values are now rejected by servers. This is consistent with the gRPC protocol spec.