0

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?

EluciusFTW
  • 2,565
  • 6
  • 42
  • 59
kyy8080
  • 47
  • 7
  • What do you expect to happens? `lock` ensures what for duration of delegate no other thread will obtain it (but doesn't guarantee anything before or after it). So first thread will do complete cycle (30 iterations), then second. What do you want? To print single message? Exactly one from one thread and one from another? – Sinatr Jul 25 '16 at 14:51
  • I expect the same. But in console window i have f.e. 20 string from thread 1, then 12 strings from thread 2. – kyy8080 Jul 25 '16 at 14:56
  • The reason could be indeed poorly implemented singleton, see [@Scott](http://stackoverflow.com/a/38571048/1997232) answer. Initially I though you get this behavior, but you expect something different. – Sinatr Jul 25 '16 at 14:58
  • What does the field `private static TestClass test;` do? – Jeroen van Langen Jul 25 '16 at 15:22
  • Nothing:) that was from another implementation – kyy8080 Jul 26 '16 at 06:11

1 Answers1

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; 
        }
    }
    ...
}
Scott Chamberlain
  • 124,994
  • 33
  • 282
  • 431