Concurrency and delegates

[Scott Gifford]

I have a question about concurrency and delegates.

Say I have a class roughly like this:

public class Class1
{
public delegate void MyEventHandler( );
private event MyEventHandler onMyEvent;

public event OnMyEvent
{
add
{
onMyEvent += value;
}
remove
{
onMyEvent -= value;
}
}

private void MyEventHappened ()
{
if (onMyEvent != null)
onMyEvent();
}

If this class is used by multiple threads, it looks like there is a
race condition. If the last event is removed between the "if
(onMyEvent != null)" and the "onMyEvent( )", then "onMyEvent( )" will
throw a Null Reference Exception, right?

So the obvious fix is to use locks. The code for the event property
is obvious, and MyEventHappened now looks like this:

private void MyEventHappened ()
{
lock(this)
{
if (onMyEvent != null)
onMyEvent();
}
}

But now we're calling the delegate with the lock held. This seems bad
in general, since the event could start doing some long-running thing.
A more immediate problem happens if that event handler uses
form.Invoke, then the UI thread tries to call back into Class1 to do
something that requires a lock. Instant deadlock!

To fix the race condition, then, we have to either catch and ignore
the Null reference exception, or make a copy of it while it's locked,
then operate on our copy (which can't change behind our backs):

private void MyEventHappened ()
{
event MyEventHandler copyOfOnMyEvent ;
lock(this)
{
copyOfOnMyEvent = onMyEvent;
}
if (copyOfOnMyEven t != null)
copyOfOnMyEvent ();
}

(does that work? Does "copyOfOnMyEven t = onMyEvent" make a copy of
onMyEvent, or just store a reference to it?)

My question is, do people actually do this? It seems quite tedious,
and I've never seen it in sample code. Is there some reason I'm not
seeing why this isn't really a race condition? Is there some other
way to deal with this?

Thanks!

---Scott.

[=?ISO-8859-1?Q?G=F6ran_Andersson?=]

Scott Gifford wrote:

My question is, do people actually do this? It seems quite tedious,
and I've never seen it in sample code. Is there some reason I'm not
seeing why this isn't really a race condition? Is there some other
way to deal with this?

The reason that you don't see this in any sample code is that normally
each thread would have their own instance of the class, so they would
not share the event.

--
Göran Andersson
_____
http://www.guffa.com

[Jon Skeet [C# MVP]]

Scott Gifford <sg******@suspe ctclass.comwrot e:

I have a question about concurrency and delegates.

<snip>

If this class is used by multiple threads, it looks like there is a
race condition. If the last event is removed between the "if
(onMyEvent != null)" and the "onMyEvent( )", then "onMyEvent( )" will
throw a Null Reference Exception, right?

Yes.

So the obvious fix is to use locks. The code for the event property
is obvious, and MyEventHappened now looks like this:

private void MyEventHappened ()
{
lock(this)
{
if (onMyEvent != null)
onMyEvent();
}
}

No, it doesn't - or at least it shouldn't. Two reasons:

1) Locking on "this" is a bad idea. Lock on a private reference which
nothing else can lock on.

2) You shouldn't run other people's code (such as event handlers) while
holding the lock - as you mention below...

But now we're calling the delegate with the lock held. This seems bad
in general, since the event could start doing some long-running thing.
A more immediate problem happens if that event handler uses
form.Invoke, then the UI thread tries to call back into Class1 to do
something that requires a lock. Instant deadlock!

Exactly.

To fix the race condition, then, we have to either catch and ignore
the Null reference exception, or make a copy of it while it's locked,
then operate on our copy (which can't change behind our backs):

Just catching and ignoring the NRE wouldn't actually help - you still
need a lock (or at least a memory barrier) to make sure you get the
most recent value.

private void MyEventHappened ()
{
event MyEventHandler copyOfOnMyEvent ;
lock(this)
{
copyOfOnMyEvent = onMyEvent;
}
if (copyOfOnMyEven t != null)
copyOfOnMyEvent ();
}

(does that work? Does "copyOfOnMyEven t = onMyEvent" make a copy of
onMyEvent, or just store a reference to it?)

It takes a copy of the reference, which is good enough as delegates are
immutable.

My question is, do people actually do this? It seems quite tedious,
and I've never seen it in sample code. Is there some reason I'm not
seeing why this isn't really a race condition? Is there some other
way to deal with this?

Yes - most classes shouldn't try to make themselves thread-safe; they
should be used in a thread-safe way instead. Unless the class is doing
something inherently threaded (e.g. being a thread pool) I wouldn't try
to make the events thread-safe.

--
Jon Skeet - <sk***@pobox.co m>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Peter Duniho]

On 2007-11-11 21:42:57 -0800, Scott Gifford <sg******@suspe ctclass.comsaid :

[...]
My question is, do people actually do this? It seems quite tedious,
and I've never seen it in sample code. Is there some reason I'm not
seeing why this isn't really a race condition? Is there some other
way to deal with this?

I believe that the basic answer is yes, if the event is supposed to be
thread-safe, people do actually do _something_ like this.

One technique I've seen suggested doesn't use locking at all. It just
copies the event. I'm not really clear on why that's considered
thread-safe (it doesn't seem to me that it would be), but it's
apparently common enough that the question of the assignment to a local
variable being optimized out has come up before. Unfortunately, I
don't recall any thread that ever provided any conclusive statements
one way or the other on either issue (whether the technique works, and
whether it can be accidently defeated by the optimizer). Here's one
example of such a discussion though:

http://groups.google.com/group/micro...b4b45776a7d7a0

Anyway,

that's a long way of saying that I think the last example you posted is
in fact the close to the recommended practice for thread-safe
event-raising. The only discrepancy would be the use of "this" as
opposed to a dedicated object instance for locking.

Jon Skeet wrote a good article on the topic. You might check it out,
assuming you haven't already:
http://www.pobox.com/~skeet/csharp/events.html

I'm not sure why he didn't mention that article in his own reply.
Maybe it's because it helps you understand how to make an event
thread-safe, even though his (perfectly reasonable) reply is to suggest
you just make sure you use the event in a thread-safe way instead. :)

Personally, I think either approach is fine. But you do need to
concern yourself with thread-safety one way or the other, for an event
used from multiple threads.

Pete

[Jon Skeet [C# MVP]]

On Nov 12, 8:00 am, Peter Duniho <NpOeStPe...@Nn OwSlPiAnMk.comw rote:

<snip>

I believe that the basic answer is yes, if the event is supposed to be
thread-safe, people do actually do _something_ like this.

One technique I've seen suggested doesn't use locking at all. It just
copies the event. I'm not really clear on why that's considered
thread-safe (it doesn't seem to me that it would be),

It's thread safe in that it won't throw a NullReferenceEx ception.
However, it won't necessarily use the latest value of the variable
unless there are memory barriers in place.

but it's apparently common enough that the question of the
assignment to a local
variable being optimized out has come up before. Unfortunately, I
don't recall any thread that ever provided any conclusive statements
one way or the other on either issue (whether the technique works, and
whether it can be accidently defeated by the optimizer). Here's one
example of such a discussion though:

I believe the consensus is that under the MS CLR 2.0 memory model,
it's safe - but it's not *strictly speaking* safe under ECMA.

that's a long way of saying that I think the last example you posted is
in fact the close to the recommended practice for thread-safe
event-raising. The only discrepancy would be the use of "this" as
opposed to a dedicated object instance for locking.

And holding the lock while calling the other code - a definite "no-
no".

Jon Skeet wrote a good article on the topic. You might check it out,
assuming you haven't already:http://www.pobox.com/~skeet/csharp/events.html

I'm not sure why he didn't mention that article in his own reply.

I was late for work :)

<snip>

Jon

[Peter Duniho]

On 2007-11-12 00:56:31 -0800, "Jon Skeet [C# MVP]" <sk***@pobox.co msaid:

>One technique I've seen suggested doesn't use locking at all. It just
copies the event. I'm not really clear on why that's considered
thread-safe (it doesn't seem to me that it would be),

It's thread safe in that it won't throw a NullReferenceEx ception.
However, it won't necessarily use the latest value of the variable
unless there are memory barriers in place.

So, copying the event instance is an atomic operation? Is that because
there's some behind-the-scenes locking going on? Or is it because the
event can be copied in a single CPU-atomic operation?

I believe the consensus is that under the MS CLR 2.0 memory model,
it's safe - but it's not *strictly speaking* safe under ECMA.

So what are the chances it could change in the future? Is this
something that people should be cautioned to avoid relying on? Or is
this one of those things that so much code would break if Microsoft
changes the model that they would never change it in a way that would
break this?

>that's a long way of saying that I think the last example you posted is
in fact the close to the recommended practice for thread-safe
event-raising. The only discrepancy would be the use of "this" as
opposed to a dedicated object instance for locking.

And holding the lock while calling the other code - a definite "no-
no".

Well, the last example in Scott's post didn't have that problem. :)

Pete

[Jon Skeet [C# MVP]]

Peter Duniho <Np*********@Nn OwSlPiAnMk.comw rote:

One technique I've seen suggested doesn't use locking at all. It just
copies the event. I'm not really clear on why that's considered
thread-safe (it doesn't seem to me that it would be),

It's thread safe in that it won't throw a NullReferenceEx ception.
However, it won't necessarily use the latest value of the variable
unless there are memory barriers in place.

So, copying the event instance is an atomic operation? Is that because
there's some behind-the-scenes locking going on? Or is it because the
event can be copied in a single CPU-atomic operation?

Reference assignments are always atomic - that's guaranteed under all
the memory models I've seen.

I believe the consensus is that under the MS CLR 2.0 memory model,
it's safe - but it's not *strictly speaking* safe under ECMA.

So what are the chances it could change in the future? Is this
something that people should be cautioned to avoid relying on? Or is
this one of those things that so much code would break if Microsoft
changes the model that they would never change it in a way that would
break this?

The latter - the situation suggested is basically insane. If you can't
rely on local variables being "safe" then life becomes impossible.

that's a long way of saying that I think the last example you posted is
in fact the close to the recommended practice for thread-safe
event-raising. The only discrepancy would be the use of "this" as
opposed to a dedicated object instance for locking.

And holding the lock while calling the other code - a definite "no-
no".

Well, the last example in Scott's post didn't have that problem. :)

Indeed - somehow I'd missed that.

--
Jon Skeet - <sk***@pobox.co m>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Peter Duniho]

On 2007-11-12 11:38:41 -0800, Jon Skeet [C# MVP] <sk***@pobox.co msaid:

>So, copying the event instance is an atomic operation? Is that because
there's some behind-the-scenes locking going on? Or is it because the
event can be copied in a single CPU-atomic operation?

Reference assignments are always atomic - that's guaranteed under all
the memory models I've seen.

Ah. The bit I was missing was that I forgot a delegate is immutable.
So assigning the reference from the event field assures a valid and
unchangeable reference to a delegate (null or otherwise). If somewhere
else, the event is subscribed or unsubscribed, the event field gets a
whole new reference, rather than the delegate it references being
changed. The original reference continues to contain what it always
did.

For some reason I was thinking that assigning the event does some sort
of cloning operation. I knew, based on other information, that it was
supposed to produce a copy of the event that was assured to not change.
But I forgot why that was (and completely overcomplicated the scenario
in the process :) ).

Pete

[Jon Skeet [C# MVP]]

Peter Duniho <Np*********@Nn OwSlPiAnMk.comw rote:

So, copying the event instance is an atomic operation? Is that because
there's some behind-the-scenes locking going on? Or is it because the
event can be copied in a single CPU-atomic operation?

Reference assignments are always atomic - that's guaranteed under all
the memory models I've seen.

Ah. The bit I was missing was that I forgot a delegate is immutable.
So assigning the reference from the event field assures a valid and
unchangeable reference to a delegate (null or otherwise). If somewhere
else, the event is subscribed or unsubscribed, the event field gets a
whole new reference, rather than the delegate it references being
changed. The original reference continues to contain what it always
did.

Exactly.

For some reason I was thinking that assigning the event does some sort
of cloning operation. I knew, based on other information, that it was
supposed to produce a copy of the event that was assured to not change.
But I forgot why that was (and completely overcomplicated the scenario
in the process :) ).

Yeah, we (as a species) seem to have a habit of doing that. We make
life a lot harder and introduce magic when there's a much simpler
explanation.

--
Jon Skeet - <sk***@pobox.co m>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Scott Gifford]

Hi Jon,

Thanks for your help! Your article that Peter linked to was very
helpful as well. A few questions below...

Jon Skeet [C# MVP] <sk***@pobox.co mwrites:

Scott Gifford <sg******@suspe ctclass.comwrot e:

[...]

you still need a lock (or at least a memory barrier) to make sure
you get the most recent value.

Without any kind of locking or memory barrier, the only problem is a
short window where another thread may have added or removed a
delegate, but our thread will still use an old copy, right?

[...]

>(does that work? Does "copyOfOnMyEven t = onMyEvent" make a copy of
onMyEvent, or just store a reference to it?)

It takes a copy of the reference, which is good enough as delegates are
immutable.

Ah, thanks, I didn't realize that delegates had that property.

It's too bad that multicast delegates weren't written to just do
nothing if they have no subscribers. That would make them less
error-prone and automatically threadsafe.

[...]

most classes shouldn't try to make themselves thread-safe; they
should be used in a thread-safe way instead. Unless the class is
doing something inherently threaded (e.g. being a thread pool) I
wouldn't try to make the events thread-safe.

Most of the classes I write that generate events run in their own
threads and call events for things that happen asynchronously (for
example, network data has arrived, user activity has occurred, new GPS
data is available, etc.). In these cases, it seems to me they have to
be thread-safe one way or another. Whenever an object decides it's no
longer interested in these events, it will need to unsubscribe. With
a race condition between the unsubscription and an asynchronous event
arriving, there's not really a safe time to unsubscribe.

There are certainly other options besides locking. For example, I
could require that the object device be closed before unsubscribing,
and promise not to generate events when the object is closed. But
this is just another way of implementing thread-safety, and doesn't
really seem any simpler than using locks.

Thanks for your help and for any further thoughts!

---Scott.