Test the double check locking mechanism in singleton for 100% code coverage

Question

I am trying to achieve 100% code coverage and trying to write unit test for the double check locking mechanism in singleton.

if (test == null) {
    synchronized (Test.class) {
        if (test == null) {
            test = injector.getInstance(Test.class);
        }
    }
    return test;
}

How do i test the scenario where the second null check is hit? I tried to read a lot and couldn't find a way to do it.

Answer 1

Update: I removed the totally unnecessary threading code from the tests for the 100% reliable variants

---

A 100% reliable testing of the classical double-checked locking idiom that uses a synchronized block as shown in OP is not possible, as for that you would need some hook between the outer if and the synchronized block or latest the synchronized block so that you know when something has passed the outer if and is now waiting at the synchronized block. Or to be more precise, after reading the field value and before waiting.

My answer will be split, I first will show a way how to do a pretty (but not 100%) reliable way to test the "outer-if-passed-inner-if-failed" case of the idiom. Then I will show three options how to slightly modify the production code to make it easily testable 100% reliable.

One more disclaimer, my example code will be in Spock and thus be Groovy code, as imho it is superior to write tests, even if your production code is in Java, especially when it comes to accessing private fields or methods which simply works in Groovy implicitly as long as they are not final or in a superclass. But the code should be easily translatable to plain JUnit Java code accordingly, especially as long as your fields and methods are not private.

---

So here first the pretty reliable way of testing the idiom with some variations and explanations inline.

def 'field should not get changed once assigned even if outer check succeeds'() {
    given:
        def insideSynchronizedBlock = new CountDownLatch(1)
        def continueTest = new CountDownLatch(1)
        // this is the value the field is set to after the
        // test passed the outer if and before checking the
        // inner if
        def testFieldValue = new Object()

    when:
        // get the object the synchronized block is synchronizing on
        // this can be some class object, internal field or whatever,
        // in OP this is the Test class object so we are using it here too
        def initializationLock = Test

        // this thread will enter the synchronized block and then
        // wait for the continueTest latch and thus hinder
        // the main thread to enter it prematurely but wait at
        // the synchronized block after checking the outer if
        //
        // if inside the synchronized block content there is something
        // that can be mocked like some generator method call,
        // alternatively to manually synchronizing on the lock and setting
        // the field in question, the generator method could be mocked
        // to first block on the continueTest latch and then return testFieldValue
        def synchronizedBlocker = Thread.start {
            synchronized (initializationLock) {
                // signal that we now block the synchronized block
                insideSynchronizedBlock.countDown()

                // wait for the continueTest latch that should be triggered
                // after the outer if was passed so that now the field
                // gets assigned so that the inner if will fail
                //
                // as we are in a different thread, an InterruptedException
                // would just be swallowed, so repeat the waiting until the
                // latch actually reached count 0, alternatively you could
                // mark the test as failed in the catch block if your test
                // framework supports this from different threads, you might
                // also consider to have some maximum waiting time that is
                // checked if an interrupt happened
                while (continueTest.count != 0) {
                    try {
                        continueTest.await()
                    } catch (InterruptedException ignore) {
                        // ignore, await is retried if necessary
                    }
                }
                // as the outer if was passed, now set the field value
                // in the testee to the test field value, we called the
                // testee field test here as was in OP example
                // after this the synchronized block will be exited
                // and the main test code can continue and check the inner if
                //
                // in the mocking case mentioned above, here testFieldValue
                // would be returned by the mocked injector method
                testee.test = testFieldValue
            }
        }

        // wait for the synchronizedBlocker to block the synchronized block
        // here if an InterruptedException happens, the test will fail,
        // alternatively you could also here do some while-loop that retries
        // the waiting, also with an optional maximum waiting time
        insideSynchronizedBlock.await()

        // this thread will trigger the continueTest latch and thus
        // implicitly cause the field to be set and the synchronized
        // block to be exited
        //
        // this is the problematic part why this solution is not 100% reliable
        // because this thread should start only after the main test code has
        // checked the outer if, otherwise the field value might be set before
        // the outer if is checked and so the outer if will already fail
        //
        // the test here will not detect this but stay green, the only way this
        // would be observed is because branch coverage would drop by one if this
        // happened; unfortunately that this happens is not too abstract but a
        // real "problem", so if you want to maintain 100% branch coverage, you
        // have to take additional measures, like for example these:
        //
        // - minimize the delay between the starting of this thread and the main
        //   test code actually passing the outer if check to increase probability
        //   that the if check was passed before this thread gets scheduled
        //   if you for example call the method in question via reflection because
        //   it is private and you don't want ot open it up for testing, this is
        //   slower than usual and you should find and get the method reference
        //   before starting this thread and you might also call it before on some
        //   dummy instance, so that the JIT compiler has a chance to optimize the
        //   access and so on
        //
        //   just do anything you can to minimize the time between starting this
        //   thread and passing the outer if in the main test code, but whatever
        //   you do, it is most probably not enough to have halfway reliable 100%
        //   branch coverage so you might need to take additional measures
        //
        // - add a yield() to this thread before calling countDown on the
        //   continueTest latch to give the main thread a chance to pass the outer
        //   if, if that is enough in your situation
        //
        //   if not, instead add a sleep() to this thread at the same place to
        //   give the main thread more time to pass the outer if, how long this
        //   sleep must be cannot be said and you just have to test what value
        //   you need to have halfway reliable results, it might already be
        //   enough to sleep for 50 ms or even less, just try it out
        //
        // - repeat the test X times so that at least with one of the tries
        //   the intended branch was hit and thus branch coverage will stay at
        //   100%, how big X is again has to be determined from case to case
        //   by trial and error or by wasting time by setting a very high value
        //
        //   in case of Spock being used the repeating can simply be achieved
        //   by adding a `where: i << (1..1000)` after the cleanup block to repeat
        //   the test a thousand times, for more information read about data
        //   driven testing in Spock, the data variable is simply ignored here
        //   and only cares for the identically repeated execution of the test
        def synchronizedUnBlocker = Thread.start {
            continueTest.countDown()
        }

    then:
        // here hopefully the outer if is passed before the synchronizedUnBlocker
        // is scheduled and then we wait at the synchronized block
        // then the synchronizedUnBlocker is scheduled and counts down the latch
        // the synchronizedBlocker is waiting for which in turn will set the
        // field value to testFieldValue
        // then the synchronizedBlocker exist and thus unblocks the synchronized
        // block, allowing the inner if to be tested here which now should fail
        // and the field value staying at testFieldValue
        // that the testFieldValue stays set is verified here, but this would also
        // be green if the outer if failed already because the synchronizedUnBlocker
        // was scheduled before the outer if was passed
        testee.getCommandPrefix(message) == testFieldValue

    cleanup:
        // wait for the helper threads to die,
        // this is not necessarily required they will die anyway
        synchronizedBlocker?.join()
        synchronizedUnBlocker?.join()
}

---

As mentioned previously, the only way (to my knowledge) to 100% reliably test the double-checked locking is by modifying the production code to be able to hook inbetween the two if checks and before the main thread blocks on some monitor. More exactly after the field value for the outer if is read and before the thread blocks.

This can be achieved in multiple ways and I'm showing some of them here, so you can pick which one you like most or develop your own way based on them. These alternatives are not tested for performance and I'm not going to make any performance statement about them, I just say that these still work correctly and are 100% reliable testable with 100% branch coverage. If you are familiar with micro-benchmarks, do some proper micro-benchmarking performance tests with these approaches and want me to add them to the answer, just tell me and I add the information.

This is the original double-checked locking Java method that I'm going to modify:

public class Test {
    private final Object fieldInitializationLock = new Object();
    private volatile String field;
    public String getField() {
        String field = this.field;
        if (field == null) {
            synchronized (fieldInitializationLock) {
                field = this.field;
                if (field == null) {
                    field = "";
                    this.field = field;
                }
            }
        }
        return field;
    }
}

and here the according test based on my explanations above:

import spock.lang.Specification

import java.util.concurrent.CountDownLatch

class DoubleCheckedLockingTest extends Specification {
    def 'field should not get changed once assigned even if outer check succeeds'() {
        given:
            def testee = new Test()
            def insideSynchronizedBlock = new CountDownLatch(1)
            def continueTest = new CountDownLatch(1)
            def testFieldValue = new String()

        when:
            def synchronizedBlocker = Thread.start {
                synchronized (testee.fieldInitializationLock) {
                    insideSynchronizedBlock.countDown()
                    while (continueTest.count != 0) {
                        try {
                            continueTest.await()
                        } catch (InterruptedException ignore) {
                            // ignore, await is retried if necessary
                        }
                    }
                    testee.field = testFieldValue
                }
            }

            insideSynchronizedBlock.await()
            def synchronizedUnBlocker = Thread.start {
                continueTest.countDown()
            }

        then:
            testee.getField().is(testFieldValue)

        cleanup:
            synchronizedBlocker?.join()
            synchronizedUnBlocker?.join()
    }
}

---

The first variation is probably also my favorite one.

It uses a higher-level synchronization tool, a ReadWriteLock, to do the double-checked locking instead of a volatile field and a synchronized block on some monitor object. Be aware, for this variant the field does not need to be volatile anymore and we also do not need the local field that prevents multiple slow access to the volatile field.

It is my favorite, because like all other 100% reliable solutions it requires a change of the production sources, but there is still only production code. The logic is just implemented with a different higher-level tool and by doing that suddenly became properly testable. The other methods might look simpler and require less changes, but they all introduce code that is exclusively used for making the idiom testable and have no productive value.

So here the modified Java sources:

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

public class Test {
    private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
    private final Lock readLock = readWriteLock.readLock();
    private final Lock writeLock = readWriteLock.writeLock();
    private String field;
    public String getField() {
        readLock.lock();
        try {
            if (field == null) {
                readLock.unlock();
                try {
                    writeLock.lock();
                    try {
                        if (field == null) {
                            field = "";
                        }
                    } finally {
                        writeLock.unlock();
                    }
                } finally {
                    readLock.lock();
                }
            }
            return field;
        } finally {
            readLock.unlock();
        }
    }
}

And here the according test. Differences to the original test are, that it works 100% reliable and needs no extra thread. We need to set a final variable, the readLock, but thanks to reflection this works just fine and is ok for test code imho. If that is not possible or wanted, we can also use some mocking tool like PowerMock, that is able to mock the new ReentrantReadWriteLock() call for example.

We simply decorate the writeLock.lock() call. Before we do the actual locking, we set the field in question, so that the inner if will fail, which is exactly what we wanted to achieve.

import spock.lang.Specification

class DoubleCheckedLockingTest extends Specification {
    def 'field should not get changed once assigned even if outer check succeeds'() {
        given:
            def testee = new Test()
            def testFieldValue = new String()

        and:
            Test.getDeclaredField("writeLock")
                    .tap { it.accessible = true }
                    .set(testee, Spy(testee.writeLock))
            testee.writeLock.lock() >> {
                testee.field = testFieldValue
                callRealMethod()
            }

        expect:
            testee.getField().is(testFieldValue)
    }
}

---

All other variations introduce something artificially to hook inbetween the if-condition being checked and the monitor being acquired, so that the synchronized blocker can be signaled to continue which the previous approach automatically has already.

---

The following variation introduces something that can be instrumented to set the field in question and does nothing at production time. In this example I use hashCode() on an Object and replace it by a stub in the test, but you can use any other variation like having some class that does nothing and just create a new object which you then instrument using PowerMock or similar. The only important point is, that you can somhow instrument it at test time, it does nothing or as less as possible at production time, and is not optimized away by the compiler.

Here the Java code, where only the two lines with the testHook are added:

public class Test {
    private final Object fieldInitializationLock = new Object();
    private volatile String field;
    private Object testHook;
    public String getField() {
        String field = this.field;
        if (field == null) {
            testHook.hashCode();
            synchronized (fieldInitializationLock) {
                field = this.field;
                if (field == null) {
                    field = "";
                    this.field = field;
                }
            }
        }
        return field;
    }
}

and the according test code where the testHook is replaced by an instrumented stub:

import spock.lang.Specification

class DoubleCheckedLockingTest extends Specification {
    def 'field should not get changed once assigned even if outer check succeeds'() {
        given:
            def testee = new Test()
            def testFieldValue = new String()

        and:
            testee.testHook = Stub(Object)
            testee.testHook.hashCode() >> {
                testee.field = testFieldValue
                1
            }

        expect:
            testee.getField().is(testFieldValue)
    }
}

---

The following variant replaces the equals check in the if by Objects.isNull(...) and then instruments that call using PowerMock. From production-code perspective this variant is also not too bad, but the current drawback is, that PowerMock has to be used and that PowerMock does not harmonize with the default and recommended JaCoCo on-the-fly instrumentation, so the tested class will have 0% test coverage, or must be instrumented using JaCoCo offline instrumentation. Only because it can be done properly, despite it's easiness or the opposite, I mention it here.

The Java class:

import static java.util.Objects.isNull;

public class Test {
    private final Object fieldInitializationLock = new Object();
    private volatile String field;
    public String getField() {
        String field = this.field;
        if (isNull(field)) {
            synchronized (fieldInitializationLock) {
                field = this.field;
                if (field == null) {
                    field = "";
                    this.field = field;
                }
            }
        }
        return field;
    }
}

and the according test:

import groovy.transform.CompileStatic
import org.junit.runner.RunWith
import org.mockito.stubbing.Answer
import org.powermock.core.classloader.annotations.PrepareForTest
import org.powermock.modules.junit4.PowerMockRunner
import spock.lang.Specification

import static org.mockito.ArgumentMatchers.isNull
import static org.mockito.Mockito.when
import static org.mockito.Mockito.withSettings
import static org.powermock.api.mockito.PowerMockito.mockStatic

@RunWith(PowerMockRunner)
@PrepareForTest(Test)
class DoubleCheckedLockingTest extends Specification {
    // this helper is necessary, or PowerMock cannot properly mock
    // the system class, as it instruments the test class to intercept
    // and record the call to the system class
    // without compile static the dynamic Groovy features prevent this
    @CompileStatic
    def staticallyCompiledHelper(Answer answer) {
        when(Objects.isNull(isNull())).thenAnswer(answer)
    }

    def 'field should not get changed once assigned even if outer check succeeds'() {
        given:
            def testee = new Test()
            def testFieldValue = new String()

        and:
            mockStatic(Objects, withSettings().stubOnly())
            staticallyCompiledHelper {
                // as the field is value is already read
                // we can already trigger the continueTest latch
                // first and do the null-check second
                testee.field = testFieldValue
                it.callRealMethod()
            }

        expect:
            testee.getField().is(testFieldValue)
    }
}

---