views:

238

answers:

6

I'm trying to write a ThreadManager for my C# application. I create several threads:
One thread for my text writer.
One thread that monitors some statistics.
Multiple threads to perform a large sequence of calculations (up to 4 threads per core and I run my app on a 2x quad core server).

My application normally runs for up to 24 hours at a time, so all the threads get created in the beginning and they persist through the entire time the app runs.

I want to have a single place where I "register" all of my treads and when the application is shutting down I simply invoke a method and it goes through all of the registered threads and shuts them down.

For that purpose I have devised the following class:

public class ThreadManager
{
    private static Object _sync = new Object();
    private static ThreadManager _instance = null;
    private static List<Thread> _threads;
    private ThreadManager()
    {
        _threads = new List<Thread>();
    }

    public static ThreadManager Instance
    {
        get
        {
            lock (_sync)
            {
                if (_instance == null)
                {
                    _instance = new ThreadManager();
                }
            }
            return _instance;
        }
    }

    public void AddThread(Thread t)
    {
        lock (_sync)
        {
            _threads.Add(t);
        }
    }

    public void Shutdown()
    {
        lock (_sync)
        {
            foreach (Thread t in _threads)
            {
                t.Abort(); // does this also abort threads that are currently blocking?
            }
        }
    }
}

I want to ensure that all of my threads are killed so the application can close properly and shutting down in the middle of some computation is just fine too. Should I be aware of anything here? Is this approach good given my situation?

+7  A: 

If you set the threads to background threads, they will be killed when the application is shut down.

myThread.IsBackground = true;

obviously if you need the threads to finish before shutdown, this is not the solution you want.

Nate Heinrich
Thanks Nate, I do set all of my threads to background threads when I create them... does that mean that I don't need to abort them at all?
Lirik
yes, if they are set to background threads, closing the application will force the threads to exit.http://msdn.microsoft.com/en-us/library/system.threading.thread.isbackground.aspx
Nate Heinrich
+2  A: 

The only specific issue I know about is this one: http://www.bluebytesoftware.com/blog/2007/01/30/MonitorEnterThreadAbortsAndOrphanedLocks.aspx

But I'd avoid having to resort to a design like this. You could force each of your threads to check some flag regularly that it's time to shut down, and when shutting down, set that flag and wait for all threads to finish (with Join()). It feels a bit more like controlled shutdown that way.

jdv
+11  A: 

Aborting threads is what you do when all else fails. It is a dangerous thing to do which you should only do as a last resort. The correct way to do this is to make your threading logic so that every worker thread responds quickly and correctly when the main thread gives it the command to shut itself down.

Coincidentally, this is the subject of my blog this week.

http://blogs.msdn.com/ericlippert/archive/2010/02/22/should-i-specify-a-timeout.aspx

Eric Lippert
That thread abort exceptions are blocked during the execution of finally blocks is new to me. Thanks for adding this.
fatcat1111
@fatcat1111: Indeed, nothing whatsoever guarantees that an aborted thread will *ever* stop as a result of the abort. Just another reason to avoid Thread.Abort; it is both dangerous *and* unreliable.
Eric Lippert
Agreed, definitely a last resort option.
Jamie Keeling
@Jamie Keeling: No, that's exactly the point: because it is not reliable it is not a last resort. Last resort would be disconnecting all power sources to the host machine.
Jason
I didn't mean as a definitive last resort, disconnecting power is the ultimate fail safe. I was merely saying that Thread.Abort(); should only be really used if only truly necessary.
Jamie Keeling
+2  A: 

What if AddThread is called while your Shutdown is running?

When shutdown finishes, the thread waiting in AddThread will add a new thread to the collection. This could lead to hangs in your app.

Add a bool flag that you ever only set in Shutdown to protect against this.

bool shouldGoAway = false;
public void AddThread(Thread t)
{
    lock (_sync)
    {
        if( ! shouldGoAway )
            _threads.Add(t);
    }
}

public void Shutdown()
{
    lock (_sync)
    {
        shouldGoAway = true;
        foreach (Thread t in _threads)
        {
            t.Abort(); // does this also abort threads that are currently blocking?
        }
    }

Also you should not use static members - there is no reason for that as you have your Singleton instance.

.Abort() does not abort threads that are blocking in unmanaged space. So if you do that you need to use some other mechanism.

S.Skov
+1  A: 

If you don't care about the worker thread state then you can loop through _thread and abort:

void DieDieDie()
{
  foreach (Thread thread in _thread)
    {
      thread.Abort();
      thread.Join(); // if you need to wait for the thread to die
    }
}

In your case you can probably just abort them all and shutdown as they're just doing calculations. But if you need to wait for a database write operation or need to close an unmanaged resource then you either need to catch the ThreadAbortException or signal the threads to kill themselves gracefully.

ebpower
A: 

You want deferred thread cancellation, which basically means that the threads terminate themselves as opposed to a thread manager cancelling threads asynchronously, which is much more ill-defined and dangerous.

I you wanted to handle thread cancellation more elegantly than immediate termination, you can use signal handlers that are triggered by events outside the thread - by your thread manager perhaps.

Dan Mantyla