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?

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.

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.

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?

Also note that unit tests are supposed to run fast (definitely < 50 ms), because running 500-5000 of them should be relatively cheap. Apart from that, races aren't something to test for in a unit test. It may be more a kind of integration test, though the sporadic nature of races makes them candidates for "One of those occasionally failing bad guys hamstringing development... Let's just ignore him."-type thinking.
I would create a seperate Test suite for race tests and execute them multiple times if need be... But there are a million better sources on the internet than me on that topic.

tldr; don't test for races in unit tests.

16-May-2013 01:09, Sebastian Graf пишет:
> 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:

[snip]

>> 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!

Sleep in concurrency code that aims to synchronize something is always bad idea (=BUG).

Sleep is a tool used to give up thread execution resources to avoid spinning on something wasting cycles. Use semaphore or analogous primitives to coordinate, see other posts.

>>
>>  - 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?

No.

>>
>>
>> Your opinion?
>
> tldr; don't test for races in unit tests.

One word - it's a stress test (smoke test). It should be a kind of loop that executes for as long as you can allow it. Days later if it didn't fail usually you either decide that it should be "good enough" or keep running them.

-- 
Dmitry Olshansky

On Wednesday, 15 May 2013 at 21:52:40 UTC, Dmitry Olshansky wrote:
> Sleep in concurrency code that aims to synchronize something is always bad idea (=BUG).

Don't say "always". Only a Sith deals in absolutes. Sure, `sleep()` in concurrent code is a bad idea because it can cause race conditions. But I *want* to (almost)cause a race condition here - that's the whole point of this unit test, to guarantee that the singleton implementation is immune to race conditions.

Crashing your car into a brick wall is usually a bad idea, but not always. When they want to test the car's safety systems, they crash it into a wall on purpose. The fact that driving safety rules save lives does not mean the crash test team has to stick to those rules - that would defeat the purpose of the crash test.

> Sleep is a tool used to give up thread execution resources to avoid spinning on something wasting cycles. Use semaphore or analogous primitives to coordinate, see other posts.

In my second version of the unit test(after reading Diggory's comment) I used a `Barrier` to coordinate, and it gave me 50% accuracy - That is, when I took the synchronization out of the implementation the unit test failed 50% of the times. I still need the `sleep()` to make sure the threads are getting mixed.

> One word - it's a stress test (smoke test). It should be a kind of loop that executes for as long as you can allow it. Days later if it didn't fail usually you either decide that it should be "good enough" or keep running them.

Why in the world would I want to make a unit test for days, waiting for a race condition that might not happen, when I can easily force a race condition?

If we return to the car's crash test example - when a crash test team wants to test a car, do they give alcohol to a team member and make him drive around while following him with measure devices? Ofcourse not - they simply drive the car directly to the brick wall.

On Wednesday, 15 May 2013 at 21:09:55 UTC, Sebastian Graf wrote:
> I would create a seperate Test suite for race tests and execute them multiple times if need be... But there are a million better sources on the internet than me on that topic.

One of the primary goals of the Phobos unit tests is to easily catch a change that causes problems with the usage of some Phobos component. If I create a second test, it won't be run by the autotester nor by people making changes to dmd\druntime\Phobos

On Thursday, May 16, 2013 00:21:58 Idan Arye wrote:
> On Wednesday, 15 May 2013 at 21:52:40 UTC, Dmitry Olshansky wrote:
> > Sleep in concurrency code that aims to synchronize something is
> > always bad idea (=BUG).
> 
> Don't say "always". Only a Sith deals in absolutes.

Of course, that in and of itself is an absolute... :)

- Jonathan M Davis

16-May-2013 02:21, Idan Arye пишет:
> On Wednesday, 15 May 2013 at 21:52:40 UTC, Dmitry Olshansky wrote:
>> Sleep in concurrency code that aims to synchronize something is always
>> bad idea (=BUG).
>
> Don't say "always". Only a Sith deals in absolutes. Sure, `sleep()` in
> concurrent code is a bad idea because it can cause race conditions.

Well I embraced the power of the dark side long ago ;)

And I can confidently say that sleep doesn't cause anything reliably at all. Try measuring time with it (e.g. in ms range) and you'll see it :)

> But
> I *want* to (almost)cause a race condition here

You can't "cause" a race condition - it's there by definition of code.
What you can cause is a data corruption that happened with one bad interleave of reads/writes because it was possible.

- that's the whole point
> of this unit test, to guarantee that the singleton implementation is
> immune to race conditions.

I can't see how suspending all threads by 500ms + arbitrary amount of time of on the order of one context switch (or more depends on how system is busy) will do that. Sleep basically says wake me up no sooner then 500ms (i.e. put back into active threads queue), it doesn't guarantee any kind of timing or priority.

Even with proper timings launching a bunch of threads at almost the same time still doesn't guarantee it. You are far better off with solid formal prof in this case (that is there).

What you might be seeing is that if all of threads on idle system preform context switch and then switch back after sleeping X ms they might collide.
>
> Crashing your car into a brick wall is usually a bad idea, but not
> always.

I'm not seeing this test do anything useful anyway - you know there is a race condition and then you try to test to see if it works as just as if there is none.

It's more like unscrewing a bunch of bolts and seeing if the car manages it to the other town. It might or not depending on how the driver rides it and on a ton of other chances - truth be told it's blind luck almost. Even if you unscrew the same bolts exactly the same way the 2nd car will not ride exactly as long a distance as 1st one.


>> Sleep is a tool used to give up thread execution resources to avoid
>> spinning on something wasting cycles. Use semaphore or analogous
>> primitives to coordinate, see other posts.
>
> In my second version of the unit test(after reading Diggory's comment) I
> used a `Barrier` to coordinate, and it gave me 50% accuracy - That is,
> when I took the synchronization out of the implementation the unit test
> failed 50% of the times. I still need the `sleep()` to make sure the
> threads are getting mixed.

And if your PC was compressing video or serving some HTTP you'll have even less no matter how you try to run them I guess...

But if you like it I think Thread.yield will work just as well, it will cause threads to do the context switch.

>
>> One word - it's a stress test (smoke test). It should be a kind of
>> loop that executes for as long as you can allow it. Days later if it
>> didn't fail usually you either decide that it should be "good enough"
>> or keep running them.
>
> Why in the world would I want to make a unit test for days, waiting for
> a race condition that might not happen, when I can easily force a race
> condition?

Sleep doesn't guarantee it. It causes context switch though and that might be what you want. Maybe creating a bunch of threads as suspended and then waking them all up could get close to that.

Another problem is about expecting something definite out of race condition. Yes, here you are getting away with single atomic read/write of pointer simply because of x86 architecture.

Technically what you'll can see with a race condition is undefined (not speaking of this simple example on x86 that IMO is pointless anyway).
Thus trying to catch it in more complex situation will require more then just putting a sleep(xyz) before a function call.

Imagine trying to test a lock-free collection ? You still need to lauch many threads and somehow try to schedule them funky. Even then I don't see how 1 single-shot can be reliable there.

-- 
Dmitry Olshansky

On 05/16/2013 12:33 PM, Jonathan M Davis wrote:
> On Thursday, May 16, 2013 00:21:58 Idan Arye wrote:
>> On Wednesday, 15 May 2013 at 21:52:40 UTC, Dmitry Olshansky wrote:
>>> Sleep in concurrency code that aims to synchronize something is
>>> always bad idea (=BUG).
>>
>> Don't say "always". Only a Sith deals in absolutes.
> 
> Of course, that in and of itself is an absolute... :)
> 
> - Jonathan M Davis

He's a witch!