Simple MultiThread Safe Log Class

What is the best approach to creating a simple multithread safe logging class? Is something like this sufficient? How would I purge the log when it's initially cr开发者_StackOverflow中文版eated?

public class Logging
{
    public Logging()
    {
    }

    public void WriteToLog(string message)
    {
        object locker = new object();

        lock(locker)
        {
            StreamWriter SW;
            SW=File.AppendText("Data\\Log.txt");
            SW.WriteLine(message);
            SW.Close();
        }
    }
}

public partial class MainWindow : Window
{
    public static MainWindow Instance { get; private set; }
    public Logging Log { get; set; }

    public MainWindow()
    {
        Instance = this;
        Log = new Logging();
    }
}

Here is a sample for a Log implemented with the Producer/Consumer pattern (with .Net 4) using a BlockingCollection. The interface is :

namespace Log
{
    public interface ILogger
    {
        void WriteLine(string msg);
        void WriteError(string errorMsg);
        void WriteError(string errorObject, string errorAction, string errorMsg);
        void WriteWarning(string errorObject, string errorAction, string errorMsg);
    }
}

and the full class code is here :

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Log
{
    // Reentrant Logger written with Producer/Consumer pattern.
    // It creates a thread that receives write commands through a Queue (a BlockingCollection).
    // The user of this log has just to call Logger.WriteLine() and the log is transparently written asynchronously.

    public class Logger : ILogger
    {
        BlockingCollection<Param> bc = new BlockingCollection<Param>();

        // Constructor create the thread that wait for work on .GetConsumingEnumerable()
        public Logger()
        {
            Task.Factory.StartNew(() =>
                    {
                        foreach (Param p in bc.GetConsumingEnumerable())
                        {
                            switch (p.Ltype)
                            {
                                case Log.Param.LogType.Info:
                                    const string LINE_MSG = "[{0}] {1}";
                                    Console.WriteLine(String.Format(LINE_MSG, LogTimeStamp(), p.Msg));
                                    break;
                                case Log.Param.LogType.Warning:
                                    const string WARNING_MSG = "[{3}] * Warning {0} (Action {1} on {2})";
                                    Console.WriteLine(String.Format(WARNING_MSG, p.Msg, p.Action, p.Obj, LogTimeStamp()));
                                    break;
                                case Log.Param.LogType.Error:
                                    const string ERROR_MSG = "[{3}] *** Error {0} (Action {1} on {2})";
                                    Console.WriteLine(String.Format(ERROR_MSG, p.Msg, p.Action, p.Obj, LogTimeStamp()));
                                    break;
                                case Log.Param.LogType.SimpleError:
                                    const string ERROR_MSG_SIMPLE = "[{0}] *** Error {1}";
                                    Console.WriteLine(String.Format(ERROR_MSG_SIMPLE, LogTimeStamp(), p.Msg));
                                    break;
                                default:
                                    Console.WriteLine(String.Format(LINE_MSG, LogTimeStamp(), p.Msg));
                                    break;
                            }
                        }
                    });
        }

        ~Logger()
        {
            // Free the writing thread
            bc.CompleteAdding();
        }

        // Just call this method to log something (it will return quickly because it just queue the work with bc.Add(p))
        public void WriteLine(string msg)
        {
            Param p = new Param(Log.Param.LogType.Info, msg);
            bc.Add(p);
        }

        public void WriteError(string errorMsg)
        {
            Param p = new Param(Log.Param.LogType.SimpleError, errorMsg);
            bc.Add(p);
        }

        public void WriteError(string errorObject, string errorAction, string errorMsg)
        {
            Param p = new Param(Log.Param.LogType.Error, errorMsg, errorAction, errorObject);
            bc.Add(p);
        }

        public void WriteWarning(string errorObject, string errorAction, string errorMsg)
        {
            Param p = new Param(Log.Param.LogType.Warning, errorMsg, errorAction, errorObject);
            bc.Add(p);
        }

        string LogTimeStamp()
        {
            DateTime now = DateTime.Now;
            return now.ToShortTimeString();
        }

    }
}

In this sample, the internal Param class used to pass information to the writing thread through the BlockingCollection is :

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Log
{
    internal class Param
    {
        internal enum LogType { Info, Warning, Error, SimpleError };

        internal LogType Ltype { get; set; }  // Type of log
        internal string Msg { get; set; }     // Message
        internal string Action { get; set; }  // Action when error or warning occurs (optional)
        internal string Obj { get; set; }     // Object that was processed whend error or warning occurs (optional)

        internal Param()
        {
            Ltype = LogType.Info;
            Msg = "";
        }
        internal Param(LogType logType, string logMsg)
        {
            Ltype = logType;
            Msg = logMsg;
        }
        internal Param(LogType logType, string logMsg, string logAction, string logObj)
        {
            Ltype = logType;
            Msg = logMsg;
            Action = logAction;
            Obj = logObj;
        }
    }
}

No, you're creating a new lock object every time the method is called. If you want to ensure that only one thread at a time can execute the code in that function, then move locker out of the function, either to an instance or a static member. If this class is instantiated every time an entry is to be written, then locker should probably be static.

public class Logging
{
    public Logging()
    {
    }

    private static readonly object locker = new object();

    public void WriteToLog(string message)
    {
        lock(locker)
        {
            StreamWriter SW;
            SW=File.AppendText("Data\\Log.txt");
            SW.WriteLine(message);
            SW.Close();
        }
    }
}

Creating a thread-safe logging implementation using a single monitor (lock) is unlikely to yield positive results. While you could do this correctly, and several answers have been posted showing how, it would have a dramatic negative effect on performance since each object doing logging would have to synchronize with every other object doing logging. Get more than one or two threads doing this at the same time and suddenly you may spend more time waiting than processing.

The other problem you run into with the single monitor approach is that you have no guarantee that threads will acquire the lock in the order they initially requested it. So, the log entries may essentially appear out of order. That can be frustrating if you're using this for trace logging.

Multi-threading is hard. Approaching it lightly will always lead to bugs.

One approach to this problem would be to implement the Producer/Consumer pattern, wherein callers to the logger only need to write to a memory buffer and return immediately rather than wait for the logger to write to disk, thus drastically reducing the performance penalty. The logging framework would, on a separate thread, consume the log data and persist it.

you need to declare the sync object at the class level:

public class Logging 
{ 
    private static readonly object locker = new object(); 

    public Logging() 
    { 
    } 

    public void WriteToLog(string message) 
    { 
        lock(locker) 
        { 
            StreamWriter sw; 
            sw = File.AppendText("Data\\Log.txt"); 
            sw.WriteLine(message); 
            sw.Close(); 

            sw.Dispose();
        } 
    } 
} 

Might be better to declare your logging class as static, and the locking object as @Adam Robinson suggested.

The question uses File.AppendText which is not an asynchronous method, and other answers correctly show that using a lock is the way to do it.

However, in many real-world cases, using an asynchronous method is preferred so the caller doesn't have to wait for this to get written. A lock isn't useful in that case as it blocks the thread and also async methods are not allowed inside the lock block.

In such situation, you could use Semaphores (SemaphoreSlim class in C#) to achieve the same thing, but with the bonus of being asynchronous and allowing asynchronous functions to be called inside the lock zone.

Here's a quick sample of using a SemaphoreSlim as an asynchronous lock:

// a semaphore as a private field in Logging class:
private static SemaphoreSlim semaphore = new SemaphoreSlim(1, 1);

// Inside WriteToLog method:
try 
{
    await semaphore.WaitAsync();

    // Code to write log to file asynchronously
}
finally
{
    semaphore.Release();
}

Please note that it's good practice to always use semaphores in try..finally blocks, so even if the code throws an exception, the semaphore gets released correctly.