Question

I have been reading a lot of articles about Singleton online, but most of the articles out only demonstrate creating simple objects with properly locking to ensure thread safe without race condition.

Simple enough to demonstrate, I put a file logging method in there. It will work for 10-20 concurrent threads but when I try 100 threads, it will fail giving "The process cannot access the file because it is being used by another process".

public class Logger
{

    private string _path = "c:\\temp\\MyTestLog.txt";

    // Singleton with padlock for DCL
    private volatile static Logger _instance;
    private static readonly object  _padLock = new Object();

    // Singleton
    public static Logger Instance()
    {
        // Implement Double Check Locking (MSDN - Thread-safe)
        if (_instance == null)
        {
            lock (_padLock)
            {
                if (_instance == null)
                    _instance = new Logger();
            }
        }

        return _instance;
    }

    // Simple WriteLog 
    public void WriteLog(string text, out bool result)
    {
        try
        {             
            // Write to file 
            using (Stream stream = File.Open(_path, FileMode.Append))
            {
                using (TextWriter writer = new StreamWriter(stream))
                {
                    writer.WriteLine(text);
                }
            }

            Debug.WriteLine(text);
            result = true;
        }
        catch (Exception ex)
        {
            Debug.WriteLine(ex.ToString());
            result = false;
        }
    }

}

To call this from client, I setup a simple Windows Form and Threading to generate multi-threads by calling:

Logger.Instance().WriteLog("test", out result);

Well, I know the reason why it failed because the previous _instance is passed to the next thread where the inner file operation wasn't completed that causes the error. But not sure how to solve this.

I also tried this.. If I replace the File IO writing with SqlConnection, I will get "The connection was not closed. The connection's current state is open" instead of "The process cannot access the file because it is being used by another process". Same idea. Of course you will use Connection Pool instead of Singleton but just for my own curiosity.

I read "C# in Depth" article and tried different ways and didn't work. Even with .NET 4.0 Lazy doesn't solve the problem. The only workaround successfully is break the Singleton rule by using [ThreadStatic] attribute on _instance and DCL is no longer needed. But then it turns the class into a multi-threaded class with lazy initialization, not real Singleton anymore.

So, I am wondering.. how does Singleton solve this problem?

Was it helpful?

Solution

As stated in the comments, your problem is that WriteLog is not thread safe at all.

Having a singleton does not mean that every single method of that class will be in the same thread.

Even if you only have one instance, its methods can still be called by multiple threads at the same time.

Solution:

Put some synchronization logic in your WriteLog method. Lock statement, ReaderWriterLockSlim... the choice is all yours!

I recommend this article if you want to know more about threading in c#:

http://www.albahari.com/threading/

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top