Writing a unit test for a singleton implementation

[Idan Arye]

On Wednesday, 15 May 2013 at 19:17:23 UTC, Diggory wrote:
> On Wednesday, 15 May 2013 at 18:28:53 UTC, Idan Arye wrote:
>> I'm making an idioms 
>> library(http://forum.dlang.org/thread/fofbrlqruxbevnxchxdp@forum.dlang.org) 
>> to add to Phobos, and one of the idioms is the singleton 
>> design pattern. Since I'm gonna send it to Phobos, it needs to 
>> have a unit test. Here is my unit test:
>>
>>    static class Foo
>>    {
>>        mixin LowLockSingleton;
>>
>>        this()
>>        {
>>            Thread.sleep(dur!"msecs"(500));
>>        }
>>    }
>>
>>    Foo[10] foos;
>>
>>    foreach(i; parallel(iota(foos.length)))
>>    {
>>        foos[i] = Foo.instance;
>>    }
>>
>>    foreach(i; 1 .. foos.length)
>>    {
>>        assert(foos[0] == foos[i]);
>>    }
>>
>>
>> This unit test works - it doesn't fail, but if I remove the 
>> `synchronized` from my singleton implementation it does fail.
>>
>> Now, this is my concern: I'm doing here a 500 millisecond 
>> sleep in the constructor, and this sleep is required to 
>> guarantee a race condition. But I'm not sure about two things:
>>
>> - Is it enough? If a slow or busy computer runs this test, the 
>> 500ms sleep of the first iteration might be over before the 
>> second iteration even starts!
>>
>> - Is it too much? Phobos has many unit tests, and they need to 
>> be run many times by many machines - is it really OK to add a 
>> 500ms delay for a single item's implementation?
>>
>>
>> Your opinion?
>
> There's no real way to reliably test race conditions. One thing 
> you could do is get a bunch of threads ready and waiting to 
> access "Foo.instance" and then notify them all at once, that 
> way you can do away with "sleep" which is not great to have in 
> a unit test anyway. Repeat this a few times and it should be 
> fairly reliable, plus it will usually be much faster because 
> you don't have to sleep.


OK, I used `core.sync.barrier` to make all threads access the 
singleton together:

     static class Foo
     {
         mixin LowLockSingleton;

         private this()
         {
             Thread.sleep(dur!"msecs"(0));
         }
     }

     Foo[10] foos;
     Thread[foos.length] threads;
     Barrier barrier = new Barrier(foos.length);

     class FooInitializer : Thread
     {
         ulong index;

         this(ulong index)
         {
             super(&run);
             this.index = index;
         }

         void run()
         {
             barrier.wait();
             foos[index] = Foo.instance;
         }
     }

     foreach(i; 0 .. foos.length)
     {
         threads[i] = new FooInitializer(i);
         threads[i].start();
     }

     foreach(thread; threads)
     {
         thread.join();
     }

     foreach(i; 1 .. foos.length)
     {
         assert(foos[0] == foos[i]);
     }

This gives me 100% accuracy. Your idea of holding all threads 
together did the trick - if I comment out the call to 
`barrier.wait()` I get a 50% accuracy, which ofcourse is not as 
nearly as good as 100%.

The sleeping, however, is still required. If I remove it I also 
get 50% accuracy. I tried to replace it with `Thread.yield()` and 
that gave me 92% accuracy - which is far better than 50% but not 
as good as 100%. A sleep of 0 seconds is not that bad a price to 
pay for that 100% accuracy.