Runtime: API Suggestion: System.Threading.Timer.Stop()

[benaadams]

Calling .Change with Timeout.Infinite to stop the timer is unexpected (though makes sense).

It would be better if there was a more easily findable method such as .Stop() which handles this

namespace System.Threading
{
    public class Timer
    {
        public void Stop() => Change(Timeout.Infinite, Timeout.Infinite);
    }
}

[ivdiazsa]

New API Write-Up

Currently, the only way to stop a Threading Timer is by calling Change(Timeout.Infinite, Timeout.Infinite), which is not the most intuitive or readable at first glance workaround approach.

Rationale and Usage

This is certainly nice-to-have, but it is an essential function when working with certain multithreaded applications that should be ready for developers to use, instead of having to look for a less readable workaround. Right now, developers are forced to read through all the documentation until Change() and put the pieces together that calling Change() with Infinite as both arguments would result in the timer stopping, without having to dispose of it and create a new one.

For example:

```C#
var timer = new Timer(...);

/* Work with the timer until you need to stop it. */

timer.Change(Timeout.Infinite, Timeout.Infinite);

/* Resume the timer and set it to 1 second. */
timer.Change(1000, 1000);

With the new API, it would become more understandable and easier to investigate:

```C#
var timer = new Timer(...);

/* Work with the timer until you need to stop it. */

timer.Stop();

/* Resume the timer and set it to 1 second. */
timer.Change(1000, 1000);

An added bonus is that this functionality would be easier to find in the documentation.

Proposed API

C# namespace System.Threading { public sealed class Timer : MarshalByRefObject, IDisposable, IAsyncDisposable { public bool Stop() { return Change(Timeout.Infinite, Timeout.Infinite); } } }

Details

• The function Change() is implemented as a bool. To follow accordingly, Stop() would be implemented as a bool as well.
• It would acquire its highest relevance when working with periodic timers. These are mostly used when only certain parts of the program require to be clocked.

[ivdiazsa]

@kouvel @mangod9 Here is the first draft of the write-up to move on with the new API process. Please take a look so we can polish it and continue with the next steps to officially add Stop() to the Threading.Timer API.

[kouvel]

Could mention that it may throw ObjectDisposedException.

The function Change() is implemented as a bool. To follow accordingly, Stop() would be implemented as a bool as well.

When used to stop the timer, Change seems to always returns true if it does not throw. I don't think it's necessary to return bool from Stop. Not sure why Change returns bool, in any situation where it can't update the timer it would be probably be reasonable to throw instead of returning false in that case too, anyway there appears to be at least one case where it returns false instead of throwing. It can be confusing because if the user doesn't realize that there is a return value that could indicate failure it may not be checked. Exception would be easier to follow and diagnose I think in cases that shouldn't happen normally.

[ivdiazsa]

So we should make Stop() a void instead, and check for the return value of Change() within. If false, then throw an exception, otherwise do nothing as the Timer was stopped successfully. Did I understand your idea correctly? @kouvel

[stephentoub]

I'd be more inclined to introduce functionality here that isn't otherwise attainable. You can achieve the desired behavior cited via timer.Change(-1, -1); as noted. But, that doesn't wait for any queued timer invocations to quiesce. If we're going to introduce new API here, I'd want that to be a part of it; otherwise, it's hard to rely on the idea of the timer being "stopped" if work associated with the timer can actually happen after that.

So, if we want to add API here, my preference would be:
C# public Task StopAsync();
which would not only Change(-1, -1) but also asynchronously wait for all work associated with the timer to complete.

[kouvel]

So we should make Stop() a void instead, and check for the return value of Change() within. If false, then throw an exception, otherwise do nothing as the Timer was stopped successfully.

There shouldn't be a case where Change(-1, -1) would return false, it could be an assertion instead, I currently don't see an exception case for that

I'd be more inclined to introduce functionality here that isn't otherwise attainable. You can achieve the desired behavior cited via timer.Change(-1, -1); as noted. But, that doesn't wait for any queued timer invocations to quiesce.

That would be a good capability to add, but it's also a distinct desire. I don't see any natural link between the two, in the sense it would make sense to add this API without adding the capability that you mentioned, and the capability may involve separate APIs that would achieve it, synchronously or asynchronously.

[stephentoub]

I don't see any natural link between the two, in the sense it would make sense to add this API without adding the capability that you mentioned, and the capability may involve separate APIs that would achieve it, synchronously or asynchronously.

My point is, we already have the capability being requested, it's just uttered as:
```C#
timer.Change(-1, -1);

instead of:
```C#
timer.Stop();

In contrast, there's no way of uttering the behavior I mentioned.

And, although I'm speculating here, I expect most developers would find it surprising that the timer may still fire after calling Stop();. So much so that I'm suggesting it's not actually a good idea to expose that behavior in a new method, that if we were going to add Stop();, it should wait for the work to quiesce as well.

You disagree?

[kouvel]

I expect most developers would find it surprising that the timer may still fire after calling Stop();

Yes that is already a problem with Change(-1, -1) or the more natural Stop(). I assume that people who want to stop timers today would already be dealing with that problem from their side, because of the lack of the capability to wait for scheduled timers to complete. Adding such a capability would probably make it easier for developers for new code. And adding the Stop() API without that capability would also make it easier for developers to discover and write code with more clear intent. I don't at all disagree that the capability would be interesting to add, I just don't think it's necessarily linked to this API addition.

[kouvel]

I would actually like to see a timer capability that does not schedule more than one callback at a time, along with the capability that you mention. I have heard from several folks where that would be useful, like in cases of very frequently ticking timers, but that is again a distinct desire and separate from this issue.

[kouvel]

I agree Stop() could be misinterpreted as StopAndWaitForAnyCurrentCallbacksToComplete() instead of StopSchedulingNewCallbacks(). As you can see I'm pretty terrible with names, but maybe that is only a naming issue? Any suggestions?

[kouvel]

I'd expect that by the time someone would want to stop a timer, that the reason for it, the work that the timer is tracking, would have already been stopped. So even before the timer is stopped, they may get spurious timer ticks and would have to deal with that type of situation. Having a capability to wait for callbacks probably would be useful, I can think of corner cases mostly but I'm struggling to find more general reasons for it. @stephentoub do you have any data or experiences to share about that?

[kouvel]

Also timer tick callbacks are not guaranteed to run in any reasonable amount of time. If such a capability were added, I would expect that it would also involve removing any pending callbacks from the relevant queues such that it actually only waits for currently-running callbacks to complete. That is more difficult to achieve and I'm not sure it's worth it. Without the removals of pending callbacks, it feels like an incomplete implementation of the capability.

[davidfowl]

I don't think we should add anything else to the timer APIs. As designed today they have a ton of warts and gotchas. I would prefer if it we focused our energy on a newer pattern https://github.com/dotnet/runtime/issues/31525.

PS: It's strange to have Stop and not Start

[stephentoub]

Yes that is already a problem with Change(-1, -1)

Yes. I'm suggesting a) if that's the behavior you want, there already exists an API for it, and b) we shouldn't propagate that behavior forward into a new API with the nice name.

or the more natural Stop()

Disagree :) Why is it more natural that Stop mean "stop firing at some point in the future but feel free to continue invoking callbacks related to this timer well after Stop returns to me" rather than "stop firing now such that I'm guaranteed no callbacks associated with this timer will be invoked after Stop has returned"?

Adding such a capability would probably make it easier for developers for new code.

Yes

And adding the Stop() API without that capability would also make it easier for developers to discover and write code with more clear intent

This is where I disagree. Exposing a "Stop" certainly makes it more discoverable, but having a "Stop" that doesn't actually stop is IMO a problem. At least with Change there's an argument to be made that you're changing the configuration but no promises are being made about how timely it is. With stop, it's right there in the name.

I would actually like to see a timer capability that does not schedule more than one callback at a time, along with the capability that you mention. I have heard from several folks where that would be useful, like in cases of very frequently ticking timers, but that is again a distinct desire and separate from this issue.

There's an issue for that:
https://github.com/dotnet/runtime/issues/31525

If you think this should be addressed in concert with that, might I suggest you look at all of the open Timer-related API proposals and come up with a coherent design for new APIs that factors in all of them? Otherwise designing these things piecemeal we run the risk of designing nice solutions for individual problems that don't play well together or as well as they otherwise could have.

I agree Stop() could be misinterpreted as StopAndWaitForAnyCurrentCallbacksToComplete() instead of StopSchedulingNewCallbacks(). As you can see I'm pretty terrible with names, but maybe that is only a naming issue? Any suggestions?

I think Stop will be, even should be, interpreted as StopAndWaitForAnyCurrentCallbacksToComplete :smile: Hence why I was suggesting it be exposed as StopAsync. The Change(-1, -1) that already exists seems like a reasonable way to achieve that behavior; I don't know why we'd need to give it a new name.

[benaadams]

That is more difficult to achieve and I'm not sure it's worth it. Without the removals of pending callbacks, it feels like an incomplete implementation of the capability.

Add a _disabled flag into the timer; then it doesn't necessarily have to deschedule to stop the execution of the callback, just flip the flag? (or check its scheduling before execution; if its off, don't do it)

[stephentoub]

Add a _disabled flag into the timer; then it doesn't necessarily have to deschedule to stop the execution of the callback, just flip the flag?

We already do that. It's just called _canceled rather than _disabled:
https://github.com/dotnet/runtime/blob/61c658183231100a5836e833c86446ff51a4654b/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs#L426

[benaadams]

We already do that. It's just called _canceled rather than _disabled:

That causes it to throw if you reschedule though? i.e. its Dispose and one way

[stephentoub]

That causes it to throw if you reschedule though? i.e. its Dispose and one way

Yes, with the current implementation where a StopAsync doesn't exist. My point was this is both feasible and already implemented; it would just need to be tweaked if such an additional API were added.

[kouvel]

My point is that it's not always a bad thing to add an API to do something that can be achieved with other APIs, there are many examples of that in the current API set and this is not much different. Like I said I agree the name of the method may confuse people but I wonder how much it matters, as I said in https://github.com/dotnet/runtime/issues/34144#issuecomment-628211435, and the questions I asked there that I'm still curious about.

I think Stop will be, even should be, interpreted as

What are your reasons for why it should or would be interpreted in the way you state, given my comment linked above? Given that the timer never offered that functionality I would actually be surprised if anyone thought that's what this API would do, especially given my linked comment above.

Perhaps there's a lot to discuss about what the timer should be, which may also implicate recreating the timer with whole new APIs, or small tweaks to APIs here and there, but the purpose of this API (ignore the name for now, or suggest a different name that would be appropriate, as per https://github.com/dotnet/runtime/issues/34144#issuecomment-628206070) is to make existing functionality more discoverable. That's not a bad thing, and it shouldn't be confused with solving problems it is currently not intended to solve.

[stephentoub]

What are your reasons for why it should or would be interpreted in the way you state, given my comment linked above?

I think the current behavior is unintuitive, especially if given the more prominent name of "Stop", and that most developers who have used Timer.Change(-1, -1) would be surprised at this subtle behavior it has, especially since it can result in nasty race conditions. That's my intuition; I could of course be wrong.

[davidfowl]

I agree with @stephentoub this change doesn't move the needle enough.

[kouvel]

if given the more prominent name of "Stop", and that most developers who have used Timer.Change(-1, -1) would be surprised at this subtle behavior it has, especially since it can result in nasty race conditions

Let's call the API DontScheduleAnyMoreTimerCallbacks(), which happens to do what Change(-1, -1) does, because I think we're a bit too stuck on the name. Do you still have an objection to add that functionality despite the awful name?

[kouvel]

this change doesn't move the needle enough

This change is not intended to move that needle

[stephentoub]

I think we're a bit too stuck on the name

But if the thing being added is just the existing behavior, the name is literally the only reason to add this. The bar to beat is Change(-1, -1), which already exists.

[kouvel]

the name is literally the only reason to add this

Yes, however there are more reasons as stated in the proposal, discoverability and saving devs time from reading through the docs to figure out how to do it with the current API which is not at all obvious. Are you suggesting that that is not a good enough reason to add a new API?

[stephentoub]

Are you suggesting that that is not a good enough reason to add a new API?

In this case, yes. What would it be called that is significantly better than what it's currently called, wouldn't require reading docs to understand the subtleties of this behavior, that wouldn't be confused with the thing that actually does wait for everything to quiesce, etc.?

[benaadams]

for everything to quiesce, etc.?

Just to be clear; are we talking about a callback currently in progress, or something scheduled starting and running after .Stop() has returned?

[stephentoub]

That the callback associated with the timer will not be running or again invoked after Stop, such that the code after Stop could reliably use or Dispose of the same resources used by the callback. Like CancellationTokenRegistration.Dispose.

[kouvel]

that wouldn't be confused with the thing that actually does wait for everything to quiesce

I'd expect that by the time someone would want to stop a timer, that the reason for it, the work that the timer is tracking, would have already been stopped. So even before the timer is stopped, they may get spurious timer ticks and would have to deal with that type of situation. It's possible for the timer to be stopped first, but due to the lack of guarantee it wouldn't matter currently. What would be a reason for someone to expect that Stop() should stop any new callbacks from running and wait for already-running callbacks to complete? In the above case perhaps it could allow them to reliably stop the timer before stopping other activities the timer is tracking, in order to prevent callbacks from occurring at unexpected times, but that's about it. That seems like a long-shot to me. What is the value of that proposal?

[kouvel]

What would it be called that is significantly better than what it's currently called, wouldn't require reading docs to understand the subtleties of this behavior

That is the same question I asked of you. But first I'd like to narrow it down to a naming issue, I'm not sure we're there yet.

[stephentoub]

That is the same question I asked of you.

My question was more rhetorical :) I can't think of a name that makes it worth adding.

[stephentoub]

What is the value of that proposal?

There are only two groups of scenarios I can think of where I've wanted stop/start behavior:

1. A recurring timer that should only be running while something else is active, pausing it to save resources while it's not and then unpausing it again when it is. For example, a timer used to cull stale connections from a connection pool, where the timer can be paused when there aren't any. If it happens to be invoked after pausing it, it's not great but it's not the end of the world.
2. Wanting to use a timer as part of an operation, and then reuse it as part of a completely different one. For example, pooling a Timer either directly or indirectly as part of something else that's been pooled. It's dangerous if the Timer for operation N can fire and end up impacting operation N + 1.

(1) is fully addressed by Change(-1, -1).

(2) doesn't have any API support in Timer. To me, it's exactly what a Stop{Async} should enable.

[kouvel]

(2) can't be done easily with the current timer APIs, to me it would be along the lines of adding APIs like StopAndWait[Async](). I'm not sure if it would be worth pooling timers - they really shouldn't be created frequently enough to warrant pooling them, I don't see the significance of that use case but I'd be open to data that indicates otherwise there. Nevertheless, I think APIs that guarantee that timers would not tick anymore, including any currently running or scheduled callbacks, should be named very clearly to indicate that. Stop() is not enough to indicate that behavior, nor StopAsync(). I had already agreed with the potential for confusion in the naming. If we were to find an appropriate name for the proposed behavior, would you still object to adding a more discoverable and readable API for already-existing functionality? If it's only the naming then we can move on to that problem but I suspect there is more to it...

[stephentoub]

they really shouldn't be created frequently enough to warrant pooling them

Timers are often created and destroyed at incredibly high frequencies; it's one of the reasons the lock used by them was showing up repeatedly as a hot source of contention and why we invested in fixing that. I agree they ideally shouldn't fire at high frequency, but that's completely different. On top of that, creating a Timer doesn't just create one Timer object... it creates three objects (Timer, TimerHolder, and TimerQueueTimer)... and one of them is finalizable (TimerHolder), making it that much more expensive.

If we were to find an appropriate name for the proposed behavior, would you still object to adding a more discoverable and readable API for already-existing functionality?

To me this is not a problem worth solving. You're talking about adding an API whose purpose is to change the timer to not schedule additional work items; "change" is the current name of that exact functionality that already exists, and I'm having trouble envisioning a different name that is not only more discoverable and more "readable" but that also conveys the subtleties of the operation in question so as to not be misunderstand, and that is so much more clear and more discoverable and more readable that it's worth additional surface area to be tested and documented and maintained.

[kouvel]

Timers are often created and destroyed at incredibly high frequencies; it's one of the reasons the lock used by them was showing up repeatedly as a hot source of contention and why we invested in fixing that.

Is this a consequence of bad design? Or is it truly practical despite the design? Perhaps there would be a way to implement a timer in a way where the most frequent contender for that use case would not face the issue despite the timer implementation here. I wonder if the real problem here has anything to do with this issue.

I'm having trouble envisioning a different name that is not only more discoverable and more "readable" but that also conveys the subtleties of the operation in question so as to not be misunderstand, and that is so much more clear and more discoverable and more readable that it's worth additional surface area to be tested and documented and maintained.

I don't see it as such a huge step from where we currently are. I think we can agree to disagree here.

[benaadams]

So use System.Timers.Timer if you want to call .Stop() and use System.Threading.Timer if you want to call .Change(-1, -1)?

[kouvel]

I wouldn't recommend using System.Timers.Timer in any scenario. It's a very inefficient wrapper for System.Threading.Timer. It's useful for UI cases, but the same thing can be done in other ways and it has bugs that haven't yet been fixed.

[benaadams]

I wouldn't recommend using System.Timers.Timer in any scenario.

I mostly thought I was joking; but seems to be the top answer on SO; so um... https://stackoverflow.com/questions/6379541/reliably-stop-system-threading-timer

[kouvel]

System.Timers.Timer.Stop() disposes the System.Threading.Timer, not ideal and also not a guarantee that no more callbacks would occur. Its implementation is entirely based on System.Threading.Timer (inefficiently) with some gimmicks to handle UI cases better, but "better" is relative.

[mangod9]

Assume its ok to close this issue, since the original proposal is not being implemented?

[mangod9]

Closing for now.