C# - Make the static class thread safe [closed]

Question

I have a simple class that I use to logging errors and other informations from my applications.

I wondering what will happen when 2 or more threads of the same application would like to log something to file. What will happen when they call Log() function at the same time?

The more important question is how could I make it thread safe and safely use it in multithreaded applications?

public static class Logger          // UWAGA!!! - now not thread safe - tylko do aplikacji jednowątkowych
{
    // lockers
    private static readonly object _consoleLocker = new object();
    private static readonly object _diskDriveLocker = new object();

    private static LoggerLevel _logLevel = LoggerLevel.Trace;
    private static List<LoggerTarget> _logTargets = new List<LoggerTarget>() { GostcompUtils.LoggerTarget.File };

    private static string _className = "";
    private static string _fileName = "";


    public static LoggerLevel Level
    {
        get
        {
            return _logLevel;
        }
        set
        {
            _logLevel = value;
        }
    }

    public static List<LoggerTarget> Targets
    {
        get
        {
            return _logTargets;
        }
        set
        {
            _logTargets = value;
        }
    }

    public static LoggerTarget AddTarget
    {
        set
        {
            if (_logTargets != null)
                _logTargets.Add(value);
        }
    }

    public static LoggerTarget RemoveTarget
    {
        set
        {
            if (_logTargets != null && _logTargets.Contains(value))
                _logTargets.Remove(value);
        }
    }

    public static string ClassName
    {
        get
        {
            return _className;
        }
        set
        {
            _className = value;
        }
    }

    public static string FileName
    {
        get
        {
            return _fileName;
        }
        set
        {
            _fileName = value;
        }
    }



    public static void Log(string message, LoggerLevel level, Exception exception = null, string extraMessage = null) // najprostsza funkcjonalna wersja f. Log(), w tej wersji FileName, Level i Targets muszą być ustawione wcześniej
    {
        if ((int)level <= (int)Level)
        {
            if (_logTargets != null && _logTargets.Count() > 0)
            {
                if (string.IsNullOrEmpty(_fileName))
                    ProcessLogMessage(message, exception, _logTargets, level, null, extraMessage, "defaultLogFile.txt");
                else
                    ProcessLogMessage(message, exception, _logTargets, level, null, extraMessage, _fileName);
            }
            else
                throw new Exception("No targets set to log the message. Please do it with Targets or AddTarget propery or use another Log() method.");
        }
    }





    //
    // inne wersje metody Log()
    //

    // 1. zeby zalogowac do innego pliku pojedynczy log

    // 2. zeby zalogowac do konkretnego targeta innego niz zdefiniowane w klasie






    //
    // prostsze wersje funkcji do logowania
    //

    public static void Error(string message, Exception exception = null)
    {
        Log(message, LoggerLevel.Error, exception);
    }

    public static void Warn(string message, Exception exception = null)
    {
        Log(message, LoggerLevel.Warn, exception);
    }

    public static void Info(string message)
    {
        Log(message, LoggerLevel.Info);
    }

    public static void Debug(string message)
    {
        Log(message, LoggerLevel.Debug);
    }

    public static void Trace(string message)
    {
        Log(message, LoggerLevel.Trace);
    }




    static private void ProcessLogMessage(string message, Exception exception, List<LoggerTarget> logTargets, LoggerLevel logLevel, string className, string extraMessage, string fileName)
    {

        StringBuilder logMessage = PrepareTextOfLogMessage(message, exception, logLevel, className, extraMessage);

        string applicationDirectory = Path.GetDirectoryName(Assembly.GetExecutingAssembly().GetName().CodeBase);
        applicationDirectory = applicationDirectory.Replace("file:\\", "");
        string loggerFile = Path.Combine(applicationDirectory, fileName);

        if (logTargets.Contains(LoggerTarget.File))
        {
            lock (_diskDriveLocker)
            {
                WriteLogToFile(logMessage.ToString(), loggerFile);
            }
        }

        if (logTargets.Contains(LoggerTarget.Console))
        {
            lock (_consoleLocker)
            {
                Console.WriteLine(logMessage.ToString());
            }
        }
    }

    static private StringBuilder PrepareTextOfLogMessage(string message, Exception exception, LoggerLevel logLevel, string className, string extraMessage)
    {
        StringBuilder logMessage = new StringBuilder("");

        if (string.IsNullOrEmpty(className))
            logMessage.Append(String.Format("[{0}] [{1}]  {2} {3}", DateTime.Now.ToString(), logLevel.ToString(), message, extraMessage) + Environment.NewLine);
        else
            logMessage.Append(String.Format("[{0}] [{1}] <{2}>  {3} {4}", DateTime.Now.ToString(), logLevel.ToString(), className, message, extraMessage) + Environment.NewLine);

        if (exception != null)
        {
            logMessage.Append("***********************************************************************************************************" + Environment.NewLine);
            logMessage.Append("* EXCEPTION OCCURANCE:       " + Environment.NewLine);
            logMessage.Append("* ---------------------------------------------------------------------------------------------------------" + Environment.NewLine);
            logMessage.Append("* Exception Message:         " + Environment.NewLine + exception.Message + Environment.NewLine);
            logMessage.Append("* ---------------------------------------------------------------------------------------------------------" + Environment.NewLine);
            logMessage.Append("* Exception Source:          " + Environment.NewLine + exception.Source + Environment.NewLine);
            logMessage.Append("* ---------------------------------------------------------------------------------------------------------" + Environment.NewLine);
            logMessage.Append("* Exception StackTrace:      " + Environment.NewLine + exception.StackTrace + Environment.NewLine);
            logMessage.Append("* ---------------------------------------------------------------------------------------------------------" + Environment.NewLine);
            logMessage.Append("* Exception's InnerException:" + Environment.NewLine + exception.StackTrace + Environment.NewLine);
            logMessage.Append("***********************************************************************************************************" + Environment.NewLine);
            // pozostałe informacje z exceptiona
        }

        return logMessage;
    }


    private static void WriteLogToFile(string strLogMessage, string strLogFile)
    {
        using (StreamWriter swLog = new StreamWriter(strLogFile, true))
        {
            swLog.Write(strLogMessage);
        }
    }


}


public enum LoggerLevel
{
    Error = 0,
    Warn,
    Info,
    Debug,
    Trace,
}


public enum LoggerTarget
{
    File = 0,
    Console,
    MessageBox,
    Email,
    Database,
}

Answer 1

First of all System.Console is thread-safe itself then you do not need any synchronization lock then:

if (logTargets.Contains(LoggerTarget.Console))
    Console.WriteLine(logMessage.ToString());

However bigger issue (for me) is that you're using a static class. Static classes are bad for testing and they're not easy to extend. It would be better something like this:

public abstract class LogTarget
{
    protected internal abstract void Log(string message, 
        LoggerLevel level,
        Exception exception,
        string extraMessage);
}

public sealed class LogTargetCollection
{
    public void Add(LogTarget item)
    {
        if (item == null)
            throw new ArgumentNullException("item");

        lock (_items) _items.Add(item);
    }

    public void Remove(LogTarget item)
    {
        lock (_items) _items.Remove(item);
    }

    internal void Log(...)
    {
        lock (_items)
        {
            foreach (var item in items)
                item.Log(...);
        }
    }

    private readonly List<LogTarget> _items = new List<LogTarget>();
}

Note that each LogTarget is responsible for his own thread-safe implementation, if required. Keeping some static methods for compatibility with existing code:

public static class Logger
{
    public static LoggerTargetCollection Targets
    {
        get { return _targets; }
    }

    public static void Error(string message, Exception exception = null)
    {
        _logTargets.Log(message, LoggerLevel.Error, exception, null);
    }

    private static readonly LoggerTargetCollection _targets = new LoggerTargetCollection();
}

You will then have two derived classes ConsoleLogTarget and DiskLogTarget. Note that target specific properties (such as FileName) must stay on the right LogTarget.

One astonishing thing you should remove: AddTarget and RemoveTarget. Properties are not shortcuts for methods, they should be (from caller point of view) nothing more than fields, they shouldn't perform actions! Using LogTargetCollection you will expose proper methods to do that (note that you may not need to make LogTargetCollection thread-safe if collection can be frozen after initialization).

WriteLogToFile() can be simplified using File.AppendAllText(). Also note that you should absolutely handle errors here, log should not crash application!

One note about exception parameter of Log() method. There are wrapper exceptions that you should individually handle, not every root exception is available through InnerException property. Moreover it may be recursive then you may want to dump each exception in the chain.

Few notes about your Log() function:

if ((int)level <= (int)Level)

You can directly compare enums and you better to avoid indentation, simply revert condition and exit:

if (level > Level)
    return;

Even in your original code _logTargets cannot be null, moreover you're calling LINQ .Count() extension method, not reading List<T>.Count property. Also here I'd drop avoidable nesting and I'd use a more descriptive exception (instead of generic Exception), all together:

if (level > Level)
    return;

if (_logTargets.Count == 0)
    throw new InvalidOperationException("No targets set ...");

ProcessLogMessage(...);

In PrepareTextOfLogMessage() you call ToString() for String.Format() arguments. It's useless then simply drop it:

String.Format("[{0}] [{1}]  {2} {3}", DateTime.Now, logLevel, ...

I assume you cannot use C# 6 otherwise you also have interpolated strings:

$"[{DateTime.Now}] [{logLevel}]..."

You do not need to initialize StringBuilder with an empty string, it will force creation of an empty buffer while parameterless constructor will already allocate some space.

var logMessage = new StringBuilder();

You also calculate loggerFile each time. It's a waste of time, it has to be done in (LogTarget derived class) once first time you need it (you may use a Lazy<T> for this).

You're always using StringBuilder.Append() but StringBuilder has a method StringBuilder.AppendFormat() to append formatted strings and a method StringBuilder.AppendLine() to append lines (adding Environment.NewLine at the end). String concatenation is slow and you should avoid it when possible then this:

logMessage.Append("* Exception Message: " + Environment.NewLine + exception.Message + Environment.NewLine);

Knowing that \n is platform dependent (then it will be translated to Environment.NewLine then you may simply write:

logMessage.AppendFormat("* Exception Message:\n{0}\n", exception.Message);

Or:

logMessage.AppendLine($"* Exception Message:\n{exception.Message}");

StringBuilder supports a fluent interface then you may also write:

logMessage.AppendFormat("...")
    .AppendFormat("...")
    .AppendFormat("...");

Very last note: after these changes you do not have LogTarget as enum but you still have LogLevel. You declare it as:

public enum LoggerLevel
{
    Error = 0,
    Warn,
    Info,
    Debug,
    Trace,
}

To explicitly set first element to 0 is redundant (it's default behavior):

 public enum LoggerLevel
 {
     Error,