1

I have a class that creates and uses a System.Threading.Timer, something like this:

using System.Threading;

public class MyClass : IDisposable
{
  private List<int> ints = new List<int>();

  private Timer t;

  public MyClass()
  {
    //Create timer in disabled state

    t = new Timer(this.OnTimer, null, Timeout.Infinite, Timeout.Infinite);
  }

  private void DisableTimer()
  {
    if (t == null) return;

    t.Change(Timeout.Infinite, Timeout.Infinite);
  }

  private void EnableTimer()
  { 
    if (t == null) return;

    //Fire timer in 1 second and every second thereafter.
    EnableTimer(1000, 1000);
  }

  private void EnableTimer(long remainingTime)
  {
    if (t == null) return;

    t.Change(remainingTime, 1000);
  }

  private void OnTimer(object state)
  {
    lock (ints) //Added since original post
    {
      DisableTimer();

      DoSomethingWithTheInts();
      ints.Clear();

      //
      //Don't reenable the timer here since ints is empty.  No need for timer
      //to fire until there is at least one element in the list.
      //
    }
  }

  public void Add(int i)
  {
    lock(ints) //Added since original post
    {
      DisableTimer();

      ints.Add(i);
      if (ints.Count > 10)
      {
        DoSomethingWithTheInts();
        ints.Clear();
      }

      if (ints.Count > 0)
      {
        EnableTimer(FigureOutHowMuchTimeIsLeft());
      }
    }
  }

  bool disposed = false;

  public void Dispose()
  {
    //Should I protect myself from the timer firing here?

    Dispose(true);
  }

  protected virtual void Dispose(bool disposing)
  {
    //Should I protect myself from the timer firing here?

    if (disposed) return;
    if (t == null) return;

    t.Dispose();
    disposed = true;
  }
}

EDIT - In my actual code I do have locks on the List in both Add and OnTimer. I accidentally left them out when I simplified my code for posting.

Essentially, I want to accumulate some data, processing it in batches. As I am accumulating, if I get 10 items OR it has been 1 second since I last processed the data, I will process what I have so far and clear my list.

Since Timer is Disposable, I have implemented the Disposable pattern on my class. My question is this: Do I need extra protection in either Dispose method to prevent any side effects of the timer event firing?

I could easily disable the timer in either or both Dispose methods. I understand that this would not necesarily make me 100% safe as the timer could fire between the time that Dispose is called and the timer is disabled. Leaving that issue aside for the moment, would it be considered a best practice to guard the Dispose method(s), to the best of my ability, against the possibility of the timer executing?

Now that I think about it, I should probably also consider what I should do if the List is not empty in Dispose. In the usage pattern of the object is should be empty by then, but I guess that anything is possible. Let's say that there were items left in the List when Dispose is called, would it be good or bad to go ahead and try to process them? I suppose that I could put in this trusty old comment:

public void Dispose()
{
  if (ints != null && ints.Count > 0)
  {
    //Should never get here.  Famous last words!
  }
}

Whether or not there are any items in the list is secondary. I am really only interested in finding out what the best practice is for dealing with potentially enabled Timers and Dispose.

If it matters, this code is actually in a Silverlight class library. It does not interact with the UI at all.

EDIT:

I found what looks like a pretty good solution here. One answer, by jsw, suggests protecting the OnTimer event with Monitor.TryEnter/Monitor.Exit, effectively putting the OnTimer code in a critical section.

Michael Burr posted what seems to be an even better solution, at least for my situation, of using a one-shot timer by setting the due time to the desired interval and setting the period to Timeout.Infinite.

For my work, I only want the timer to fire if at least one item has been added to the List. So, to begin with, my timer is disabled. Upon entering Add, disable the timer so that it does not fire during the Add. When an item is added, process the list (if necessary). Before leaving Add, if there are any items in the list (i.e. if the list has not been processed), enable the timer with the due time set to the remaining interval and the period set to Timeout.Infinite.

With a one-shot timer, it is not even necessary to disable the timer in the OnTimer event since the timer will not fire again anyway. I also don't have to enable the timer when I leave OnTimer as there will not be any items in the list. I will wait until another item is added to the list before enabling the one-shot timer again. Thanks!

EDIT - I think that this is the final version. It uses a one-shot timer. The timer is enabled when the list goes from zero members to one member. If we hit the count threshold in Add, the items are processed and the timer is disabled. Access to the list of items is guarded by a lock. I am on the fence as to whether a one-shot timer buys me much over a "normal" timer other than that it will only fire when there are items in the list and only 1 second after the first item was added. If I use a normal timer, then this sequence could happen: Add 11 items in rapid succession. Adding the 11th causes the items to be processed and removed from the list. Assume 0.5 seconds is left on the timer. Add 1 more item. Time will fire in approx 0.5 seconds and the one item will be processed and removed. With one-shot timer the timer is reenabled when the 1 item is added and will not fire until a full 1 second interval (more or less) has elasped. Does it matter? Probably not. Anyway, here is a version that I think is reasonably safe and does what I want it to do.

using System.Threading;

public class MyClass : IDisposable
{
  private List<int> ints = new List<int>();

  private Timer t;

  public MyClass()
  {
    //Create timer in disabled state

    t = new Timer(this.OnTimer, null, Timeout.Infinite, Timeout.Infinite);
  }

  private void DisableTimer()
  {
    if (t == null) return;

    t.Change(Timeout.Infinite, Timeout.Infinite);
  }

  private void EnableTimer()
  { 
    if (t == null) return;

    //Fire event in 1 second but no events thereafter.
    EnableTimer(1000, Timeout.Infinite);
  }

  private void DoSomethingWithTheInts()
  {
    foreach (int i in ints)
    {
      Whatever(i);
    }
  }

  private void OnTimer(object state)
  {
    lock (ints)
    {
      if (disposed) return;
      DoSomethingWithTheInts();
      ints.Clear();
    }
  }

  public void Add(int i)
  {
    lock(ints)
    {
      if (disposed) return;

      ints.Add(i);
      if (ints.Count > 10)
      {
        DoSomethingWithTheInts();
        ints.Clear();
      }

      if (ints.Count == 0)
      {
        DisableTimer();
      }
      else
      if (ints.Count == 1)
      {
        EnableTimer();
      }
    }
  }

  bool disposed = false;

  public void Dispose()
  {
    if (disposed) return;

    Dispose(true);
  }

  protected virtual void Dispose(bool disposing)
  {
    lock(ints)
    {
      DisableTimer();
      if (disposed) return;
      if (t == null) return;

      t.Dispose();
      disposed = true;
    }
  }
}
Community
  • 1
  • 1
wageoghe
  • 27,390
  • 13
  • 88
  • 116

2 Answers2

1

I see some serious problems with synchronization in this code. I think what you are trying to solve is a classic reader-writer problem. With current approach it is highly probably You are going to run into some problems, like What if someone tries to modify the list when processing it?

I STRONGLY recommend using parallel extensions for .net or (when using .net 3.5 or earlier) using classes such like ReaderWriterlock or even simple Lock keyword. Also remember that System.Threading.Timer is a asynchronous call, so the OnTimer is called from SEPARATE thread (from a .net ThreadPool) so YOU definately need some synchronization (like possibly locking the collection).

I'd have a look at concurrent collections in .NEt 4 or use some synchronization primitives in .net 3.5 or earlier. DO not disable the timer in ADD method. Write correct code in OnTimer like

lock(daObject)
{
    if (list.Count > 10)
        DoSTHWithList;
}

This code is the simplest (although definetely not optimal) that should work. Also similiar code should be added to the Add method (locking the collection). Hope it helps, if not msg me. luke

luckyluke
  • 1,553
  • 9
  • 16
  • Thanks. I left out the locks when I simplified my actual code for posting. Just curious, why should I not disable the timer during OnTimer? The interval should be such that timer would not fire during OnTimer anyway, but if the interval happens to be short and OnTimer takes a long time, OnTimer could fire and then where would I be? I suppose another approach is to have a flag in OnTimer that would allow me to short-circuit any reentrant OnTimer calls. – wageoghe Nov 02 '10 at 22:00
  • If the second (and subsequent) on time fires when the first one is still processing You should write the OnTimer in such a manner that it Will do the correct Thing (like do nothing for example) or perhaps collect another 10 items and do sth to them (like the first one copied the ten items and unlocked the collection and now is processing them, if processing of one item takes a long time). Remember that the timer is on separate thread, so it pulses, checks the state of the class, does something and dies... – luckyluke Nov 02 '10 at 22:05
1

You should not have to disable the timer during OnTimer be cause you have a lock around the call, hence all the threads are waiting for the first one to finish...

dexter
  • 7,063
  • 9
  • 54
  • 71