How best to dispose a thread that is sleeping because of AutoResetEvent.WaitOne()

Question

I have a windows service that sends email in a one of 5 threads (done to increase the speed the service can send email):

private AutoResetEvent block;
private ThreadedQueue<Message> messageQueue;        

private void DoSend()
{
    try
    {   
        while(!this.disposing)
        {
            this.block.WaitOne();

            Message message = null; 
            if (this.messageQueue.TryDequeue(out message))
            {                       
                this.block.Set();
            }                   

            if(message != null)
            {
                this.Send(message);                 
            }
        }
    }
    catch(Exception ex)
    {
        // Log
    }
}

I have a Queue method that adds one or more new message to the messageQueue and calls block.Set() so that one of the 5 threads can send the message. When one of the threads is allowed to run, so long as there are messages in the queue, block.Set() is called so that the next message can be de-queued and another of 5 threads will work to send it. And so on, until the queue is empty. This all works OK.

However when I dispose my object, I set the disposing variable and then for each thread:

if(thread.ThreadState == ThreadState.Running)
{
    thread.Join();
}
else if(thread.ThreadState == ThreadState.WaitSleepJoin)
{
    thread.Abort();
}

Most of the time, the threads are sleeping due to the block.WaitOne and so the above code aborts the thread. However this causes thread abort exceptions to be logged. I could catch thread abort exceptions separately to other exceptions and choose not to log, but it doesn't seem very clean.

What is the best way to clean up these threads without causing this excess logging?

UPDATE:

I've changed the above to:

private ManualResetEvent block;
private ThreadedQueue<Message> messageQueue;        

private void DoSend()
{
    try
    {   
        while(!this.disposing)
        {
            this.block.WaitOne();

            Message message = null; 
            if (!this.messageQueue.TryDequeue(out message) && !this.disposing)
            {                       
                // There's nothing else to send for now to block the sending threads
                // unless we're disposing as we want the other threads to exit too
                this.block.Reset();
            }                   

            if(message != null)
            {
                this.Send(message);                 
            }
        }
    }
    catch(Exception ex)
    {
        // Log
    }
}

public void Dispose()
{           
    this.disposing = true;
    this.block.Set();           
    foreach(Thread thread in this.sendingThreads) {             
        thread.Join();
    }
    this.block.Dispose();
    this.sendingThreads = null;
}

Thanks for the help.

Create a special "message of death" that causes any thread that receives it to terminate. Then queue as many messages of death as you have threads that need to be terminated. — David Schwartz, Feb 04 '13 at 10:51
..or arrange for any thread that gets the "message of death" to requeue it before terminating. This will kill all threads with one message. — Martin James, Feb 04 '13 at 12:26

score 2 · Accepted Answer · answered Feb 04 '13 at 13:18

2

You are playing a very dangerous game. Your code is particularly prone to deadlock. You'll see the thread state as ThreadState.Running and the thread calls WaitOne() a microsecond later. Your Join() call will deadlock and never return.

You can get a thread that's blocked on a WaitOne() call to unblock by disposing the AutoResetEvent. That will throw a predicable exception, ObjectDisposedException, one you can catch. Use another ManualResetEvent to signal the thread to exit. No need for Thread.Abort() that way.

answered Feb 04 '13 at 13:18

Hans Passant

922,412
146
1,693
2,536

Good observation. Yes that is possible. I think the ThreadState.Running if needs to be removed. With you're proposed solution; are you suggesting I dispose the AutoResetEvent object and catch the resulting exception. Why do I need another ManualResetEvent, surely the above code will exit once it encounters the ObjectDisposedException? – Lawrence Phillips Feb 04 '13 at 13:52
1

Calling block.Dispose() does not cause the block on the threads to be release. They remain asleep. – Lawrence Phillips Feb 04 '13 at 14:28
1

Disposing the ARE is not enough, the thread might not be blocking on it. You need an extra signal to get it to see that it needs to exit its processing loop. Use the debugger to see what it is doing. Debug + Windows + Threads. – Hans Passant Feb 04 '13 at 14:33

score 1 · Answer 2 · answered Feb 05 '13 at 01:50

1

Use BlockingCollection instead. it will produce simple clean and short code which can be understood, managed and debugged...

one producer five consumers... threading 101.

http://msdn.microsoft.com/en-us/library/dd267312.aspx

answered Feb 05 '13 at 01:50

Boppity Bop

9,613
13
72
151

How best to dispose a thread that is sleeping because of AutoResetEvent.WaitOne()

2 Answers2