views:

64

answers:

3

In my windows service I create one "parent" foreground thread that in turn spawns "child" threads using ThreadPool (which means they are background) to execute tasks.

What is the best way to close foreground thread gracefully on windows service stop?

Here is my current implementation (stripped out of task-specific logic):

public partial class TaskScheduler : ServiceBase
{
   private static AutoResetEvent _finishedTaskAutoResetEvent = new AutoResetEvent(false);

   //This flag is used to increase chances of the Spawning Thread to finish gracefully when service stops.
   private bool StopRequested { get; set; }

   private int _executingTasksCount;

   private int ExecutingTasksCount { get { return _executingTasksCount; } }

   private void IncCurrentTasksCount()
   {
       Interlocked.Increment(ref _executingTasksCount);
   }

   private void DecCurrentTasksCount()
   {
       Interlocked.Decrement(ref _executingTasksCount);
   }

   public TaskScheduler()
   {
       InitializeComponent();

       Thread spawningThread = new Thread(DoSpawnTaskExecutionThreads);

       spawningThread.Name = "Spawning Thread";
       spawningThread.IsBackground = false;
       spawningThread.Start();
   }

   protected override void OnStart(string[] args)
   {
   }

   protected override void OnStop()
   {
       StopRequested = true;
   }

   private void DoSpawnTaskExecutionThreads()
   {
       //We check StopRequested to try and finish this thread gracefully when service stops.
       while (!StopRequested)
       {
           while (!StopRequested && ExecutingTasksCount < MaxPooledTasks)
           {
               ThreadPool.QueueUserWorkItem(ExecuteTask, new Task());

               IncCurrentTasksCount();
           }

           _finishedTaskAutoResetEvent.WaitOne();
       }

       //Either all task execution threads will finish or the process will be terminated forcibly.
       while (ExecutingTasksCount > 0)
       {
           Thread.Sleep(200); //Check five times a second.
       }

       _eventLog.WriteEntry("The Spawning Thread finished along with task execution threads.");
   }

   private void ExecuteTask(object state)
   {
       try
       {
           Task task = (Task)state;

           task.Execute();
       }
       catch
       {
           // Handle exception.
       }
       finally
       {
           DecCurrentTasksCount();
           _finishedTaskAutoResetEvent.Set();
       }
   }

}

+2  A: 

There is one issues I see here:

StopRequested should not be an automatic property. You should define this as a property with a backing field, in order to mark it volatile.

private volatile bool stopRequested;
private bool StopRequested
{
    get { return this.stopRequested; }
    set { this.stopRequested = value; }
}

Without this, it's possible that the exit condition may not be seen (at least right away) by your thread when it's set by the service.

Also, if .NET 4 is an option, there are much simpler designed that could be done using CancellationToken and BlockingCollection<T>.

Reed Copsey
The only place I intend to change StopRequested in is OnStop(). Thank you for suggestion.
Den
@Den: OnStop will be called from a separate thread. Without this, the TaskScheduler's thread will not (necessarily) see the change.
Reed Copsey
@Den: Basically what would happen is that the JIT might see that `DoSpawnTaskExecutionThreads` never changes `StopRequested` and so it might try to optimize the repetitive reads by lifting them and combining them into one outside and above the loops. The `WaitOne` call you have in the loop will stop the compiler from making that optimization in reality, but that is case of it working by accident. It's best to follow Reed's advice here so that there is no doubt.
Brian Gideon
A: 

You can use the Join method to "gracefully" kill the thread. MSDN has some information about the method.

Pedro
That won't work here - you can't block the service thread, or the service host will kill it forcibly, as there's a timeout in place on shutting down services.
Reed Copsey
+1  A: 

I see a couple of problems with the code.

  • The check of StopRequested is not thread-safe.
  • The check of ExecutingTaskCount is not thread-safe.
  • Since _finishedTaskAutoResetEvent is an AutoResetEvent signals can get lost because that WaitHandle does not maintain a count. Maybe that is what you want, but it could result in some strange spinning of the nested while loops.

Here is how I would refactor your code. It uses the CountdownEvent class which is available in .NET 4.0.

public class TaskScheduler : ServiceBase
{
    private m_Stop as ManualResetEvent = new ManualResetEvent(false);

    protected override void OnStart(string[] args)           
    {           
      var thread = new Thread(DoSpawnTaskExecutionThreads);
      thread.Name = "Spawning Thread";
      thread.IsBackground = false;
      thread.Start();
    }           

    protected override OnStop()
    {
      m_Stop.Set();
    }

    public DoSpawnTaskExecutionThreads()
    {
      // The semaphore will control how many concurrent tasks can run.
      var pool = new Semaphore(MaxPooledThreads, MaxPooledThreads);

      // The countdown event will be used to wait for any pending tasks.
      // Initialize the count to 1 so that we treat this thread as if it 
      // were a work item. This is necessary to avoid a subtle race
      // with a real work item that completes quickly.
      var tasks = new CountdownEvent(1);

      // This array will be used to control the spinning of the loop.
      var all = new WaitHandle[] { pool, m_Stop };

      while (WaitHandle.WaitAny(all) == 0)
      {
        // Indicate that there is another task.
        tasks.AddCount();

        // Queue the task.
        Thread.QueueUserWorkItem(
          (state) =>
          {
            try
            {
              var task = (Task)state;
              task.Execute();
            }
            finally
            {
              pool.Release(); // Allow another task to be queued.
              tasks.Signal(); // Indicate that this task is complete.
            }
          }, new Task());
      }

      // Indicate that the main thread is complete.
      tasks.Signal();

      // Wait for all pending tasks.
      tasks.Wait();
    }
}
Brian Gideon
@Brian Gideon Thank you for such a thorough explanation and sample code. I have some questions though: 1) "The check of ExecutingTaskCount is not thread-safe": Why? I am only modifying it using Interlocked class. If I would still want to use it for some reason how would I go about it? When would you recommend to use Interlocked class?2) "... _finishedTaskAutoResetEvent is an AutoResetEvent signals can get lost because that WaitHandle does not maintain a count ...": What might be the cause of such a situation? Task throwing an unhandled exception and me not handling it for some reason?
Den
RE #1...To make `ExecutingTasksCount` thread-safe you will have to do a volatile read of `_executingTasksCount`. This can be done using the `Interlocked.CompareExchange` method or marking the variable as `volatile`.
Brian Gideon
RE #2...Imagine a hypothetical scenario (that is very unlikely) where all task threads get preempted between `DecCurrentTasksCount` and `_finishedTaskAutoResetEvent.Set`. I think your nested loops guard against any problems, but I am envisioning weird ways that they can behave. Again, I don't think there is actually any problem with that approach, but it is difficult to think about.
Brian Gideon