Thread safe logging class implementation

Question

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

Answer 1

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?