Multi Threaded Error Logger

Question

I am trying to design a class that will log errors to a text file, the application is using threads heavily so it may be that 2 errors that need to be written at the same time. I know there are 3rd party solutions available but for this particular case I would like to roll my own. What I have at the moment seems to work, I was just curious to see if anyone has any tips,improvements etc. The main aim is to not Block the UI while waiting for errors to be logged.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Text;
using System.Data;
using System.Data.SqlClient;
using System.Reflection;
using System.Collections.Concurrent;
using System.Windows.Forms;
using System.Threading.Tasks;
 /// <summary>
 /// Error Logger will keep a list of all the exceptions and write them to a log periodically
 /// This is to ensure we dont get file locks trying to write to the file from multiple threads
 /// </summary>
 public class ErrorLogger : IDisposable
 {
 internal class ExceptionInfo
 {
 public Exception ExceptionObject { get; set; }
 public DateTime Date { get; set; }
 public int UserId { get; set; }
 }
 private static ErrorLogger _instance = null;
 public static ErrorLogger Instance
 {
 get
 {
 if (_instance == null)
 {
 _instance = new ErrorLogger();
 }
 return _instance;
 }
 }
 private Timer _timer = new Timer();
 private Task _writeTask = null;
 private ConcurrentQueue<ExceptionInfo> _exceptions = new ConcurrentQueue<ExceptionInfo>();
 public ErrorLogger()
 {
 _timer.Interval = 1000;
 _timer.Tick += new EventHandler(_timer_Tick);
 _timer.Start();
 }
 void _timer_Tick(object sender, EventArgs e)
 {
 if (_writeTask == null)
 {
 _writeTask = Task.Factory.StartNew(delegate
 {
 WriteLog();
 });
 }
 else if (_writeTask.IsCompleted && _exceptions.Count > 0)
 {
 _writeTask.Dispose();
 _writeTask = Task.Factory.StartNew(delegate
 {
 WriteLog();
 });
 }
 }
 public void LogErrror(Exception ex)
 {
 _exceptions.Enqueue(new ExceptionInfo() { ExceptionObject = ex, Date = DateTime.Now });
 }
 private void WriteLog()
 {
 string directory = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), @"First Choice Software\Pronet Outlook");
 string filePath = Path.Combine(directory, @"ErrorLog.txt");
 if (!Directory.Exists(directory))
 {
 Directory.CreateDirectory(directory);
 }
 if (!File.Exists(filePath))
 {
 File.Create(filePath).Close();
 }
 ExceptionInfo exInfo = null;
 System.IO.StreamWriter sw = null;
 try
 {
 sw = new System.IO.StreamWriter(filePath, true);
 while (_exceptions.TryDequeue(out exInfo))
 {
 string errorMessage = string.Format("{1}{5}{5}{5}{5}{5}{1}Version : {6}{1}Date : {0}{1}UserId : {2}{1}Message : {3}{1}Stack Trace : {4}", DateTime.Now,
 Environment.NewLine,
 exInfo.UserId,
 exInfo.ExceptionObject.Message,
 exInfo.ExceptionObject.StackTrace,
 "========",
 Assembly.GetExecutingAssembly().GetName().Version.ToString(4));
 Debug.Write(errorMessage);
 char[] c = errorMessage.ToCharArray();
 sw.Write(c, 0, c.Length);
 }
 }
 catch
 {
 }
 finally
 {
 if (sw != null)
 {
 sw.Close();
 sw.Dispose();
 }
 }
 }
 public void LogErrror(Exception ex, int pUserId)
 {
 _exceptions.Enqueue(new ExceptionInfo() { ExceptionObject = ex, Date = DateTime.Now,UserId = pUserId });
 }
 public void Dispose()
 {
 _exceptions.Enqueue(new ExceptionInfo() { ExceptionObject = new Exception("Disposing") });
 _timer.Stop();
 _timer.Dispose();
 if (_writeTask != null)
 {
 //If there are exceptions still to be logged, log them now, blocking UI now is ok
 if (_writeTask.IsCompleted && _exceptions.Count > 0)
 {
 WriteLog();
 }
 else if (!_writeTask.IsCompleted)
 {
 _writeTask.Wait();
 }
 _writeTask.Dispose();
 }
 _exceptions = null;
 _instance.Dispose();
 _instance = null;
 }
 }

• almaz: What are the reasons for rolling out your own logger? What functionality you find missing in existing solutions?
• Justim Odendaal: It's not that existing solutions don't offer functionality, but I'd rather not include a dependency just to log errors. I'm trying to keep the application size down to a minimum with as few 3rd part references as possible.
• Robert Snyder: I believe I would use a Queue and TraceSource. So you'd make your Logger class, have a method such as WriteLog(string) that method would add to your queue of strings. Then in either a different thread, or a timer, or something have it empty out your queue. But if you use TraceSource you can turn on and off tracing from a config file without having to recompile. And you can also change the location of your log file on a whim.
• svick: I think having a disposable singleton is a bad idea. Case in point: your Dispose() has a race condition (what happens if _writeTask is checked when it hasn't completed yet, but already exited the loop?). Also, I think it will cause stack overflow, since it's calling itself.
• Kiquenet: any final solution with full source code working ? maybe more complex sample for notify errors in each thread and after WaitAll shows a summary ?

Answer 1

Instead of creating a task to check the queue every second it's probably better to just to create it once and wait for the data (errors) to arrive. BlockingCollection will help here to provide automated blocking until the new data is available. I've simplified file operations here (each log entry will cause the file to be opened/closed), and it might be acceptable if you don't expect lots of exceptions from your application (as for me that's a reasonable assumption).

With a bit of more advanced code (e.g. using CancellationTokenSource) you could add a logic to keep the file opened for certain period, but I would initially go with simple solution, and change it only when it becomes the bottleneck....

P.S. I strongly dislike creation of exception in order to log the fact of disposition. It shows that your log doesn't contain exceptions only, so you should consider having exception object as an optional parameter in log entry.

public class ErrorLogger : IDisposable
{
 private class ExceptionInfo
 {
 public Exception ExceptionObject { get; set; }
 public DateTime Date { get; set; }
 public int UserId { get; set; }
 }
 private static readonly Lazy<ErrorLogger> _instance = new Lazy<ErrorLogger>(); //if you want to enforce the usage of singleton you should declare private default constructor.
 public static ErrorLogger Instance { get { return _instance.Value; } }
 private readonly Task _writeTask = Task.Run(WriteLog);
 private readonly BlockingCollection<ExceptionInfo> _exceptions = new BlockingCollection<ExceptionInfo>();
 public void LogError(Exception ex)
 {
 _exceptions.Add(new ExceptionInfo { ExceptionObject = ex, Date = DateTime.Now });
 }
 public void LogError(Exception ex, int pUserId)
 {
 _exceptions.Add(new ExceptionInfo { ExceptionObject = ex, Date = DateTime.Now, UserId = pUserId });
 }
 private void WriteLog()
 {
 string directory = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), @"First Choice Software\Pronet Outlook");
 string filePath = Path.Combine(directory, @"ErrorLog.txt");
 if (!Directory.Exists(directory))
 Directory.CreateDirectory(directory);
 foreach (var exInfo in _exceptions.GetConsumingEnumerable())
 {
 string errorMessage = string.Format(@"
========================================
Version : {4}
Date : {0}
UserId : {1}
Message : {2}
Stack Trace : {3}", DateTime.Now,
 exInfo.UserId,
 exInfo.ExceptionObject.Message,
 exInfo.ExceptionObject.StackTrace,
 Assembly.GetExecutingAssembly().GetName().Version.ToString(4));
 Debug.Write(errorMessage);
 File.AppendAllText(filePath, errorMessage)
 }
 }
 public void Dispose()
 {
 LogError(new Exception("Disposing"));
 _exceptions.CompleteAdding();
 _writeTask.Wait();
 _exceptions.Dispose();
 }
}

• svick: This probably won't matter much in a GUI application, but I think having a thread blocked most of the time is quite wasteful. And since the alternative here is not much more complicated, I fail to see why would this be a good idea.
• almaz: @svick I followed the pattern of saving a log to disk in a separate thread as it was in original solution. I think having one thread dedicated to this task is a better choice then creating (out of threadpool, but still...) the thread every second. I would suggest to use the standard logging library, but it seems not an option for asker.
• Jesse C. Slicer: Since the Task class implements IDisposable, shouldn't _writeTask be .Dispose()'d in your Dispose() method after the .Wait()? Same goes for the _exceptions BlockingCollection object.
• almaz: @JesseC.Slicer There is a nice article Do I need to dispose of Tasks? I agree that in order to be strict I should have BlockingCollection disposed.
• Jesse C. Slicer: @almaz Stephen Toub's pretty trustworthy, so kudos for posting that.