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.

Answer 1

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.

Answer 2

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