Skip to content
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

Setting a deadline longer than 49 days,17 hours fails silently #751

Closed
shshzi opened this issue Feb 3, 2020 · 5 comments · Fixed by #762
Closed

Setting a deadline longer than 49 days,17 hours fails silently #751

shshzi opened this issue Feb 3, 2020 · 5 comments · Fixed by #762
Labels
bug Something isn't working
Milestone

Comments

@shshzi
Copy link

shshzi commented Feb 3, 2020

What version of gRPC and what language are you using?

master branch, 2.26

What operating system (Linux, Windows,...) and version?

Windows

What runtime / compiler are you using (e.g. .NET Core SDK version dotnet --info)

.net core 3.0

What did you do?

Setting a deadline to 50 days fails.
Easiest way to reproduce is to hijack a test case (for example: UnaryCall_Http1Response_ThrowError) and change "AddSeconds(1)" to AddDays(50) / AddDays(49)

What did you expect to see?

Both AddDays(49) and AddDays(50) should have worked

What did you see instead?

Test with deadline of 49 days succeed, 50 days fails silently (timeout)

This is caused because System.Threading.Timer is limited to INT_MAX milliseconds (which is roughly 49 days, 17 hours, 2 minutes and some).
InitializeCall() in GrpcCall.cs fails in line 596 with:
System.ArgumentOutOfRangeException: 'Time-out interval must be less than 2^32-2. (Parameter 'dueTime')'

RunCall() is an async function, but its output is ignored (The exception is never handled gracefully)

The end result is that the Grpc call is never started, and no exception is generated to user.

@shshzi shshzi added the bug Something isn't working label Feb 3, 2020
@JunTaoLuo
Copy link
Contributor

We should definitely not be failing silently here. We will need to fix that.

If we want to support deadlines longer than 49 days, which is rather long for a gRPC call, we'll need to modify our use of System.Threading.Timer.

@JunTaoLuo JunTaoLuo added this to the 5.0 Preview 1 milestone Feb 3, 2020
@JamesNK
Copy link
Member

JamesNK commented Feb 3, 2020

Probably should log that deadline is too long and not start a timer.

Should also research what grpccore does

@shshzi
Copy link
Author

shshzi commented Feb 3, 2020

One easy solution would be to replace _timeout (of type TimeSpan) with an absolute DateTime. Then, set a timer to the difference between DateTime.Now and the deadline, with a maximum value of 49 days, and then verify if the deadline has passed in the DeadlineExceeded callback. If not, re-set the timer.
If you agree with this approach, I'm happy to submit a PR.

@JamesNK
Copy link
Member

JamesNK commented Feb 4, 2020

Maybe. That sounds like a possible solution but I'm wary of complicating things for an extreme edge-case. A deadline that long is abusing the original indent of the feature.

Why do you want such a long deadline?

@shshzi
Copy link
Author

shshzi commented Feb 4, 2020

In my use case, clients are maintaining a connection to the server to let it know they are still connected, and to allow it to transmit back messages with instructions for work items that clients need to perform.

I've actually discovered the bug when I mis-converted another value into a DateTime object that isn't equal to DateTime.MaxValue (which has specific handling). I don't specifically need deadlines that are longer than 50 days (I'm using DateTime.MaxValue), but the lack of error should be fixed.

Another solution would be to disallow it completely, and throw an exception at the API level.
What questions do we need to answer to make a decision?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants