Disposing of thread with infinite loop

Question

I have an infinite loop that is used to consume items from a BlockingCollection.

public class MessageFileLogger
{
    private BlockingCollection<ILogItem> _messageQueue;
    private Thread _worker;
    private bool _enabled = false;
    public MessageFileLogger()
    {
         _worker = new Thread(LogMessage);
         _worker.IsBackground = true;
         _worker.Start();
    }

    private void LogMessage()
    {
        while (_enabled)
        {
            if (_messageQueue.Count > 0)
            {
                itm = _messageQueue.Take();
                processItem(itm);
            }
            else
            {
                Thread.Sleep(1000);
            }
        }
    }
}

which is referenced by another object that gets instantiated every minute or couple of minutes (could be moved out to 1 hour increments or such).

public class Helper
{
    MessageFileLogger _logger;
    public Helper(string logFilePath, LogMode logMode)
    {
        _logger = new MessageFileLogger(logFilePath, logMode);
        _logger.Enabled = true;
    }

    public void foo()
    {
    }
}

Question #1) What can I do to ensure that the thread is exited when the object that references it is no longer needed?

Note: Helper only needs to call foo, so once it no longer needs to call foo, the object can be garbage collected. So, incorporating a using statement with Helper is certainly a possibility.

Question #2) Does _messageQueue need to be disposed? If so, how do I dispose of it without it affecting the LogMessage thread? (I tried disposing of it while the thread was running and no surprise got an error).

I tried extending IDisposable (in MessageFileLogger):

public void Dispose()
{
   _enabled = false;
   _messageQueue.Dispose();
}

and I haven't had any issues with this but I'm not confident that I just haven't had an issue yet. Also, would this mean that Helper also needs to IDisposable and a using statement needs to be used with Helper?

Note: This question is based on the same code I had with another question of mine.

Answer 1

First off, your consumer shouldn't be calling Thread.Sleep. It also most certainly shouldn't be checking the count of the collection. The whole point of BlockingCollection is that when you call Take, it either gives you and item, or it waits until there is an item to give you, and then gives it to you. So you can just keep calling Take in a loop with nothing else. This prevents you from waiting some fraction of a second when there is already an item you could be processing.

Better still, you can simply use GetConsumingEnumerable to get a sequence of items.

Your consumer can now look like this:

foreach(var item in _messageQueue.GetConsumingEnumerable())
    processItem(item);

Additionally, BlockingCollection has built in support for indicating that the queue is done. Simply have the producer call CompleteAdding to indicate that no more items will be added. After doing that, once the queue is empty, the Enumerable will end, and the foreach loop will finish. The consumer can do any clean up it needs to at that point in time.

In addition to the fact that using the BlockingCollection to determine when you're done is just generally more convenient, it's also correct, unlike your code. Since _enabled isn't volatile, even though you're reading and writing to it from different threads, you're not introducing the proper memory barriers, so the consumer is likely to be reading a stale value of that variable for some time. When you use mechanisms from the BCL specifically designed to handle these types of multithreaded situations you can be sure that they'll be handled properly on your behalf, without you needing to think about them.