[Mono-list] Class library developers: locking issues to keep in mind

[Alexander Klyubin]

Flaw #2:

I see your point about the lock and flushing of data from local copies 
of thread to shared main memory.
Consider following situation: "initializer" is flushed first to main 
memory. A thread executes GetHeavyObject. It already got initializer 
pointing to DummyInit from shared memory. Now, what happens is that this 
thread does NOT perform an explicit read barrier and hence may end up 
with incompletely initialized singleton instance. This is because the 
data of singleton instance may not yet been fully flushed to shared main 
memory.

What I don't understand, is whether JVM is allowed to load local copies 
from shared memory without explicit read barriers? This happens in 
GetHeavyObject method. You do not have an explicit memory barrier there. 
Anwering this might make the picture clearer and mark the preconditions 
to flaw #2 as impossible.

Flaw #3:

What I was saying is not that several instances of singleton will be 
created, but that thread will be executing in case singleton != null:

lock (typeof (Singleton)) {
   ++Count;
}

This is not that big an issue, but anyway.

Regards,
Alexander Klyubin

Serge wrote:

>>Clever, but broken :)
>>
> Well, let's try to repair ;-)
> 
> Let's use Java model, since NET's locking semantics are not clear at the
> moment :)
> Quotes are taken from Doug Lea's "Concurrent Programmin in Java (2nd ed.)"
> chapter 2.2.7.2 pp 94-95, this chapter is also available online ([1]).
> 
> 
>>Flaw #1: Now, the order in which JVM sends updates from local memory to
>>shared mememory is arbitrary.
>>
> That's right. Good point.
> "If a field is declared as volatile, any value written to it is flushed and
> made visible by the writer thread before the writer thread performs any
> further memory operation (i.e., for the purposes at hand it is flushed
> immediately)."
> So declaring them volatile would help (but see below, flaw #2). And Miguel
> confirmed that volatile has essentially the same semantics in .NET.
> 
> 
>>Flaw #2: Assuming that initializer points to DummyInit, singleton may
>>still point not to null but to a incompletely initialized instance of
>>Singleton instance
>>
> Nope, there are only two possibilities - either fully initialized singleton
> or null. Instance is only created inside the fully synchronized method
> (RealInit):
> "In essence, releasing a lock forces a flush of all writes from working
> memory employed by the thread, and acquiring a lock forces a (re)load of the
> values of accessible fields." But this is only in the case if "writing
> thread releases a synchronization lock and a reading thread subsequently
> acquires that same synchronization lock", so you're right, due to
> unspecified flushing sequence another thread that sees DummyInit, could see
> (singleton == null) but never incompletely initialized singleton.
> Volatile should help with that (changes are immediately visible).
> Other possible fixes (in absence of volatile for example, or if volatile
> doesn't guaranties immediate flushing):
> 1) Add yet another check:
>      public static object GetHeavyObject ()
>      {
>           // calling either RealInit or DummyInit
>           initializer  ();
> 
>           // at this point singleton is either null
>           // or fully initialized object
> 
>           // now check it again
>           if (singleton == null) RealInit ();
> 
>           // if RealInit invoked here, then
>           // all values will be reloaded.
>           // That's because RealInit is fully-synched,
>           // see quote for Flow 2
> 
>           return singleton;
>      }
> 
> It is safe to call RealInit, because it is a) fully-synchronized b) contains
> it's own check so singleton will be created only once in any case.
> 
> 2) Spawn yet another thread inside the RealInit, that will perform
> initialization and exit. Then Join.
> "As a thread terminates, all written variables are flushed to main memory."
> Of course, thread only spawned if (singleton == null)
> 
> 
>>Flaw #3: A minor one. The order of sending updated to shared memory
>>might be vice versa: first singleton, then initializer. In that case,
>>RealInit might be executed several times for one thread, because
>>initializer is not always changed to DummyInit inside RealInit. If
>>singleton is not null, initializer will still keep on poiting to ReadInit.
>>
> 
> Nope, it will never execute more than once for one thread - RealInit is
> entirely locked, if (sinleton == null) branch is only executed exactly one
> in any case.
> However, different thread(s) could enter RealInit before it's completed, it
> will wait on method's lock held by the first thread. After the first thread
> finishes executing RealInit and releases lock all changes are visible to the
> second thread (see quote from Flow 2).
> Actually, this one is the main problem. Theoretically, there could be
> arbitrary number of thread waiting for RealInit lock. Those are the threads
> started during the time RealInit's body is executed. This will work
> correctly though - RealInit acts just like simple initializer with
> serialized access (without DCL). But this will have performance impact of
> course.
> 
> Anyhow, I think either volatile access or above two fixes should eliminate
> any issues (sure, it's quite possible that I'm wrong :-)
> 
> Any thoughts?
> 
> Sergey
> 
> 
> [1] http://gee.cs.oswego.edu/dl/cpj/jmm.html
> 
> 
> 
> 
> 
> 
> 
> _______________________________________________
> Mono-list maillist  -  Mono-list@ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-list
>