Unit testing the c# double lock pattern

Question

I'm experimenting writing some unit tests for the following code

private bool _initCalled = false;
private void Initialize()
{
    if (!_initCalled)
    {
        lock (this)
        {
            if (_initCalled)
                return;
            NoConfigInit();
            _initCalled = true;
        }
    }
}

What is the best way to test this code to hit the code path if (_initCalled) return?

Alternative ways of representing this code are also welcome, but I'm curious about how to test these patterns for correctness.

Answer 1

I have a way to test the code that you wrote, but it has a couple of conditions:

1. You need to be using Enterprise edition of Visual Studio to use Microsoft Fakes (you may be able to get a similar concept working with free alternative called Prig, but i have no experience with it)

2. You have to be targeting .net framework, not .net core.

We have to alter your code a little, like so:

public class Class3
  {
    private bool _initCalled = false;
    public void Initialize()
    {
      if (!_initCalled)
      {
        lock (this)
        {
          // we need to insert an empty DummyMethod here
          DummyMethod();
          if (_initCalled)
            return;
          NoConfigInit();
          _initCalled = true;
        }
      }
    }

    private void DummyMethod()
    {
      // This method stays empty in production code.
      // It provides the hook for the unit test.
    }

    private void NoConfigInit()
    {

    }
 }

Then, after generating the fakes for the library, we can write the test like so:

[TestMethod]
public void TestMethod1()
{
  using (ShimsContext.Create())
  {
    // This is the value to capture whether the NoConfigInit was called
    var initCalled = false;

    // Here the DummyMethod comes into play
    Namespace.Fakes.ShimClass3.AllInstances.DummyMethod =
      class3 =>
        typeof(Class3).GetField("_initCalled", BindingFlags.Instance | BindingFlags.NonPublic)
          .SetValue(class3, true);

    // This is just a hook to check whether the NoConfigInit is called.
    // You may be able to test this using some other ways (e.g. asserting on state etc.)
    Namespace.Fakes.ShimClass3.AllInstances.NoConfigInit = class3 => initCalled = true;

    // act
    new Class3().Initialize();

    // assert
    Assert.IsFalse(initCalled);
  }
}

If you debug the test you will see that it exits in the second check.

I agree this is not an ideal way to test it, as we had to modify the original code.

Another option along the same lines is to change _initCalled to be a property -> then Fakes can hook into setters and getters so you can avoid the DummyMethod, and simply return true on second call, like so (in the unit test):

     int calls = 0;
     Namespace.Fakes.ShimClass3.AllInstances.InitCalledGet = class3 => calls++ > 0;

Answer 2

One way to test this (without Microsoft Fakes or similar, using a mocking framework instead, ie. FakeItEasy, Moq or NSubstitute) is to do two test cases, one test case where you call the Initialize-method at the same time in two different threads to test the if-statement inside the lock, and one test case where you call the Initialize-method consecutively to test the if-statement outside the lock. Something like this:

using System.Threading.Tasks;
using FakeItEasy;
using Xunit;

namespace MyNamespace
{
    public class MyClass
    {
        private readonly object _lock = new object();
        private readonly IMyInterface _noConfig;
        private bool _initCalled;

        public MyClass(IMyInterface noConfig)
        {
            _noConfig = noConfig;
        }

        public void Initialize()
        {
            if (_initCalled)
            {
                return;
            }

            lock (_lock)
            {
                if (_initCalled)
                {
                    return;
                }

                _noConfig.Init();
                _initCalled = true;
            }
        }
    }

    public interface IMyInterface
    {
        void Init();
    }

    public class MyTests
    {
        private readonly IMyInterface _myInterface;
        private readonly MyClass _myClass;

        public MyTests()
        {
            _myInterface = A.Fake<IMyInterface>();
            _myClass = new MyClass(_myInterface);
        }

        [Fact]
        public async void Initialize_CallConcurrently_InitNoConfigOnlyCalledOnce()
        {
            A.CallTo(() => _myInterface.Init()).Invokes(() => Thread.Sleep(TimeSpan.FromMilliseconds(5)));

            Task[] tasks =
            {
                Task.Run(() => _myClass.Initialize()),
                Task.Run(() => _myClass.Initialize())
            };
            await Task.WhenAll(tasks);

            A.CallTo(() => _myInterface.Init()).MustHaveHappenedOnceExactly();
        }

        [Fact]
        public void Initialize_CallConsecutively_InitNoConfigOnlyCalledOnce()
        {
            _myClass.Initialize();
            _myClass.Initialize();

            A.CallTo(() => _myInterface.Init()).MustHaveHappenedOnceExactly();
        }
    }
}

The small delay in the first test make sure that the method actually takes some time to execute, if you remove it or if the delay is too low the test will still be green, but it might result in that the second execution sometimes will hit the outer if-statement instead of the inner.

As you can see I also made use of dependency injection to make it easy to test so the noConfig.Init method is just called once. This is not necessary, you can do it in other ways as well, but in this case I thought it was the nicest solution.