Thread safe logging class implementation

Would the following be the correct way to implement a fairly straightforward thread-safe logging class?

I know that I never explicitly close the TextWriter, would that be a problem?

When I initially used the TextWriter.Synchronized method, it did not seem to work until I initialized it in a static constructor and made it readonly like so:

public static class Logger
{
    static readonly TextWriter tw; 

    static Logger()
    {
        tw = TextWriter.Synchronized(File.AppendText(SPath() + "\\Log.txt")); 
    }

    public static string SPath()
    {
        return ConfigManager.GetAppSetting("logPath"); 
    }

    public static void Write(string logMessage)
    {
        try
        {
            Log(logMessage, tw);
        }
        catch (IOException e)
        {
            tw.Close();
        }
    }

    public static void Log(string logMessage, TextWriter w)
    {
        w.WriteLine("{0} {1}", DateTime.Now.ToLongTimeString(),
            DateTime.Now.ToLongDateString());
        w.WriteLine("  :");
        w.WriteLine("  :{0}", logMessage);
        w.WriteLine("-------------------------------");

        // Update the underlying file.
        w.Flush();
    }
}

I'm going to take a completely different approach here than the other answers and assume you actually want to learn how to write better thread-aware code, and are not looking for 3rd party suggestions from us (even though you may actually end up using one.)

As others have said, you are creating a thread safe TextWriter which means calls to WriteLine are thread-safe, that doesn't mean that a bunch of calls to WriteLine are going to be performed as an atomic operation. By that I mean there is no guarantee that the four WriteLine calls are going to happen in sequence. You may have a thread-safe TextWriter, but you don't have a thread-safe Logger.Log method ;) Why? Because at any point during those four calls, another thread might decide to call Log also. This means your WriteLine calls will be out of sync. The way to fix this is by using a lock statement like so:

private static readonly object _syncObject = new object();

public static void Log(string logMessage, TextWriter w)    {
   // only one thread can own this lock, so other threads
   // entering this method will wait here until lock is
   // available.
   lock(_syncObject) {
      w.WriteLine("{0} {1}", DateTime.Now.ToLongTimeString(),
          DateTime.Now.ToLongDateString());
      w.WriteLine("  :");
      w.WriteLine("  :{0}", logMessage);
      w.WriteLine("-------------------------------");
      // Update the underlying file.
      w.Flush();
   }
}

So, now you have a thread-safe TextWriter AND a thread-safe Logger.

Make sense?

Someone pointed me to this post while discussing some logging issues today. We already have pretty good answers here, but I'm adding my answer just to show a simpler version of the Logger class which does the exact same thing, in completely Threadsafe way.
One main thing to notice here is, no TextWriter.Synchronized is required for thread safety, as we are writing the file within a proper lock.

Note: This has already been discussed in the comments section of x0n's answer.

public static class Logger
{
    static readonly object _locker = new object();

    public static void Log(string logMessage)
    {
        try
        {
            var logFilePath = Path.Combine(@"C:\YourLogDirectoryHere", "Log.txt");
            //Use this for daily log files : "Log" + DateTime.Now.ToString("yyyy-MM-dd") + ".txt";
            WriteToLog(logMessage, logFilePath);
        }
        catch (Exception e)
        {
            //log log-exception somewhere else if required!
        }
    }

    static void WriteToLog(string logMessage, string logFilePath)
    {
        lock (_locker)
        {
            File.AppendAllText(logFilePath,
                    string.Format("Logged on: {1} at: {2}{0}Message: {3}{0}--------------------{0}", 
                    Environment.NewLine, DateTime.Now.ToLongDateString(),
                    DateTime.Now.ToLongTimeString(), logMessage));
        }
    }
}

To log something, simply call as

Logger.Log("Some important event has occurred!");

And it will make a log entry like this

Logged on: 07 October 2015 at: 02:11:23
Message: Some important event has occurred!
--------------------

While calling TextWriter.Synchronized will protect that single instance of TextWriter, it will not synchronize your writes so that one "Log" call stays together inside of the file.

If you call Write (or Log using the internal TextWriter instance) from multiple threads, the individual WriteLine calls may be interwoven, making your date and time stamps unusable.

I would personally use a third party logging solution that already exists for this. If that is not an option, synchronizing this yourself (even with a simple lock) will likely be more useful than using the framework's TextWriter.Synchronized wrapper.

Thread safe logging class implementation

Related

Recent Posts