Question on Locking and Deadlocks

[Chris Newby]

I'm trying to undertand some details on the C# "lock" statement. I came
across some code during a review and thought I spotted a deadlock scenario,
but the programmer said he'd taken it from a reliable source.

Does the following code contain a potential deadlock? or are the locks
granted to syncRoot specific to each method?
================================================== ========
private static readonly object syncRoot = new object();
private static object resource;

public static object Resource
{
get
{
if( resource == null )
{
lock( syncRoot )
{
resource = new object();
}
}
return resource;
}
}

public static void DoSomethingToResource()
{
lock( syncRoot )
{
object resourcePlaceHolder = Resource;
}
}
================================================== ========

TIA//

[Ignacio Machin \( .NET/ C# MVP \)]

"Chris Newby" <ch*********@rockcreekglobal.comwrote in message
news:uu**************@TK2MSFTNGP04.phx.gbl...

I'm trying to undertand some details on the C# "lock" statement. I came
across some code during a review and thought I spotted a deadlock
scenario, but the programmer said he'd taken it from a reliable source.

Not very reliable for real :)

the Reourse property is not thread safe, the comparison should be inside the
lock statement. If not it's possible to create two instances if two thread
happen to evaluate the if before it's assigned in either thread. NOTE that
I see the DoSomethingToResource and its locking, but as the Resource
property is public you can call it from another method that do not use the
lock.

The correct wa is

lock( syncRoot )
{
if( resource == null )

{
resource = new object();
}
}

[Chris Newby]

Thanks Ignacio ... actually I made a mistake in the declaration. The private
field "resource" should have been declared volatile:

private static volatile object resource;

this would ensure that if one thread begins writing to "resource" any other
thread is blocked from reading it until the write operation is complete ...
correct?

also, the example I gave for DoSomethingToResource is not very good ...
let's suppose that it does something that requires a lock. Can I safely lock
on "syncRoot" from within DoSomethingToResource even though it references
the public property accessor "Resource"? or must I use a different reference
for creating the lock?

(note that normally, i would in fact use a different object reference to be
on the safe side. but i'm still curious)

Thanks!

"Ignacio Machin ( .NET/ C# MVP )" <machin TA laceupsolutions.comwrote in
message news:OA**************@TK2MSFTNGP06.phx.gbl...

>
"Chris Newby" <ch*********@rockcreekglobal.comwrote in message
news:uu**************@TK2MSFTNGP04.phx.gbl...

>I'm trying to undertand some details on the C# "lock" statement. I came
across some code during a review and thought I spotted a deadlock
scenario, but the programmer said he'd taken it from a reliable source.

Not very reliable for real :)

the Reourse property is not thread safe, the comparison should be inside
the lock statement. If not it's possible to create two instances if two
thread happen to evaluate the if before it's assigned in either thread.
NOTE that I see the DoSomethingToResource and its locking, but as the
Resource property is public you can call it from another method that do
not use the lock.

The correct wa is

lock( syncRoot )
{
if( resource == null )

{
resource = new object();
}
}

[Barry Kelly]

Chris Newby wrote:

Thanks Ignacio ... actually I made a mistake in the declaration. The private
field "resource" should have been declared volatile:

private static volatile object resource;

this would ensure that if one thread begins writing to "resource" any other
thread is blocked from reading it until the write operation is complete ...
correct?

No. Reads and writes to aligned, native-word-sized fields and smaller,
are atomic, but that isn't affected by volatile and the situation is
much more complicated.

Volatile affects a mental model of how reads and writes of fields work
(aka the memory model). Basically, reads can't move before of a read of
a volatile field, and nor can writes move after the write of a volatile
field. The semantics are quite mind-bending.

And these reads and writes have nothing to do with CPU caches, in case
you read that elsewhere - CPU caches have cache coherency logic which
takes care of that itself, at performance cost. Rather it can affect the
order of retiring of memory commands within the processor (very
low-level stuff) or the level of aggressiveness of compiler
optimizations permitted.

Check here for the hairy details:

http://discuss.develop.com/archives/...=DOTNET&P=R375

and more from the same author here:

http://msdn.microsoft.com/msdnmag/is...s/default.aspx

And here (the second example) is a correct implementation of the pattern
you're after:

http://blogs.msdn.com/brada/archive/...12/130935.aspx

There is a slight contradiction between these two sources for the
correctness of the first example on Brad's page. Viewing 'volatile' as
stopping writes moving after the write of a volatile field, it would
appear that having the singleton field volatile will prevent "delayed
publication" of the Singleton's private fields, if any, as initialized
in the constructor. However, Vance suggests otherwise in the first
article, but if I read the second one correctly, the fact that the field
is volatile (he refers to a "head field" in a linked list in the second
article) should prevent late publication of private fields set in the
constructor.

Technically, volatile isn't required on x86, nor the .NET 2.0 memory
model, because writes can't change order (on the same thread).

-- Barry

--
http://barrkel.blogspot.com/

[Brian Gideon]

On Mar 9, 3:52 pm, Barry Kelly <[e-mail]:

There is a slight contradiction between these two sources for the
correctness of the first example on Brad's page. Viewing 'volatile' as
stopping writes moving after the write of a volatile field, it would
appear that having the singleton field volatile will prevent "delayed
publication" of the Singleton's private fields, if any, as initialized
in the constructor. However, Vance suggests otherwise in the first
article, but if I read the second one correctly, the fact that the field
is volatile (he refers to a "head field" in a linked list in the second
article) should prevent late publication of private fields set in the
constructor.

Vance later said that the first example in his post (the first source)
was written incorrectly. That is, that it was actually thread-safe
when the intent was the opposite. He meant to leave off the volatile
modifier.

[Brian Gideon]

On Mar 9, 2:29 pm, "Chris Newby" <[e-mail]>
wrote:

Thanks Ignacio ... actually I made a mistake in the declaration. The private
field "resource" should have been declared volatile:

private static volatile object resource;

this would ensure that if one thread begins writing to "resource" any other
thread is blocked from reading it until the write operation is complete ...
correct?

Chris,

The volatile keyword wouldn't have made it anymore thread-safe. It
still requires that second check, hence the name double-checked
locking. Personally, I avoid these lock free strategies because
they're *incredibly* difficult to get right. Just take the lock
everytime. It's easier, safer, and faster than some think.

Brian

[Barry Kelly]

Brian Gideon wrote:

On Mar 9, 3:52 pm, Barry Kelly <[e-mail]:

There is a slight contradiction between these two sources for the
correctness of the first example on Brad's page. Viewing 'volatile' as
stopping writes moving after the write of a volatile field, it would
appear that having the singleton field volatile will prevent "delayed
publication" of the Singleton's private fields, if any, as initialized
in the constructor. However, Vance suggests otherwise in the first
article, but if I read the second one correctly, the fact that the field
is volatile (he refers to a "head field" in a linked list in the second
article) should prevent late publication of private fields set in the
constructor.

Vance later said that the first example in his post (the first source)
was written incorrectly. That is, that it was actually thread-safe
when the intent was the opposite. He meant to leave off the volatile
modifier.

That makes sense. That mistake left me confused until I read more about
the issue in other sources, and made me even more suspicious of
lock-free code - even folks like Vance make mistakes in carefully
written expository articles :)

-- Barry

--
http://barrkel.blogspot.com/

[Ignacio Machin \( .NET/ C# MVP \)]

Hi,

"Brian Gideon" <br*********@yahoo.comwrote in message
news:11**********************@c51g2000cwc.googlegr oups.com...

On Mar 9, 2:29 pm, "Chris Newby" <[e-mail]>

>
The volatile keyword wouldn't have made it anymore thread-safe. It
still requires that second check, hence the name double-checked
locking. Personally, I avoid these lock free strategies because
they're *incredibly* difficult to get right. Just take the lock
everytime. It's easier, safer, and faster than some think.

I agree with you, always locking is safer and not that costly.

I do remember several threads discussing that, and somewhere I read that the
double blocking is not garanteed to work as expected.