Writing a unit test for a singleton implementation

[Dmitry Olshansky]

17-May-2013 04:57, Idan Arye пишет:
> On Thursday, 16 May 2013 at 21:10:52 UTC, Dmitry Olshansky wrote:
>> 16-May-2013 02:21, Idan Arye пишет:
>>> 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 why I added the "(almost)". A race condition happens when one
> thread reads a variable and writes it back based on the old value, and
> between that read and write another thread writes to that variable. By
> adding a `sleep()
` between that read and write, I can conjure my own
> race condition.

A terminological moment - a race condition is not that fact "bad 
interleaving of reads-writes did happen".

Race condition is totally a static property of a code (or a program), 
whereby at least 2 threads may simultaneously access memory location and 
there is no ordering of operations insured (or happens before relations 
established). It's then said that the program has race condition, 
sometimes calling it potential if there is no proof that 2 threads can 
access it simultaneously.

>> 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.
>
> But if I have a system in my car that allows it to keep traveling even
> after unscrewing some bolts,

The trick here is that race condition is not guaranteed to fail in a 
specific way.

> OK, I just tested it by playing for video files at once, and the
> accuracy dropped from 100% to around 96%.
>
> Still, the point of unit tests is to make sure that code changes do not
> break old code. Most of them are super trivial, because many of the bugs
> they prevent are also super trivial - the kind of bugs that make you
> want to hit yourself for being so stupid you didn't notice them before.
> If the library or system have a good set of unit tests, a programmer
> that runs them can catch those bugs happening in trivial examples right
> after they are introduced.
>
> So, if a Phobos\druntime\dmd developer somehow manages to change
> something that breaks my singleton implementation, the next time they
> run the unit tests there is over 90% chance that unit test will catch
> the bug even if they put a heavy burden on their machine - and if they
> don't put such a heavy burden while developing, those chances climb very
> near to 100%.
>
> So yea, my unit test is not 100% accurate in worst case scenarios - but
> it still does it's job.

Chance == smoke test, so I think we finally in agreement :)
Then by definition running it multiple times you'll have (1 - p)^^n 
chance for bugs escaping.
In general since you can't know probability you don't know how long 
should you run it. Hence my reference to the running it long enough.

>
>> But if you like it I think Thread.yield will work just as well, it
>> will cause threads to do the context switch.
>
> I tested both. `Thread.yield()` gives around 92% accuracy, while
> `Thread.sleep(dur!"msecs"(0))` gives 100% accuracy(when I don't play 4
> video files at once). I have no idea why this happens, but it happens.
>
>> 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.
>
> That's what I did in the second version - I suspended all the threads
> using `core.sync.barrier`, and the barrier took care to resume them all
> at once. This allowed me to use a 0ms sleep - but the call to `sleep()`
> is still required.

To ensure context switch. My guess is that yield didn't work because 
then it could be the case that there is no extra work in the system. In 
which case the OS can stay on the same thread (and avoid switching) 
after yield.

>> 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.
>
> How is it an atomic read/write

Each operations is atomic.

 > if I call `sleep()` *between* the read
 > and the write?

I'm not seeing it. I think I've misread the code anyway.

If you test TLS low-lock singleton

vs

if(instance_ != null) //this is critical point, how do you test it?
	synchronize(mut){
		if(instance_ != null)
			instance_ = ...;
	}
return instance_;

Or the one that isn't locked at all?

Can you show the code actually - bogus singleton and correct one?

>> 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
>> launch many threads and somehow try to schedule them funky. Even then I
>> don't see how 1 single-shot can be reliable there.
>
> True - my testing method is not possible with lock-free collections,
> since I can't put a `sleep()` call inside an atomic operation. But I
> *can* put a sleep call inside a `synchronized` block(or more precisely,
> inside a constructor that is being invoked inside a synchronized block),
> so my testing method does work for my case.

Which is not what I thought you are trying to test. In this trivial case 
I'd avoid such tests completely YMMV.

-- 
Dmitry Olshansky