C# Threading Patterns - is this a good idea?

Question

I was playing with a project of mine today and found an interesting little snippet, given the following pattern, you can safely cleanup a thread, even if it's forced to close early. My project is a network server where it spawns a new thread for each client. I've found this useful for early termination from the remote side, but also from the local side (I can just call .Abort() from inside my processing code).

Are there any problems you can see with this, or any suggestions you'd make to anyone looking at a similar approach?

Test case follows:

---

using System;
using System.Threading;

class Program
{
 static Thread t1 = new Thread(thread1);
 static Thread t2 = new Thread(thread2);

 public static void Main(string[] args)
 {
  t1.Start();
  t2.Start();
  t1.Join();
 }

 public static void thread1() {
  try {
   // Do our work here, for this test just look busy.
   while(true) {
    Thread.Sleep(100);
   }
  } finally {
   Console.WriteLine("We're exiting thread1 cleanly.\n");
   // Do any cleanup that might be needed here.
  }
 }

 public static void thread2() {
  Thread.Sleep(500);
  t1.Abort();
 }
}

For reference, without the try/finally block, the thread just dies as one would expect.

Answer 1

Whether or not this will "safely" cleanup a thread cannot be discerned from a general code sample unfortunately. It's highly dependent upon the actual code that is executed within the thread. There are multiple issues you must consider. Each represents a potential bug in the code.

1. If the thread is currently in native code, it will not immediately respect the Thread.Abort call. It will do all of the work it wants to do in native code and will not throw until the code returns back to managed. Until this happens thread2 will hang.
2. Any native resources that are not freed in a finally block will be leaked in this scenario. All native resources should be freed in a finally block but not all code does this and it's an issue to consider.
3. Any locks that are not freed in a finally block will remain in a lock'd state and can lead to future dead locks.

There are other issues which are slipping my mind at the moment. But hopefully this will give you some guidance with your application.

Answer 2

Aborting another thread at all is just a bad idea unless the whole application is coming down. It's too easy to leave your program in an unknown state. Aborting your own thread is occasionally useful - ASP.NET throws a ThreadAbortException if you want to prematurely end the response, for example - but it's not a terribly nice design.

Safe clean-up of a thread should be mutual - there should be some shared flag requesting that the thread shuts down. The thread should check that flag periodically and quit appropriately.

Answer 3

It is generally not a good idea to abort threads. What you can do is poll for a stopRequested flag which can be set from other threads. Below is a sample WorkerThread class for your reference. For more information on how to use it, please refer to http://devpinoy.org/blogs/jakelite/archive/2008/12/20/threading-patterns-the-worker-thread-pattern.aspx

public abstract class WorkerThreadBase : IDisposable
{
    private Thread _workerThread;
    protected internal ManualResetEvent _stopping;
    protected internal ManualResetEvent _stopped;
    private bool _disposed;
    private bool _disposing;
    private string _name;

    protected WorkerThreadBase()
        : this(null, ThreadPriority.Normal)
    {
    }

    protected WorkerThreadBase(string name)
        : this(name, ThreadPriority.Normal)
    {
    }

    protected WorkerThreadBase(string name,
        ThreadPriority priority)
        : this(name, priority, false)
    {
    }

    protected WorkerThreadBase(string name,
        ThreadPriority priority,
        bool isBackground)
    {
        _disposing = false;
        _disposed = false;
        _stopping = new ManualResetEvent(false);
        _stopped = new ManualResetEvent(false);

        _name = name == null ? GetType().Name : name; ;
        _workerThread = new Thread(threadProc);
        _workerThread.Name = _name;
        _workerThread.Priority = priority;
        _workerThread.IsBackground = isBackground;
    }

    protected bool StopRequested
    {
        get { return _stopping.WaitOne(1, true); }
    }

    protected bool Disposing
    {
        get { return _disposing; }
    }

    protected bool Disposed
    {
        get { return _disposed; }
    }

    public string Name
    {
        get { return _name; }            
    }   

    public void Start()
    {
        ThrowIfDisposedOrDisposing();
        _workerThread.Start();
    }

    public void Stop()
    {
        ThrowIfDisposedOrDisposing();
        _stopping.Set();
        _stopped.WaitOne();
    }

    public void WaitForExit()
    {
        ThrowIfDisposedOrDisposing();            
        _stopped.WaitOne();
    }

    #region IDisposable Members

    public void Dispose()
    {
        dispose(true);
    }

    #endregion

    public static void WaitAll(params WorkerThreadBase[] threads)
    { 
        WaitHandle.WaitAll(
            Array.ConvertAll<WorkerThreadBase, WaitHandle>(
                threads,
                delegate(WorkerThreadBase workerThread)
                { return workerThread._stopped; }));
    }

    public static void WaitAny(params WorkerThreadBase[] threads)
    {
        WaitHandle.WaitAny(
            Array.ConvertAll<WorkerThreadBase, WaitHandle>(
                threads,
                delegate(WorkerThreadBase workerThread)
                { return workerThread._stopped; }));
    }

    protected virtual void Dispose(bool disposing)
    {
        //stop the thread;
        Stop();

        //make sure the thread joins the main thread
        _workerThread.Join(1000);

        //dispose of the waithandles
        DisposeWaitHandle(_stopping);
        DisposeWaitHandle(_stopped);
    }

    protected void ThrowIfDisposedOrDisposing()
    {
        if (_disposing)
        {
            throw new InvalidOperationException(
                Properties.Resources.ERROR_OBJECT_DISPOSING);
        }

        if (_disposed)
        {
            throw new ObjectDisposedException(
                GetType().Name,
                Properties.Resources.ERROR_OBJECT_DISPOSED);
        }
    }

    protected void DisposeWaitHandle(WaitHandle waitHandle)
    {
        if (waitHandle != null)
        {
            waitHandle.Close();
            waitHandle = null;
        }
    }

    protected abstract void Work();

    private void dispose(bool disposing)
    {
        //do nothing if disposed more than once
        if (_disposed)
        {
            return;
        }

        if (disposing)
        {
            _disposing = disposing;

            Dispose(disposing);

            _disposing = false;
            //mark as disposed
            _disposed = true;
        }
    }

    private void threadProc()
    {
        Work();
        _stopped.Set();
    }        
}