Gfrktyl

I'm using log4net in my application and have a static Log class which creates an instance of ILog with a default log type, e.g CMS. The ILog instance is then used by the class to call the log4net logging methods passing in a LogItem type as the log message to enforce a strict logging format.

The user of the class can also change the logger targeted by the logger at runtime by setting a public static property LogType for example: Log.LogType = Enums.LogType.Database.

Here is the class:

/// <summary>

/// Log wrapper around ILog - enforces consistent log format.

/// </summary>

public static class Log

{

    private static ILog Logger = LogManager.GetLogger(LogType.CMS.ToString());



    /// <summary>

    /// Sets the Logger to be used.

    /// </summary>

    public static LogType LogType

    {

        set

        {

            Logger = LogManager.GetLogger(value.ToString());

        }

    }



    /// <summary>

    /// Logs information to the system.

    /// </summary>

    /// <param name="source">Source of the info.</param>

    /// <param name="description">Info description.</param>

    /// <param name="exc">Associated Exception object.</param>

    public static void Info(string source, string description, Exception exc = null)

    {

        Logger.Info(new LogItem() { Source = source, Description = description }, exc);

    }



    /// <summary>

    /// Logs a warning to the system.

    /// </summary>

    /// <param name="source">Source of the info.</param>

    /// <param name="description">Info description.</param>

    /// <param name="exc">Associated Exception object.</param>

    public static void Warn(string source, string description, Exception exc = null)

    {

        Logger.Warn(new LogItem() { Source = source, Description = description }, exc);

    }



    /// <summary>

    /// Logs an error to the system.

    /// </summary>

    /// <param name="source">Source of the info.</param>

    /// <param name="description">Info description.</param>

    /// <param name="exc">Associated Exception object.</param>

    public static void Error(string source, string description, Exception exc)

    {

        Logger.Error(new LogItem() { Source = source, Description = description }, exc);

    }

}

I'm concerned on whether the LogType prop would be thread safe, what would happen if this is changed by another class before the logger does its job?

I've read that log4net itself is thread safe and will block when calling the appenders - would having a single static class that everything logs through eventually cause a bottleneck?

This is not thread safe, just because the LogType static property. The basic problem of such design is that any log operation while setting the log type requires two calls and changes in global state. Imagine how an hypothetical client code would look like:

Log.LogType = LogType.Database;

//What happens between these two lines?

Log.Info("CodeReview", "Testing error logging", new Exception("Kaboom"));

The first line sets the global log type, and the second uses it, quite simple. But what happens if another thread runs similar code in between? LogType can be changed in the meanwhile (look at my comment in the code), changing the effective destination to the unsuspecting code. Thus, it's not thread safe. As you mention, since Log4Net in itself is thread safe, logs methods have no problems with this, but the destination can be somewhat surprising.

You mention a lock to overcome this in comments. Again an hypothetical usage could be like this:

lock (typeof(Log))

{

    Log.LogType = LogType.Database;

    Log.Info("CodeReview", "Testing error logging", new Exception("Kaboom"));

}

This solves the issue but at a high cost, the "fix" must be implemented in each and every call, instead of doing it centrally from within the Log class. Since the operation spans 2 methods, you can't lock within a method. Additionally this increases contention and reduces performance, as all logging is now syncronized and each thread must wait for the others to finish logging. While Log4Net could potentially do the same internally, it may also implement other optimizations, that such approach will nulify.

The root problem comes from the global state, the logger is a just stored in a static variable and can be overwriten by anyone, including between calls. A possible solution would be to pass the log type as a parameter and create a local logger for each call and get rid of the LogType property altogether:

public static void Info(string source, string description, Exception exc = null, LogType logType = LogType.CMS)

{

    ILog logger = LogManager.GetLogger(logType.ToString());

    logger.Info(new LogItem() { Source = source, Description = description }, exc);

}

This results in NO global state, which eliminates chances of races conditions, and also simplify client code, as now every log request is a single call to the appropriate method.