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;
        }
    }

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();
    }
}