C# Timer elapsed still runs little bit more after stopped

Question

I am working with windows forms. I am using System.Timers.Timer . Although I stopped the timer with timer.Stop(), It still goes little bit more. I put some bool variable to prevent this but no luck. Anybody knows anyhting about it ? Thank you.

            timer = new System.Timers.Timer();
            timer.Elapsed += OnTimedEvent;
            timer.Interval = 1000;
            timer.start();

public void cancelConnectingSituation(Boolean result)
        {
            connecting = false;
            timer.Stop();
            if (result)
            {
                amountLabel.Text = "Connected";
            } 
            else
            {
                amountLabel.Text = "Connection fail";
            }
        }


private void OnTimedEvent(Object source, ElapsedEventArgs e)
        {
            if (device.position == 2 && connecting)
            {
                refreshTime();
                setConnectingText();
            }
            else if (connecting)
            {
                setConnectingText();
            }
            else
            {
                refreshTimeAndAmount();
            }
        }

Please [update your question](http://stackoverflow.com/posts/35349971/edit) to include the code of you delcaring the timer, starting the timer and stopping the timer. Right now the answer to your question is "This happens because you have a bug in your code" but we will not be able to help you fix that bug without seeing the code that has the bug. — Scott Chamberlain, Feb 11 '16 at 21:05
Please show a [mcve] - otherwise we'll be guessing, basically. — Jon Skeet, Feb 11 '16 at 21:08
The `timer.Interval = 1;` line is telling the timer to fire in 1 ms intervals. What is the point of that line? Doing this right before stopping the timer is an easy way to get one or more events fired quickly in succession. — vgru, Feb 11 '16 at 21:16
Can you elaborate (as a edit to the question) what "It still goes little bit more" means. Currently it is unclear what you want it to do. For example if the timer is in the middle of `refreshTime()` and you call `timer.Stop()` do you expect `setConnectingText()` to ***not*** be called? — Scott Chamberlain, Feb 11 '16 at 21:16
@groo 'timer.Interval = 1;' if timer event fired before, to finish it quickly. later I already call the the 'timer.stop()' — a-f-a, Feb 11 '16 at 21:20
Scott Chamberlain that also could be my problem, yes may be. — a-f-a, Feb 11 '16 at 21:22
@a-f-a: timer's event handler is being executed on a different thread (unless you set its `SynchronizingObject`). Once the event is fired, handler will take a certain time to execute (on a separate thread), regardless of the status of your timer. Setting `Interval` to `1` only tells the timer to start firing events every 1 ms, it won't "speed up" event execution in any way. And each of these fired events will get queued into the thread pool, i.e. get its own thread. — vgru, Feb 11 '16 at 21:25
@a-f-a: since this is winforms code, I would suggest you to use the `System.Windows.Forms.Timer` instead, because it always dispatches the event handler on the (one and only) UI thread. This makes synchronization much simpler, since your UI event handlers (button clicks and similar) are also fired on this same thread. — vgru, Feb 11 '16 at 21:26

score 6 · Accepted Answer · answered Feb 11 '16 at 21:08

6

When a System.Timers.Timer Elapsed event is fired, it is fired on a background, ThreadPool thread. This means that, by the time you call Stop, an event might already have fired and this thread is queued for execution.

If you want to ensure your event doesn't fire after you stop the timer, you will need (besides your boolean variable) a lock:

 readonly object _lock = new object();
 volatile bool _stopped = false;

 void Stop()
 {
     lock (_lock)
     {
         _stopped = true;
         _timer.Stop();
     }
 }

 void Timer_Elapsed(...) 
 {
     lock (_lock)
     {
         if (_stopped)
             return;

         // do stuff
     }
 }

Or, even simpler:

 readonly object _lock = new object();

 void Stop()
 {
     lock (_lock)
     {
         _timer.Enabled = false; // equivalent to calling Stop()
     }
 }

 void Timer_Elapsed(...) 
 {
     lock (_lock)
     {
         if (!_timer.Enabled)
             return;

         // do stuff
     }
 }

answered Feb 11 '16 at 21:08

vgru

49,838
16
120
201

@a-f-a: now that you've posted your code, I would also suggest you remove the line where you set `Interval` to `1` (1 ms). That doesn't make sense if you are trying to stop the timer. – vgru Feb 11 '16 at 21:20
I had this exact question and I implemented the same. But yet I have concern that the queued up Elapses will (since they must run sequential due to the lock) take a long time to finish. What about moving the check for if(!_timer.Enabled) return before the lock? it probably doesn't matter if the _timer.Enabled = false is locked at all. I haven't thought it all the way through yet though. It might be good to have it before and after the Lock. (the "before" would be to stop any more to be queued up after timer is disabled, and the "after" would be to short-circuit the already queued elapses) – Terence Apr 13 '17 at 14:21
@Terence: If you add a check before the lock, you still need to add it after the lock had been acquired. Adding two checks in this manner is called [double-checked locking](http://stackoverflow.com/a/394932/69809), and helps reduce contention in case of many simultaneous threads as you've noted. But it also introduces a possibility of creating a race condition, as you add more complexity to your code. – vgru Apr 13 '17 at 23:04
You also shouldn't remove the lock from the `Stop` method `_timer.Enabled = false` if you want to guarantee that threads are really stopped after the call to `Stop` returns. Placing `_timer.Enabled = false;` before the lock (if that's what you suggested) *could* improve performance, if you really have lots of threads waiting on the `lock` inside the handler (which actually indicates issues with timing and the duration of the handler - do you really want to starve the thread pool?). – vgru Apr 13 '17 at 23:10
But then again if you also want to have a `Start` method which can revive the timer, avoiding locks for performance reasons complicates things during start->stop transitions and disposing of your class (which must implement `IDisposable` since `Timer` is `IDisposable`). To quote Jon Skeet: *(if) I have to choose between double-checked locking and "lock every time" code, I'd go for "locking every time" until I'd got real evidence that it was causing a bottleneck.* – vgru Apr 13 '17 at 23:13
Btw, if your jobs are long and you think a bunch of threads will start getting queued perhaps you can: a) disable the timer while the handler is running, or b) add a lockless volatile check before the lock inside the handler to skip an iteration if the handler is already busy (instead of blocking a threadpool thread, if exact timing is not important). – vgru Apr 13 '17 at 23:15

C# Timer elapsed still runs little bit more after stopped

1 Answers1