Thread safe logging class in C# to use from DLL

Question

I tried to sample up a logging class from this question and answer(I want to use this class from C# DLL). Made slight modification mainly to file name, and also how data is written. I am interested if this can be considered thread safe logging class? I am particularly concerned by the comment under the answer. See below. I didn't particularly understand the comment.

You should also ditch the TextWriter.SyncTextWriter if you are going this way. Every method call on SyncTextWriter takes a lock on the instance. This is not necessary if you are wrapping the calls to the TextWriter in your own Logger classs methods. If you are passing a raw SyncTextWriter` around you can deadlock like this: lock(textWriter){ textWriter.WriteLine("test"); // oops deadlock

Here is the class:

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

    static readonly TextWriter tw; 

    static Logger()
    {
         // Note1:  
         // My intent was that the log file name is same as the date on which the log
         // entry was added by the user - but apparently this approach as below is not
         // correct since it is in static constructor and the same date will be set for the file names even if log entries were added at different dates
         // Note2: The current answer doesn't address this, I would appreciate if someone can tell me where to move this code too to achieve what I want
         const string FMT = "yyyy-MM-dd";
         DateTime now1 = DateTime.Now;
         string strDate = now1.ToString(FMT);

        tw = TextWriter.Synchronized(File.AppendText(SPath() + "\\Log " + strDate + ".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)    {
      // 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} {2}", DateTime.Now.ToLongTimeString(),
          DateTime.Now.ToLongDateString(), logMessage);

       // Where to call flush??
       w.Flush();
     }
   }
}

PS. You can see classes like NLog have some requirements when they must be called from DLL(like config file must be in exe file which is calling DLL etc.). I'd like to avoid such things, as well as configurations which these classes require.

Answer 1

I don't think Log should be public, it is confusing. Me, as a client of your logging API, shouldn't need to create an instance of TextWriter to be able to log something. If I want to do some IO, I'll do it all by myself and won't pass by the logging API.

The naming is a little flawed. tw should be (I think), logTextWriter or something like that. Good variable name is worth more than the space you save by having bad ones. SPath should be GetLoggingPath or something like that.

If I understand your code correctly, if I leave the application running for 30 days, my log file will always be dated of the 1st day I opened the application, because of the static ctor, it may cause confusion.

In the Log method, there is a (very very very) small window for a bug. Instead of doing :

w.WriteLine("{0} {1} {2}", DateTime.Now.ToLongTimeString(), DateTime.Now.ToLongDateString(), logMessage);

you should do :

DateTime now = DateTime.Now;
w.WriteLine("{0} {1} {2}", now.ToLongTimeString(), now.ToLongDateString(), logMessage);

(If you don't want to consider the millisecond that could make DateTime.Now change, just consider I think it'd also be more readable.)

I'm no threading expert, but I know for a fact that your code is thread-safe, though there might (or not) be concurrency issues I'm not aware of if you have lot of calls on the Log method.

Finally, logging classes are usually interfaced. If you ever want to unit test your classes that uses the logging API, you don't want to be writing in a real file. Your class is static, meaning you can't interface it. This is a good opportunity to use the Singleton pattern :

public interface ILogger
{
    void Write(string logMessage);
}

public class Logger : ILogger
{
    private static readonly object _syncObject = new object();
    private static readonly Lazy<ILogger> instance = new Lazy<ILogger>(() => new Logger(),LazyThreadSafetyMode.ExecutionAndPublication);

    private readonly TextWriter textWriter; 

    public static ILogger Instance => instance.Value;

    private Logger()
    {
        const string FMT = "yyyy-MM-dd";
        DateTime now1 = DateTime.Now;
        string strDate = now1.ToString(FMT);

        textWriter = TextWriter.Synchronized(File.AppendText(SPath() + "\\Log " + strDate + ".txt")); 
    } 

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

    private void Log(string logMessage, TextWriter w)    {
        lock(_syncObject) {
            DateTime now = DateTime.Now;
            w.WriteLine("{0} {1} {2}", now.ToLongTimeString(), now.ToLongDateString(), logMessage);

            w.Flush();
        }
    }

    private static string GetPath()
    {
        return ConfigManager.GetAppSetting("logPath"); 
    }
}

Note the usage of Lazy<ILogger> which is a thread safe lazy loader generic utility to make sure the constructor isn't called twice by two different threads.

Using this interface, you can then use the dependancy injection to use your ILogger interface everywhere without caring about the implementation! :)

Answer 2

You're going to have a severe performance drag with all that writing to a text file constantly. I would recommend that you avoid that if at all possible and create an internal buffer.

private static List<string> _buffer = new List<string>;
private static int _maxBufferSize = 25; // number of messages
                                        // permitted in the
                                        // buffer at a time

public void Write(string logMessage)
{
    lock (_buffer)
    {
        _buffer.Add(logMessage);

        Save(_buffer);
    }
}

Why is this a superior solution? Because you can adjust just how frequently you write to the buffer. You could also extract this further:

private static void Save(List<string> buffer)
{
    if (buffer.Count > _maxBufferSize)
    {
        // write to log here
        buffer.Clear();
    }
}

It's up to you if this method is public or private, but either one should suffice.