When using time based cancellations, there are a few potential problems.
CancellationTokenSource requires starting a timer.CancellationTokenSource is disposed, the timeout will never fire and the `CancellationToken will not be cancelled. This isn't clear in the documentation. CancellationTokenSource must be tracked in some way so it can be properly disposed. Otherwise a memory leak can occur. For one-offs this is not a big deal. But as we increasingly use CancellationToken, it can become an significant expense in terms of memory and system resources.
An alternative would be to allow the CancellationToken itself to have an optional DateTime field that can be checked. While it would make the CancellationToken slightly more expensive in terms of memory, it would eliminate the need for an expensive timer.
What I think would make more sense would be to make CancellationToken struct itself smarter about timeouts. When you create a CancellationTokenSource with a timeout, it should calculate the timestamp of the expiration, and bake that into the token. Then various properties on the CancellationToken, such as IsCancellationRequested, would check the current time against that timestamp and return appropriately if that timestamp is in the past.
CC @stephentoub
Or bake the Timestamp into the CancellationTokenSource (since CancellationToken is just a wrapper on its pointer) and delay the Timer creation until there is a registration?
That sounds like a reasonable compromise.
My only objection is that it means we're still stuck with dealing with the lifetime of the CancellationTokenSource. I'd rather not use it if I don't have to.
I don't understand the proposal. How would registered callbacks work?
delay the Timer creation until there is a registration
That is a reasonable thing to experiment with as an implementation detail. However a) it will increase the size of the CTS, b) from what I've seen it's rare to have a CTS that's never registered with, and c) from what I've seen most CTS's are registered with soon after creation. If you can demonstrate otherwise, it's worth exploring (but it doesn't require an issue to track the hypothetical).
For those of us playing at home, what does it actually mean to "register" a CTS?
cancellationToken.Register(someDelegate)
Ah, I don't think I've seen any code use that. Most of what I deal with just occasionally checks it in a loop.
Would be a safe place to hang the CTS.Dispose call?
Does this extension solve your need?
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#using-a-timeout
Ah, I don't think I've seen ant code use that. Most of what I deal with just occasionally checks it in a loop.
Yup, understood. But it's used in a ton of places in library code, most often in leaf nodes of a call tree where the operation transitions from managed to native. Search corefx and coreclr for Register and UnsafeRegister if you want to see some examples. For example, any operation that directly or indirectly performs asynchronous network operations will involve a registration.
Thank you for the suggestion. Given the conversation, I'm going to close this as not being actionable. As noted, if you see opportunities in the implementation to make it more efficient, we'd welcome a PR, but we don't need an issue tracking that possibility.