1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ enforces consistent log format - not really, it just provides three APIs but it doesn't enforce actually anything because anyone could use LogManager.GetLogger(...); anywhere in your application and do what they desire. \$\endgroup\$ – t3chb0t Sep 28 '18 at 9:06
  • 1
    \$\begingroup\$ This is also not thread-safe because if two threads use the LogType property at the same time one of them will overwrite the otherone and you'll be logging information from a different thread in the wrong log. You'd better take a look a dependency injection and pass loggers this way. \$\endgroup\$ – t3chb0t Sep 28 '18 at 9:09
  • \$\begingroup\$ @t3chb0t - Wouldn't the other projects in the solution need a reference to log4net to do that? The log class is in it's own project, other projects would not be able to directly call LogManager without going to the effort of installing log4net, unless I'm missing something. Regarding LogType this was my concern, how would this work with DI? Can you inject property values on the fly? Or is there a way to make it thread safe with a lock maybe? Thanks \$\endgroup\$ – DGibbs Sep 28 '18 at 9:19
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.