17

I have a Windows Service implemented in C# that needs to do some work every so often. I've implemented this using a System.Threading.Timer with a callback method that is responsible for scheduling the next callback. I am having trouble gracefully stopping (i.e. disposing) the timer. Here's some simplified code you can run in a console app that illustrates my problem:

const int tickInterval = 1000; // one second

timer = new Timer( state => {
                       // simulate some work that takes ten seconds
                       Thread.Sleep( tickInterval * 10 );

                       // when the work is done, schedule the next callback in one second
                       timer.Change( tickInterval, Timeout.Infinite );
                   },
                   null,
                   tickInterval, // first callback in one second
                   Timeout.Infinite );

// simulate the Windows Service happily running for a while before the user tells it to stop
Thread.Sleep( tickInterval * 3 );

// try to gracefully dispose the timer while a callback is in progress
var waitHandle = new ManualResetEvent( false );
timer.Dispose( waitHandle );
waitHandle.WaitOne();

The problem is that I get an ObjectDisposedException from timer.Change on the callback thread while waitHandle.WaitOne is blocking. What am I doing wrong?

The documentation for the Dispose overload I'm using says:

The timer is not disposed until all currently queued callbacks have completed.

Edit: It appears that this statement from the documentation may be incorrect. Can someone verify?

I know that I could work around the problem by adding some signaling between the callback and the disposal code as Henk Holterman suggested below, but I don't want to do this unless absolutely necessary.

William Gross
  • 2,083
  • 2
  • 17
  • 35
  • Why can't you let the timer run by itself every 10 seconds? Why are you manually rescheduling it? – Tudor Sep 10 '12 at 15:36
  • 1
    @Tudor: Because sometimes the work takes longer, and I don't want multiple callbacks overlapping in their execution. – William Gross Sep 10 '12 at 15:37
  • Why are you manually exposing the timer when the service is stopped? When the AppDomain unloads all memory is re-claimed anyway. – Daniel Hilgarth Sep 10 '12 at 15:38
  • @DanielHilgarth: In my real implementation, I'm doing the timer disposal from `ServiceBase.OnStop`. I want to make sure the timer is disposed and no callbacks are in progress before I let the service shut down. I also have other shut down code I need to run, and I don't want to do this until I'm sure the timer is completely gone. – William Gross Sep 10 '12 at 15:43
  • I'm beginning to wonder if you are encountering a subtle problem tied to the Thread callback reference as the anonymous delegate to the constructor....what if you (for testing) converted it to a named function...given that the timer fires on a secondary CLR thread.. – David W Sep 10 '12 at 16:28
  • @DavidW: In the real Windows Service implementation that I have, the callback *is* a named function and I still run into the same issue. – William Gross Sep 10 '12 at 16:54
  • @WilliamGross DARN...ok, good enough :) – David W Sep 10 '12 at 17:24
  • That's caused by a [documented race condition](https://msdn.microsoft.com/en-us/library/b97tkt95(v=vs.110).aspx#Anchor_2). You might want to look at my answer [here](http://stackoverflow.com/a/35510799/684096) because it shows how to handle this. – BatteryBackupUnit Feb 19 '16 at 17:19

4 Answers4

12

With this code

 timer = new Timer( state => {
                   // simulate some work that takes ten seconds
                   Thread.Sleep( tickInterval * 10 );

                   // when the work is done, schedule the next callback in one second
                   timer.Change( tickInterval, Timeout.Infinite );
               },
               null,
               tickInterval, // first callback in one second
               Timeout.Infinite );

it is almost certain that you will Dispose the timer while it is sleeping.

You will have to safeguard the code after Sleep() to detect a Disposed timer. Since there is no IsDisposed property a quick and dirty static bool stopping = false; might do the trick.

H H
  • 263,252
  • 30
  • 330
  • 514
  • Thank you; I just thought I wouldn't need to do something like that since the documentation states "The timer is not disposed until all currently queued callbacks have completed." What does that sentence mean? – William Gross Sep 10 '12 at 15:52
  • hat sentence means you should be right but I saw a ObjectDisposed exception on timer.Change(). Get rid of that one anyway, there are other ways to prevent re-entrance. – H H Sep 10 '12 at 15:55
  • 3
    Wondering if perhaps the phrase "queued callbacks" literally means additional callbacks that may be pending, but not currently executing, eg a currently executing callback is, by definition, not queued? – David W Sep 10 '12 at 18:10
  • @DavidW: Yeah, maybe. I am just going to work around this problem for now by catching and suppressing `ObjectDisposedException` from `timer.Change` in the callback. Maybe eventually someone from Microsoft will see this and give me a better answer. – William Gross Sep 10 '12 at 18:17
  • 1
    You seem to have a reproducible case that contradicts the docs, consider posting it on [MS Connect](http://connect.microsoft.com/SearchResultsLive.aspx?SearchQuery=threading.timer) – H H Sep 10 '12 at 18:36
1

As described in "Concurrent Programming on Windows":
Create a dummy class InvalidWaitHandle, inheriting from WaitHandle:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.ComponentModel;
using System.Data;
using System.Diagnostics;
using System.Threading;

namespace MyNameSpace
{
    class InvalidWaitHandle : WaitHandle
    {

    }
}

Hence you can Dispose a System.Threading.Timer properly like this:

public static void DisposeTimer()
{
   MyTimer.Dispose(new InvalidWaitHandle());
   MyTimer = null;
}
MrDoubleU
  • 11
  • 2
0

Possible solution for protecting the callback method from working on a disposed timer:

ManualResetEvent waitHandle = new ManualResetEvent(false);
if (!timer.Dispose(waitHandle) || waitHandle.WaitOne((int)timeout.TotalMilliseconds)
{
    waitHandle.Close();  // Only close when not timeout
}

See also: https://stackoverflow.com/a/15902261/193178

Rolf Kristensen
  • 17,785
  • 1
  • 51
  • 70
-4

You do not need to dispose of the timer to stop it. You can call Timer.Stop() or set Timer.Enabled to false, either of which will stop the timer from running.

Mike Aski
  • 9,180
  • 4
  • 46
  • 63
Addy
  • 2,414
  • 1
  • 23
  • 43