How to make a class Thread Safe

Question

I am writing a C# application. I have (a kind of) logging class. and this logging class will be used by many threads. How to make this class thread safe? Should I make it as singleton? what are the best practices there? Is there a document that I can read about how to make it thread-safe?

Thanks

Answer 1

use lock() so that multiple threads will not use the logging at the same time

Answer 2

• Try and do most computation using local variables and then alter the state of the object in one quick locked block.
• Keep in mind that some variables might change between when you read them and when you get altering the state.

Answer 3

I would use an off the shelf logger for this, since there are several that are rock solid and simple to use. No need to roll your own. I reccomend Log4Net.

Answer 4

In C#, any object can be used to protect a "critical section", or in other words, code that must not be executed by two threads at the same time.

For example, the following will synchronize access to the SharedLogger.Write method, so only a single thread is logging a message at any given time.

public class SharedLogger : ILogger
{
   public static SharedLogger Instance = new SharedLogger();

   public void Write(string s)
   {
      lock (_lock)
      {
         _writer.Write(s);
      }
   }

   private SharedLogger() 
   { 
      _writer = new LogWriter();
   }

   private object _lock;
   private LogWriter _writer;
}

Answer 5

Pursuant to BCS' answer:

BCS is describing the case of a stateless object. Such an object is intrinsically thread safe because it has no variables of it's own that can be clobbered by calls from different theads.

The logger described does have a file handle (sorry, not C# user, maybe it's called IDiskFileResource or some such MS-ism) which it must serialize use of.

So, separate the storage of messages from the logic that writes them to the log file. The logic should only operate on one message at a time.

One way to do it is: if the logger object were to keep a queue of message objects, and the logger object merely has the logic to pop a message off the queue, then extract the useful stuff from a message object, then write that to the log, then look for another message in the queue - then you can make that thread safe by having the queue's add/remove/queue_size/etc operations thread safe. It would require the logger class, a message class and a thread safe queue (which is probably a third class, an instance of which is a member variable of the logger class).

Answer 6

I'm not sure I can add anything to what has already been said about making a logging class thread-safe. As has been stated, to do this you must synchronize access to the resource, i.e., the log file, so that only one thread attempts to log to it at a time. The C# lock keyword is the proper way to do this.

However, I will address (1) the singleton approach and (2) the usability of the approach that you ultimately decide to use.

(1) If your application writes all of its log messages to a single log file, then the singleton pattern is definitely the route to go. The log file will be opened at startup and closed at shutdown, and the singleton pattern fits this concept of operations perfectly. As @dtb pointed out, though, remember that making a class a singleton does not guarantee thread safety. Use the lock keyword for that.

(2) As for the usability of the approach, consider this suggested solution:

public class SharedLogger : ILogger
{
   public static SharedLogger Instance = new SharedLogger();
   public void Write(string s)
   {
      lock (_lock)
      {
         _writer.Write(s);
      }
   }
   private SharedLogger()
   {
       _writer = new LogWriter();
   }
   private object _lock;
   private LogWriter _writer;
}

Let me first say that this approach is generally ok. It defines a singleton instance of SharedLogger via the Instance static variable and prevents others from instantiating the class by virtue of the private constructor. This is the essence of the singleton pattern, but I would strongly recommend reading and following Jon Skeet's advice regarding singletons in C# before going too far.

However, what I want to focus on is the usability of this solution. By 'usability,' I'm referring to the way one would use this implementation to log a message. Consider what the invocation looks like:

SharedLogger.Instance.Write("log message");

That whole 'Instance' part just looks wrong, but there's no way to avoid it given the implementation. Instead, consider this alternative:

public static class SharedLogger : ILogger
{
   private static LogWriter _writer = new LogWriter();
   private static object _lock = new object();
   public static void Write(string s)
   {
       lock (_lock)
       {
           _writer.Write(s);
       }
   }
}

Notice that the class is now static, which means that all of the its members and methods have to be static. It's not substantively different from the earlier example, but consider its usage.

SharedLogger.Write("log message");

That's much simpler to code against.

The point is not to denigrate the former solution, but to suggest that the usability of whatever solution you choose is an important aspect not to be overlooked. A good, usable API can make code simpler to write, more elegant, and easier to maintain.