Skip to content

Add Task.Wait(TimeSpan, CancellationToken) api #30067

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

Closed
NinoFloris opened this issue Jun 27, 2019 · 4 comments
Closed

Add Task.Wait(TimeSpan, CancellationToken) api #30067

NinoFloris opened this issue Jun 27, 2019 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks
Milestone

Comments

@NinoFloris
Copy link
Contributor

The current api might tempt people to directly cast TimeSpan.TotalMilliseconds to int (guilty) without realizing that there may be super rare cases where this truncates high ranges.

There is a Task.Wait(TimeSpan) overload to pair with the Task.Wait(int) method so it would only be good if the same would be true for Task.Wait(int, CancellationToken).

This came up in a review of an Npgsql PR npgsql/npgsql#2508 (comment)

@Clockwork-Muse
Copy link
Contributor

truncates high ranges

Do you mean "overflows high ranges"? Since using checked here would cause it to throw? (Yet another situation where checked-by-default is probably the correct setting).

Actually, I think I'm more curious what somebody's doing that would result in them having a wait of (2 ^31 - 1) / 1000 / 60 / 60 /24 = 24 days....

@BhaaLseN
Copy link

BhaaLseN commented Jul 3, 2019

Sounds like a good idea. I was going over some code with a coworker today and they looked into using Task.WaitAll/Task.WaitAny with a non-small timeout value that would read a lot nicer if it could simply be TimeSpan.FromXxxxx(value) instead of a magic number that need not be immediately obvious to the reader. Just like Task.Wait, the overloads without the CancellationToken have an overload that accepts a TimeSpan.

Also, in the end it became (int)TimeSpan.FromMinutes(10).TotalMilliseconds, as most would probably expect. Longer than just 600000, but you don't have to juggle the correct number of zeroes in your head just to figure out how much (or little) it actually is.

@GSPP
Copy link

GSPP commented Jul 22, 2019

It is a design bug of .NET 1.0 that timeouts sometimes are expressed as int milliseconds. This should not exist at all. Now I guess it needs to be carried forward to new APIs for consistency.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@stephentoub
Copy link
Member

Closing as one specific case of #14336. It'd be good to address this holistically if it's going to be addressed.

@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Apr 6, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks
Projects
None yet
Development

No branches or pull requests

7 participants