Thread safe logging class in C# to use from DLL

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 st

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 instance = new Lazy(() => 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 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! :)

StackExchange Code Review Q#106724, answer score: 12