Lock() in multithreading program

Question

I have a simple program that simulates my error situation. I have a singleton class that gets a messages from several threads. The execution must be blocked until the function is executed.

class Program
{
    private static TestClass test;
    static void Main(string[] args)
    {
        Thread a = new Thread(TestFunctionB);
        a.Start();

        Thread b = new Thread(TestFunctionB);
        b.Start();
    }

    private static void TestFunctionB()
    {
        TestClass test = TestClass.Instance;
        for (int i = 0; i < 15; i++)
        {
            test.Handle(i, Thread.CurrentThread.ManagedThreadId);
        }
    }
}

class TestClass
{
    private readonly object _lockObject;
    private static TestClass _instance;

    private TestClass()
    {
        _lockObject = new object();
    }

    public static TestClass Instance
    {
        get { return _instance ?? (_instance = new TestClass()); }
    }

    private void RunLocked(Action action)
    {
        lock (_lockObject)
        {
            action.Invoke();
        }
    }

    public void Handle(int counter, int threadId)
    {
        Console.WriteLine("\nThreadId = {0}, counter = {1}\n", threadId, counter);

        RunLocked(() =>
                  {
                      Console.WriteLine("\nFunction Handle ThreadId = {0}, counter = {1}\n", threadId, counter);

                      for (int i = 0; i < 30; i++)
                      {
                          Console.WriteLine("Funktion Handle threadId = {0}, counter = {1}, i = {2}", threadId, counter, i);
                          //Thread.Sleep(100);
                      }

                  });

        Console.WriteLine("\nFunction Handle free ThreadId = {0}, counter = {1}\n", threadId, counter);

    }


}

`

I excpect that threads write the output one after another, but in the console the threads outputs are mixed. Is the lock statement not correct?

Answer 1

I don't know if it is your only problem but get { return _instance ?? (_instance = new TestClass()); } is not atomic, you may end up with more than one instance returned.

Use the Lazy<T> class to guarantee that only one instance of the singleton is created.

class TestClass
{
    private readonly object _lockObject;
    private readonly static Lazy<TestClass> _instance = new Lazy<TestClass>(x=> new TestClass());

    private TestClass()
    {
        _lockObject = new object();
    }

    public static TestClass Instance
    {
        get { return _instance.Value; }
    }
    ...
}

If you don't have access to .NET 4.0 or newer you will need to lock around your singleton creation.

class TestClass
{
    private readonly object _lockObject;
    private static readonly object _singletonLock = new Object();
    private static TestClass _instance;

    private TestClass()
    {
        _lockObject = new object();
    }

    public static TestClass Instance
    {
        get 
        { 
            if(_instance == null)
            {
                lock(_singletonLock)
                {
                    if(_instance == null)
                    {
                        _instance = new TestClass ();
                    }
                }
            }
            return _instance; 
        }
    }
    ...
}