[Mono-devel-list] Double-locking and thread safety

[Kornél Pál]

Note that I changed subject because this dicussion is not specific to my
patch altough it applies to my patch.

First of all: If any of my conclusions are wrong please let me know.

> From: Jonathan Pryor
>> private static ResourceManager GetResourceManager ()
>> {
>>  if (resources == null)
>>   lock (resourcesLock) {
>>    if (resources == null)
>>     resources = new GetTextResourceManager (typeof
>> (Locale).Assembly.GetName
>> ().Name, typeof (Locale).Assembly);
>>   }
>>  return resources;
>> }
>>
>> I think this is good, because it is intended to initialize resources only
>> once.
>
> It looks good.  It acts good.  It isn't good.
>
> See:
> http://galactus.ximian.com/pipermail/mono-devel-list/2004-February/004101.html
> http://blogs.msdn.com/cbrumme/archive/2003/05/17/51445.aspx

Using the following code as the example:

if (a == null)
{
  lock(obj)
  {
    if (a == null) a = new A();
  }
}

I see the problem as the following:

1. Declaring "a" as volatile would solve the problem but it results in
performance loss because of memory barriers for each read.
2. Removing the check outside the lock would solve the problem but it
results in performance loss because of memory barriers for each read.
3. It is not a problem that "a" may be seen as null even if it was already
created because lock will do memory barrier.
4. The problem is that "a" may be assigned before it's content is stored
(has the same side effects as using the object without executing the
constructor) because reordering and this will cause problems if a processor
did not cached value of "a" and wants to read it from the shared memory then
it will see the object in "a" but will see an uninitialized object.
5. Using a memory barrier after the object was created but before it is
assigned to "a" solves the problem and does not result in performance loss
when the object is already created.

So the following code is good:
if (a == null)
{
  lock(obj)
  {
    if (a == null)
    {
      object newA = new A();
      Thread.MemoryBarrier();
      a = newA;
    }
  }
}

I think using a lock is better than constructing a new instance because the
new instance would be dropped if it's already created as we want to use a
single instance so it's better to suspend the thread by doing a wait than
creating a new instance because the CPU time can be used for useful
operations instead of wating time.

http://blogs.msdn.com/cbrumme/archive/2003/05/17/51445.aspx:
"Realize that synchronization is expensive.  The full fence implied by
Interlocked.Increment can be many 100’s of cycles on modern hardware.  That
penalty may continue to grow, in relative terms."

I think this applies to Interlocked.CompareExchange as well.

So I suggest to use this model instead of Interlocked.CompareExchange.
Furthermore I think using two Thread.MemoryBarrier() is less effective than
using lock and and a single Thread.MemoryBarrier() because there will be not
useless object creation, Thread.MemoryBarrier() allways does a full barrier
while lock does acrique at the begining and release at the end and
Interlocked.CompareExchange does barriers as well so there are more barriers
and thus locks in your "lock-free" code than in the locked code.

> From: Jörg Rosenkranz
> How about using Thread.MemoryBarrier how it's been suggested
> in Brad Abrams' blog:
> http://blogs.msdn.com/brada/archive/2004/05/12/130935.aspx

This code does the same as the above code so this code should be good.

> Is this supposed to work on Mono?

Thread.MemoryBarrier is not implemented by Mono yet (no exception but does
nothing), but it seems that it is required only by IA64 that is not
supported by Mono yet so you can already use it but will be implemented only
when it will be necessary.

Kornél