Getting away from BackgroundWorker towards TPL for a logging class

Question

I currently have written a simple event logger in the view of the old Backgroundworker class. I am trying to convert it to a TPL implementation.

I do not have enough usage with threading in C# to really prefer one over the other but I do know that TPL is becoming more preferred to I wanted to stick with it as much as I can. Another reason is that with the current code I can not find an easy way to make the EventLog class thread safe. I am finding myself using BeginInvoke to write to the log from non UI threads which just seems messy to me.

So here is the original code.

public class EventLog
{
    public String LogPath { get; set; }
    public List<LogEvent> Events { get; private set; }

    public static EventLog Instance { get { return lazyInstance.Value; } }
    private static readonly Lazy<EventLog> lazyInstance = new Lazy<EventLog>(() => new EventLog());

    private EventLog()
    {
        Events = new List<LogEvent>();
        LogPath = Assembly.GetExecutingAssembly().CodeBase;
        LogPath = Path.GetDirectoryName(LogPath);
        LogPath = LogPath.Replace("file:\\", "");
        LogPath = LogPath + "\\Log.txt";
    }

    public override void publish(LogEvent newEvent)
    {
        Events.Add(newEvent);
        if (!LogEventWriter.Instance.IsBusy)
            LogEventWriter.Instance.RunWorkerAsync(LogPath);
        LogEventWriter.Instance.LogEvents.Add(newEvent);
    }
}

internal class LogEventWriter : BackgroundWorker
{
    public BlockingCollection<LogEvent> LogEvents { get; set; }

    public static LogEventWriter Instance { get { return lazyInstance.Value; } }
    private static readonly Lazy<LogEventWriter> lazyInstance = new Lazy<LogEventWriter>(() => new LogEventWriter());

    private LogEventWriter()
    {
        WorkerSupportsCancellation = true;
        LogEvents = new BlockingCollection<LogEvent>();
    }

    protected override void OnDoWork(DoWorkEventArgs e)
    {
        if (e.Argument != null && e.Argument is String)
        {
            String logPath = (String)e.Argument;
            using (StreamWriter logFile = new StreamWriter(logPath, true))
            {
                while (!CancellationPending)
                {
                    LogEvent anEvent = LogEvents.Take();
                    logFile.WriteLine(anEvent.Message);
                    logFile.Flush();
                    if (anEvent.Message.Contains("Application Terminated"))
                        break;
                }
                logFile.Close();
            }
        }
        e.Cancel = true;
    }
}

My current train of thought for the log is to write the log to a file ASAP in case of a system failure so that the log will have as much information as it can. This is what the Backgroundworker is for. I also just keep a List<LogEvent> in the EventLog class so that the user might be able to search the current log for specific events (not fully implemented/polished off).

Here is my current TPL solution. I have tried as best I can to wrap the logging functionality into Tasks but it I still feel like I should have a function akin to publish and not have to directly place LogEvents into a BlockingCollection<> so that I can run the logging on a separate thread from the main UI.

Also is there a cleaner way to stop the Tasks without having to send a "special" LogEvent to them to break from their loops?

public class EventLog
{
    public static EventLog Instance { get { return lazyInstance.Value; } }
    private static readonly Lazy<EventLog> lazyInstance = new Lazy<EventLog>(() => new EventLog());

    public String LogPath { get; set; }
    public ConcurrentQueue<LogEvent> Events { get; set; }

    private EventLog()
    {
        Events = new ConcurrentQueue<LogEvent>();
        WriteQueue = new BlockingCollection<LogEvent>();
        LogEventQueue = new BlockingCollection<LogEvent>();

        LogPath = Assembly.GetExecutingAssembly().CodeBase;
        LogPath = Path.GetDirectoryName(LogPath);
        LogPath = LogPath.Replace("file:\\", "");
        LogPath = LogPath + "\\LogASDF.txt";

        StartManager();
        StartWriter();
    }

    public BlockingCollection<LogEvent> LogEventQueue { get; set; }
    private void StartManager()
    {
        var writeTask = Task.Factory.StartNew(() =>
        {
            while (true)
            {
                LogEvent anEvent = LogEventQueue.Take();
                Events.Enqueue(anEvent);
                WriteQueue.Add(anEvent);
                if (anEvent.Message.Contains("Application Terminated"))
                    break;
            }
        });
    }

    private BlockingCollection<LogEvent> WriteQueue { get; set; }
    private void StartWriter()
    {
        var writeTask = Task.Factory.StartNew(() =>
        {
            using (StreamWriter logFile = new StreamWriter(LogPath, true))
            {
                while(true)
                {
                    LogEvent anEvent = WriteQueue.Take();
                    logFile.WriteLine(anEvent.Message);
                    logFile.Flush();
                    if (anEvent.Message.Contains("Application Terminated"))
                        break;
                }
                logFile.Close();
            }
        });
    }
}

1. How can I properly use CancellationToken to cancel both of the tasks? I do not see how if BlockingCollection is blocking, I always have to "pulse" the collection to get it to unblock.
2. Is there a "cleaner" way to insert an LogEvent into the log without having to directly insert it into LogEventQueue?

Answer 1

Right now your code isn't thread-safe as you have this:

public List<LogEvent> Events { get; private set; }

List<T> isn't thread safe, and can be changed from outer code. And I can't see is it used at all.

Also, you really should use CancellationToken in your code because of in other case you can get troubles: for example, you have 5 messages in queue, and you decide to cancel your work. In this case check for Shutdown Log will break loop only after some time, which is confusing for end-user of your class.

Also, there is an overload for BlockingCollection<T>.Take Method with CancellationToken, but in case of cancel you'll get to catch the OperationCanceledException:

try
{
    LogEvent anEvent = WriteQueue.Take(CancellationPending);
}
catch (OperationCanceledException ex)
{
    // handle stop here;
}

Infinite loop in very bad practice in multi-threading, I suggest not to use it.