Ulf Honkanen Ulf Honkanen - 2 months ago 16
C# Question

Is this design thread safe?

In c#, the main class created a Logger object that will be accessed by many threads. The logger object looks like (simplified)

public sealed class Logger
{
private ConcurrentQueue<string> queue = new ConcurrentQueue<string>();

public Logger()
{
// create other objects here AND a thread that extracts
// from the queue and writes to a file
// because queue is thread safe this is perfectly ok
}

public void Log(string whatToLog)
{
// Now, is this safe? This method will be called by several threads
// perhaps at the same time

string s = whatToLog + " " + DateTime.Now.ToString();
queue.Enqueue(s);

// The thread created in the constructor will extract and log
}
}


Is this OK from a design point of view? My two questions are:


  • Is "string s = whatToLog + " " + DateTime.Now.ToString();" ok if this method is accessed by several threads at the same time? I guess yes because any thread will have its own copy of s, right?

  • If the Logger object is accessed by several threads at the same time using only the Log() method, is everything safe then?



Thanks

Answer

The ConcurrentQueue will make sure that the queue-part will be thread safe. The string s you construct will not make it more or less thread-safe

  • In the current form, you should instantiate the logger, and pass the reference to each thread that will use this class
  • Although thread-safe, it does not guarantee sequentiality of the items
  • Queues cannot grow infinitely, make sure that your mechanism to dequeue can keep up

Improvements:

  • Make the class static, easier access for several threads
  • Separate concerns on reading and writing; this can be done by making several essential function internal and placing classes in the same namespace
  • use C#6 string interpolation

Code with improvements

public static class Logger
{
    private static ConcurrentQueue<string> queue = new ConcurrentQueue<string>();

    public static void Log(string LogMessage)
    {
        // thread safe logging
        queue.Enqueue($"{LogMessage} {DateTime.Now}");
    }


    //dequeue only within namespace
    internal static string Dequeue() {
        string dequeuedItem;
        queue.TryDequeue(out dequeuedItem);
        return dequeuedItem;
    }
}

public class LoggerReader
{
    public LoggerReader()
    {
        // create other objects here AND a thread that extracts
        // from the queue and writes to a file
        // because queue is thread safe this is perfectly ok
        string logItem = Logger.Dequeue();
    }
}
Comments