Static Semaphore passed as member to Thread class instance = bug?

Question

There are a few windows service projects in our codebase that follow this model, and I just want to make sure it's not a bug.

In this model, there is at least one static "timer" class, managed by a System.Timers.Timer, that retrieves batches of data to process from the database. A Semaphore is initialized as a static member. After retrieving a batch of work, a new worker thread instance is spawned off to process each row or list item, and the threads are managed by the parent timer class' static semaphore.

internal class BatchTimer
{
  private static Semaphore m_sem = new Semaphore(Settings.Default.ThreadCount, 
     Settings.Default.ThreadCount);
  private static System.Timers.Timer m_timer = new System.Timers.Timer();

  internal static void DoWork(object source, ElapstedEventArgs e)
  { 
    List<WorkRow> work = Work.GetBatch(Settings.Default.ObjectsPerIteration);
    foreach (WorkRow row in work)
    {
      WorkerThread worker = new WorkerThread
      {
        Semaphore = m_sem,
        Work = row
      };    
      Thread thread = new Thread(worker.Run);
      // Release() is called in finally block of WorkerThread.Run.
      m_sem.WaitOne(); 
      thread.Start();
    }
  }
}

The static Semaphore from the timer class is passed to a member of the worker thread class, rather than having the worker thread call Release() on the parent timer class' semaphore. I think it was assumed that it would work since it's a reference type. But my question is, wouldn't this prevent garbage collection?

Answer 1

BAsically, you have a GCRoot to the worker as long as the thread you called worker.Run on is, well, running. When the Run method ends, the worker is candidate for Garbage Collection (since it's no longer referenced in the main thread, nor in any worker thread).

The worker gets a reference to the mutex, but you would only have a problem if it was the other way round (having a static variable holding a reference to your worker).

So, basically, the GC would take care of your worker.

You may run into trouble if one of your worker blocks. You'd end up having a convoy or, worse, if all your workers are stuck, no work done at all. I would implement a timeout in the worker to make sure it doesn't block the whole process.