views:

1602

answers:

3

I have a System.Threading.Timer that calls its appropriate event handler (callback) every 10 ms. The method itself is not reentrant and can sometimes take way longer than 10 ms. Thus, I want to stop the timer during method execution.

Code:

private Timer _creatorTimer;

// BackgroundWorker's work
private void CreatorWork(object sender, DoWorkEventArgs e) {
      _creatorTimer = new Timer(CreatorLoop, null, 0, 10);

      // some other code that worker is doing while the timer is active
      // ...
      // ...
}

private void CreatorLoop(object state) {
      // Stop timer (prevent reentering)
      _creatorTimer.Change(Timeout.Infinite, 0);

      /*
          ... Work here
      */

      // Reenable timer
      _creatorTimer.Change(10, 0);
}

MSDN states that the callback method is called (every time the timer fires) in separate thread from the thread pool. That means that if I stop the timer the first thing in method it still doesn't neccessarily prevent the timer to fire and run another instance of the method before the first one had a chance to stop the timer.

Should maybe the timer (or even the non-reentrant method itself) be locked? What is the right way to prevent timer from firing during execution of its callback (and non-reentrant) method?

+1  A: 

I've had similar situation with a System.Timers.Timer, where the elapsed event is executed from a threadpool and needs to be reentrant.

I used this method to get around the issue:

private void tmr_Elapsed(object sender, EventArgs e)
{
    tmr.Enabled = false;
    // Do Stuff
    tmr.Enabled = true;
}

Depending on what you're doing you may want to consider a System.Timers.Timer, here's a nice summary from MSDN

                                         System.Windows.Forms    System.Timers         System.Threading  
Timer event runs on what thread?         UI thread               UI or worker thread   Worker thread
Instances are thread safe?               No                      Yes                   No
Familiar/intuitive object model?         Yes                     Yes                   No
Requires Windows Forms?                  Yes                     No                    No
Metronome-quality beat?                  No                      Yes*                  Yes*
Timer event supports state object?       No                      No                    Yes
Initial timer event can be scheduled?    No                      No                    Yes
Class supports inheritance?              Yes                     Yes                   No

* Depending on the availability of system resources (for example, worker threads)
ParmesanCodice
I believe that this does not actually get around the issue. Well, it may in 99.9% cases, but if the system does not give processor time to your event handler before the next timer's Elapsed event firing, two different threads may execute the method in parallel.
kornelijepetak
Good point! You could always use this in conjunction with a locking solution like jsw's
ParmesanCodice
+9  A: 

You could let the timer continue firing the callback method but wrap your non-reentrant code in a Monitor.TryEnter/Exit. No need to stop/restart the timer in that case; overlapping calls will not acquire the lock and return immediately.

 private void CreatorLoop(object state) 
 {
   if (Monitor.TryEnter(lockObject)
   {
     try
     {
       // Work here
     }
     finally
     {
       Monitor.Exit(lockObject);
     }
   }
 }
jsw
+1 I never really thought of a use for TryEnter; that is very interesting.
Schmuli
This seems to be doing the trick. Two or more threads may enter the method, but only one will actually work. I have also stopped the timer after Monitor.TryEnter() so it does not fire at all during execution (there is no need for it to fire), just in case the execution time is much greater than the timer's period. Timer is restarted after the work in method completes.
kornelijepetak
+1 Nice solution!
ParmesanCodice
+2  A: 

A couple possible solutions:

  • have the real work done in yet another thread delegate that's waiting on an event. The timer callback merely signals the event. The worker thread cannot be reentered, as it's a single thread that does its work only when the event is signaled. The timer is reentrant, since all it does is signal the event (seems a little roundabout and wasteful, but it'll work)
  • have the timer created with only a start timeout and no periodic timeout so it'll fire only once. The timer callback will dispose of that timer object and create a new one when it has completed its work that will also only fire once.

You may be able to manage option #2 without disposing/creating a new object by using the Change() method of the original timer object, but I'm not sure what the behavior is exactly of calling Change() with a new start timeout after the first timeout has expired. That would be worth a test or two.

Edit:


I did the test - manipulating the timer as a restartable one-shot seems to work perfectly, and it's much simpler than the other methods. Here's some sample code based on yours as a starting point (a few details may have changed to get it to compile on my machine):

private Timer _creatorTimer;

// BackgroundWorker's work
private void CreatorWork(object sender, EventArgs e) {
    // note: there's only a start timeout, and no repeat timeout
    //   so this will fire only once
    _creatorTimer = new Timer(CreatorLoop, null, 1000, Timeout.Infinite);

    // some other code that worker is doing while the timer is active
    // ...
    // ...
}

private void CreatorLoop(object state) {
    Console.WriteLine( "In CreatorLoop...");
    /*
        ... Work here
    */
    Thread.Sleep( 3000);

    // Reenable timer
    Console.WriteLine( "Exiting...");

    // now we reset the timer's start time, so it'll fire again
    //   there's no chance of reentrancy, except for actually
    //   exiting the method (and there's no danger even if that
    //   happens because it's safe at this point).
    _creatorTimer.Change(1000, Timeout.Infinite);
}
Michael Burr
This seems to work, yes, but the code becomes less clean when there's many outcomes of the method (many if-else branches and exceptions). But seems to be a good solution when it comes to performance because no sync mechanisms are used. Other than that, this WILL NOT work if there are other methods/threads/timers that can try to enter this method. Then of course, we are reentering non-reentrant method, which is where monitors work better. Thanks for solution and the test, anyway. It's a nice idea.
kornelijepetak
The complexity of the code is no more of an issue than if you're using a mutex - just wrap the code in a `try`/`finally` or simply call another routine that has the complexity and leave the timer callback routine a simple 'call then reset the timer'. If the callback will be used by multiple timers, then yes this technique won't work and you'll need a true synchronization object. I find that it's pretty uncommon for a timer callback to be used by multiple timers - particularly when the callback is very complex (that generally means the timer callback is designed for a very specific purpose).
Michael Burr