72

Well I've searched a lot for a solution to this. I'm looking for a clean and simple way to prevent the callback method of a System.Threading.Timer from being invoked after I've stopped it.

I can't seem to find any, and this has led me, on occassion, to resort to the dreaded thread-thread.sleep-thread.abort combo.

Can it be done using lock?

GEOCHET
  • 21,119
  • 15
  • 74
  • 98
JayPea
  • 9,551
  • 7
  • 44
  • 65
  • 2
    The *after* is not your real problem. Stopping it *while* the callback runs is the hard case. Which is quite possible. Use a lock. – Hans Passant Jun 16 '11 at 23:37
  • 1
    Well, while not a big problem it still should be checked and handled. Otherwise callback could try to use some resources that were already freed or assuming that they were never called again etc. I have seen many times strange errors on application exit caused by incorrect assumptions in the callbacks. – Ivan Danilov Jun 17 '11 at 01:34

11 Answers11

146

An easier solution might to be to set the Timer never to resume; the method Timer.Change can take values for dueTime and period that instruct the timer never to restart:

this.Timer.Change(Timeout.Infinite, Timeout.Infinite);

Whilst changing to use System.Timers.Timer might be a "better" solution, there are always going to be times when that's not practical; just using Timeout.Infinite should suffice.

Owen Blacker
  • 4,117
  • 2
  • 33
  • 70
  • 15
    This is the best answer. Posters and Readers may well be using System.Threading.Timers for a good reason (like me). This method just stops the timer, which was just what I needed. – Steve Hibbert Oct 20 '14 at 16:03
  • 1
    this is strangely how to delete the underlying timer. the value is really just -1, which is converted to `uint`, which is basically `uint.MaxValue` (though they don't use that explicitly and instead use `(uint)-1`. The `Timeout` class says: "A constant used by methods that take a timeout (Object.Wait, Thread.Sleep etc) to indicate that no timeout should occur." The timer class explicitly disables the timer based on this value. – Dave Cousineau May 21 '22 at 22:08
44

like Conrad Frix suggested you should use the System.Timers.Timer class instead, like:

private System.Timers.Timer _timer = new System.Timers.Timer();
private volatile bool _requestStop = false;

public constructor()
{
    _timer.Interval = 100;
    _timer.Elapsed += OnTimerElapsed;
    _timer.AutoReset = false;
    _timer.Start();
}

private void OnTimerElapsed(object sender, System.Timers.ElapsedEventArgs e)
{
    // do work....
    if (!_requestStop)
    {
        _timer.Start();//restart the timer
    }
}

private void Stop()
{
    _requestStop = true;
    _timer.Stop();
}

private void Start()
{
    _requestStop = false;
    _timer.Start();
}
Owen Blacker
  • 4,117
  • 2
  • 33
  • 70
Jalal Said
  • 15,906
  • 7
  • 45
  • 68
  • Thanks. I tried this an it appears to be working. I can't help but feel that there's a race condition in there somewhere, but in practice it hasn't failed. Can someone else comment on this? – JayPea Jun 17 '11 at 20:51
  • What is the reason of putting AutoReset = false and then doing _timer.Stop() in the Elapsed-method? Wouldn't AutoReset=true accomplish the same thing? – Kyberias Sep 02 '11 at 19:14
  • 7
    The reason I'm confused about this answer is that it does not seem to solve the problem. The caller of Stop() here still cannot be sure that the "do work" part is not executed after the Stop() call. And I believe that was the original question/problem. – Kyberias Sep 02 '11 at 19:36
  • @Kyberias: It doesn't accomplish any thing that setting `AutoReset` to false doesn't solve, however I leave it there by mistake.. – Jalal Said Sep 02 '11 at 20:00
  • 1
    Your implementation of Stop(), will not guarantee that any active thread have left OnTimerElapsed(). So race-condition will occur if one afterwards release/stop other objects, that the OnTimerElapsed() makes use of. – Rolf Kristensen Sep 09 '13 at 21:18
  • @Rolf: no it doesn't, this code will not kill the active thread if any by calling `Stop()`, the purpose of this not to insure that their is no active threads that already running and passes the first `if (!_requestStop)' on `OnTimerElapsed`, that is it, this code is not using for cancellation the running code inside the `OnTimerElapsed` method, if you want so you have to be use another method inside 'OnTimerElapced' like 'CancellationTaken` or so to handle this issue if you want to. – Jalal Said Sep 10 '13 at 10:23
  • Microsoft favors System.Threading.Timer before System.Timers.Timer. (From https://stackoverflow.com/questions/1416803/system-timers-timer-vs-system-threading-timer/37953192#37953192) – Michael Freidgeim Nov 21 '20 at 22:53
14

The MSDN Docs suggest that you use the Dispose(WaitHandle) method to stop the timer + be informed when callbacks will no longer be invoked.

Will A
  • 24,780
  • 5
  • 50
  • 61
12

For the System.Threading.Timer one can do the following (Will also protect the callback-method from working on a disposed timer - ObjectDisposedException):

class TimerHelper : IDisposable
{
    private System.Threading.Timer _timer;
    private readonly object _threadLock = new object();

    public event Action<Timer,object> TimerEvent;

    public void Start(TimeSpan timerInterval, bool triggerAtStart = false,
        object state = null)
    {
        Stop();
        _timer = new System.Threading.Timer(Timer_Elapsed, state,
            System.Threading.Timeout.Infinite, System.Threading.Timeout.Infinite);

        if (triggerAtStart)
        {
            _timer.Change(TimeSpan.FromTicks(0), timerInterval);
        }
        else
        {
            _timer.Change(timerInterval, timerInterval);
        }
    }

    public void Stop(TimeSpan timeout = TimeSpan.FromMinutes(2))
    {
        // Wait for timer queue to be emptied, before we continue
        // (Timer threads should have left the callback method given)
        // - http://woowaabob.blogspot.dk/2010/05/properly-disposing-systemthreadingtimer.html
        // - http://blogs.msdn.com/b/danielvl/archive/2011/02/18/disposing-system-threading-timer.aspx
        lock (_threadLock)
        {
            if (_timer != null)
            {
                ManualResetEvent waitHandle = new ManualResetEvent(false)
                if (_timer.Dispose(waitHandle))
                {
                   // Timer has not been disposed by someone else
                   if (!waitHandle.WaitOne(timeout))
                      throw new TimeoutException("Timeout waiting for timer to stop");
                }
                waitHandle.Close();   // Only close if Dispose has completed succesful
                _timer = null;
            }
        }
    }

    public void Dispose()
    {
        Stop();
        TimerEvent = null;
    }

    void Timer_Elapsed(object state)
    {
        // Ensure that we don't have multiple timers active at the same time
        // - Also prevents ObjectDisposedException when using Timer-object
        //   inside this method
        // - Maybe consider to use _timer.Change(interval, Timeout.Infinite)
        //   (AutoReset = false)
        if (Monitor.TryEnter(_threadLock))
        {
            try
            {
                if (_timer==null)
                    return;

                Action<Timer, object> timerEvent = TimerEvent;
                if (timerEvent != null)
                {
                    timerEvent(_timer, state);
                }
            }
            finally
            {
                Monitor.Exit(_threadLock);
            }
        }
    }
}

This is how one can use it:

void StartTimer()
{
    TimerHelper _timerHelper = new TimerHelper();
    _timerHelper.TimerEvent += (timer,state) => Timer_Elapsed();
    _timerHelper.Start(TimeSpan.FromSeconds(5));
    System.Threading.Sleep(TimeSpan.FromSeconds(12));
    _timerHelper.Stop();
}

void Timer_Elapsed()
{
   // Do what you want to do
}
Rolf Kristensen
  • 17,785
  • 1
  • 51
  • 70
  • This doesn't seem to work reliably. I quite often get a timeout even though no more callbacks are running. But of course, you code doesn't check for timeout, so... – BatteryBackupUnit Feb 19 '16 at 15:48
  • @BatteryBackupUnit If the task executed by the timer takes more than 2 minutes, then yes this implementation will fail. I would probably add a cancellation-token to the state-object and monitor this in your TimerEvent-subscription-method (or change the timeout to infinite) – Rolf Kristensen Mar 20 '16 at 08:57
6

For what it's worth, we use this pattern quite a bit:

// set up timer
Timer timer = new Timer(...);
...

// stop timer
timer.Dispose();
timer = null;
...

// timer callback
{
  if (timer != null)
  {
    ..
  }
}
  • 11
    this would not be a thread safe because (1) the timer = null could be updated from a thread and the call back thread still see the old value of it. (2) if you use the timer instance inside the timer call back after the "if (timer != null)" check; it could be null because another thread call the stop method and make it null. I really don't recommend this pattern unless you do something for thread safe like lock... – Jalal Said Jun 17 '11 at 02:05
  • 1
    @JalalAldeenSaa'd: Would it be OK if the Timer variable is marked "volatile"? – RenniePet Oct 26 '14 at 00:57
  • @RenniePet: no, even when it is volatile the race still can occuers, 'volatile' doesn't forbid the timer variable from being updated, i.e. it is not a lock statement – Jalal Said Oct 29 '14 at 14:42
4

This answer relates to System.Threading.Timer

I've read a lot of nonsense about how to synchronize disposal of System.Threading.Timer all over the net. So that's why I'm posting this in an attempt to rectify the situation somewhat. Feel free to tell me off / call me out if something I'm writing is wrong ;-)

Pitfalls

In my opinion there's these pitfalls:

  • Timer.Dispose(WaitHandle) can return false. It does so in case it's already been disposed (I had to look at the source code). In that case it won't set the WaitHandle - so don't wait on it!
  • not handling a WaitHandle timeout. Seriously - what are you waiting for in case you're not interested in a timeout?
  • Concurrency issue as mentioned here on msdn where an ObjectDisposedException can occur during (not after) disposal.
  • Timer.Dispose(WaitHandle) does not work properly with -Slim waithandles, or not as one would expect. For example, the following does not work (it blocks forever):
 using(var manualResetEventSlim = new ManualResetEventSlim)
 {
     timer.Dispose(manualResetEventSlim.WaitHandle);
     manualResetEventSlim.Wait();
 }

Solution

Well the title is a bit "bold" i guess, but below is my attempt to deal with the issue - a wrapper which handles double-disposal, timeouts, and ObjectDisposedException. It does not provide all of the methods on Timer though - but feel free to add them.

internal class Timer
{
    private readonly TimeSpan _disposalTimeout;
    
    private readonly System.Threading.Timer _timer;

    private bool _disposeEnded;

    public Timer(TimeSpan disposalTimeout)
    {
        _disposalTimeout = disposalTimeout;
        _timer = new System.Threading.Timer(HandleTimerElapsed);
    }

    public event Action Elapsed;

    public void TriggerOnceIn(TimeSpan time)
    {
        try
        {
            _timer.Change(time, Timeout.InfiniteTimeSpan);
        }
        catch (ObjectDisposedException)
        {
            // race condition with Dispose can cause trigger to be called when underlying
            // timer is being disposed - and a change will fail in this case.
            // see 
            // https://msdn.microsoft.com/en-us/library/b97tkt95(v=vs.110).aspx#Anchor_2
            if (_disposeEnded)
            {
                // we still want to throw the exception in case someone really tries
                // to change the timer after disposal has finished
                // of course there's a slight race condition here where we might not
                // throw even though disposal is already done.
                // since the offending code would most likely already be "failing"
                // unreliably i personally can live with increasing the
                // "unreliable failure" time-window slightly
                throw;
            }
        }
    }

    private void HandleTimerElapsed(object state)
    {
        Elapsed?.Invoke();
    }

    public void Dispose()
    {
        var waitHandle = new ManualResetEvent(false));

        // returns false on second dispose
        if (_timer.Dispose(waitHandle))
        {
            if (waitHandle.WaitOne(_disposalTimeout))
            {
                _disposeEnded = true;
                waitHandle.Dispose();
            }
            else
            {
                // don't dispose the wait handle, because the timer might still use it.
                // Disposing it might cause an ObjectDisposedException on 
                // the timer thread - whereas not disposing it will 
                // result in the GC cleaning up the resources later
                throw new TimeoutException(
                    "Timeout waiting for timer to stop. (...)");
            }
        }
    }
}
BatteryBackupUnit
  • 12,934
  • 1
  • 42
  • 68
  • Consider removing the using-statement around ManualResetEvent in Dispose(). If the waitHandle.WaitOne() fails then it will throw exception and dispose the event-handle, and when the timer finally completes then it will throw ObjectDisposedException. – Rolf Kristensen Jan 29 '17 at 18:11
  • @RolfKristensen thanks, but I don't quite understand. What's the benefit of removing `using`? Let's say `WaitOne(x)` fails with `ArgumentOutOfRangeException` - what's going to happen? I don't think the `ManualResetEvent` would be disposed in that case. As per the docs, `ManualResetEvent.Dispose()` does not throw an exception in any case, so there shouldn't be a problem with `using` hiding another exception, too. – BatteryBackupUnit Feb 02 '17 at 11:08
  • 1
    You give the waitHandle to the timer.Dispose(). This means the timer will call the waitHandle when complete. When you throw the TimeoutException, then the using-statement will close the waitHandle. When the timer-thread finally calls the waitHandle, then it will experience an ObjectDisposedException. – Rolf Kristensen Feb 02 '17 at 16:11
  • @RolfKristensen I see. It might cause an unhandled exception -> immediate shutdown of the process (I haven't tested it; maybe also the Timer itself is swallowing the exception...). In that case I agree it would be beneficial to only dispose when `WaitOne` returned true. – BatteryBackupUnit Sep 17 '21 at 10:21
  • @BatteryBackupUnit Pardon my ignorance, but I'm not clear on when/how I would use `TriggerOnceIn()`. Is the purpose to stop the timer while the event is running? – Mike Bruno Mar 17 '22 at 17:04
  • 1
    @MikeBruno The `Timer` wrapper-class example above is not a recurring timer but rather each time you call `TriggerOnceIn` the `Timer.Elapsed` event will be raised after the `TimeSpan time` you passed `TriggerOnceIn`. If you call `TriggerOnceIn` multiple times *before* the `Timer.Elapsed` event, the `Timer.Elapsed` event *should* only happen once (at the latest time specified). Note though, that due to the concurrent nature, there's no guarantee for that. Also `Timer.Dispose()` *should* cancel `Elapsed` from occurring, but of course it might be occurring concurrently at the same moment. – BatteryBackupUnit Mar 18 '22 at 07:20
3

To me, this seems to be the correct way to go: Just call dispose when you are done with the timer. That will stop the timer and prevent future scheduled calls.

See example below.

class Program
{
    static void Main(string[] args)
    {
        WriteOneEverySecond w = new WriteOneEverySecond();
        w.ScheduleInBackground();
        Console.ReadKey();
        w.StopTimer();
        Console.ReadKey();
    }
}

class WriteOneEverySecond
{
    private Timer myTimer;

    public void StopTimer()
    {
        myTimer.Dispose();
        myTimer = null;
    }

    public void ScheduleInBackground()
    {
        myTimer = new Timer(RunJob, null, 1000, 1000);
    }

    public void RunJob(object state)
    {
        Console.WriteLine("Timer Fired at: " + DateTime.Now);
    }
}
Pang
  • 9,564
  • 146
  • 81
  • 122
3

You can't guarantee that your code that supposed to stop the timer will execute before timer event invocation. For example, suppose on time moment 0 you initialized timer to call event when time moment 5 comes. Then on time moment 3 you decided that you no longer needed the call. And called method you want to write here. Then while method was JIT-ted comes time moment 4 and OS decides that your thread exhaust its time slice and switch. And timer will invoke the event no matter how you try - your code just won't have a chance to run in worst case scenario.

That's why it is safer to provide some logic in the event handler. Maybe some ManualResetEvent that will be Reset as soon as you no longer needed event invocation. So you Dispose the timer, and then set the ManualResetEvent. And in the timer event handler first thing you do is test ManualResetEvent. If it is in reset state - just return immediately. Thus you can effectively guard against undesired execution of some code.

Ivan Danilov
  • 14,287
  • 6
  • 48
  • 66
  • why don't you propose a solution that is self-contained? – agent-j Jun 17 '11 at 00:52
  • 1
    Self-contained solution exists and it is called Timer. It just needs to be used correctly. – Ivan Danilov Jun 17 '11 at 01:27
  • I am suggesting that since it is possible to use Timer incorrectly, why not create a reusable class that CANNOT be used incorrectly in this regard. The class can contain the complexity so you don't have to worry about it. – agent-j Jun 17 '11 at 01:29
  • 2
    In fact I can't think almost any synchronization instrument that couldn't be used incorrectly at all. I don't try to propose solution except timer because as a general-purpose tool it is as simple as I can imagine. Why to make something more complex and error-prone? I don't think it will simplify anything. – Ivan Danilov Jun 17 '11 at 01:38
2

You can stop a timer by creating a class like this and calling it from, for example, your callback method:

public class InvalidWaitHandle : WaitHandle
{
    public IntPtr Handle
    {
        get { return InvalidHandle; }
        set { throw new InvalidOperationException(); }
    }
}

Instantiating timer:

_t = new Timer(DisplayTimerCallback, TBlockTimerDisplay, 0, 1000);

Then inside callback method:

if (_secondsElapsed > 80)
{
    _t.Dispose(new InvalidWaitHandle());
}
Stonetip
  • 1,150
  • 10
  • 20
2

Perhaps you should do the opposite. Use system.timers.timer, set the AutoReset to false and only Start it when you want to

Conrad Frix
  • 51,984
  • 12
  • 96
  • 155
1

There is a MSDN link how to achieve stop timer correctly. Use ControlThreadProc() method with HandleElapsed(object sender, ElapsedEventArgs e) event synchronized by syncPoint static class variable. Comment out Thread.Sleep(testRunsFor); on ControlThreadProc() if it is not suitable(probably). The key is there that using static variable and an atomic operation like Interlocked.CompareExchange on conditional statements.

Link : Timer.Stop Method

Community
  • 1
  • 1
Tarık Özgün Güner
  • 1,051
  • 10
  • 10