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 (copyOfOnMyEvent != null)
copyOfOnMyEvent();
}

(does that work? Does "copyOfOnMyEvent = 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?=]

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.

 

[Jon Skeet [C# MVP]]

I have a question about concurrency and delegates.

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 (copyOfOnMyEvent != null)
copyOfOnMyEvent();
}

(does that work? Does "copyOfOnMyEvent = 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.

 

[Peter Duniho]

[...]
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/micr...read/thread/3a0c55a45e964f6c/f3b4b45776a7d7a0

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]]

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 NullReferenceException.
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]

It's thread safe in that it won't throw a NullReferenceException.
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?

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]]

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.

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.

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

Indeed - somehow I'd missed that.

 

[Peter Duniho]

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]]

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.

 

[Scott Gifford]

Hi Jon,

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

[...]

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?

[...]

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.

 

[Peter Duniho]

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?

I suspect that the issue Jon's talking about is that without a memory
barrier, the "old copy" could be _very_ old. If the event field had
been optimized into a register, or is being kept in the CPU cache, etc.
then the version used when raising the event could be so out of date,
that even accounting for thread execution ordering you still wouldn't
get the correct result.

Put another way, another thread could have changed the event field
before the method raising the event gets a copy of the field, and the
method raising the event might still not get the changed value set by
the other thread.

If I recall correctly, while the "volatile" keyword could solve this
general issue, events can't actually have that keyword using the C#
shortcut syntax (i.e. using the "event" keyword). Of course, to use
the lock() statement properly to address the issue, you'd have to
explicitly declare your event anyway (as shown in Jon's article), so if
you're going to do that I suppose you could just use "volatile" with
the delegate field associated with the event to address the issue,
rather than actually locking.

Or, I could be wrong and Jon's talking about something completely
different. In which case, I'll just have to be corrected again.

Pete

 

[Jon Skeet [C# MVP]]

I suspect that the issue Jon's talking about is that without a memory
barrier, the "old copy" could be _very_ old. If the event field had
been optimized into a register, or is being kept in the CPU cache, etc.
then the version used when raising the event could be so out of date,
that even accounting for thread execution ordering you still wouldn't
get the correct result.

Exactly. It's frankly unlikely, but should be considered.

If I recall correctly, while the "volatile" keyword could solve this
general issue, events can't actually have that keyword using the C#
shortcut syntax (i.e. using the "event" keyword). Of course, to use
the lock() statement properly to address the issue, you'd have to
explicitly declare your event anyway (as shown in Jon's article), so if
you're going to do that I suppose you could just use "volatile" with
the delegate field associated with the event to address the issue,
rather than actually locking.

Well, that would *nearly* make it thread-safe - but not quite.
Consider two subscriptions occurring at the same time. I'll represent
delegate instances as lists of actions in square brackets. Suppose we
start with [a] and we have subscriptions of and [c]. Without any
locking, we could get:

Thread 1: read variable onto stack - result = [a]
Thread 2: read variable onto stack - result = [a]
Thread 1: call Delegate.Combine ([a], ) - result = [a, b]
Thread 2: call Delegate.Combine ([a], [c]) - result = [a, c]
Thread 1: store [a, b] in variable
Thread 2: store [a, c] in variable

The result is that the subscription of "b" is lost.

It's the equivalent of incrementing a volatile variable without using
a lock or Interlocked - even though each operation of atomic and
volatile, the combined operation isn't.

Or, I could be wrong and Jon's talking about something completely
different. In which case, I'll just have to be corrected again.

Nope, you were spot on apart from the problem above - which I hadn't
thought of before either!

Jon

 

[Peter Duniho]

[...]

If I recall correctly, while the "volatile" keyword could solve this
general issue, events can't actually have that keyword using the C#
shortcut syntax (i.e. using the "event" keyword). Of course, to use
the lock() statement properly to address the issue, you'd have to
explicitly declare your event anyway (as shown in Jon's article), so if
you're going to do that I suppose you could just use "volatile" with
the delegate field associated with the event to address the issue,
rather than actually locking.

Well, that would *nearly* make it thread-safe - but not quite.

Sorry, I should have been more clear. I meant using "volatile" _in
addition to_ the lock() semantics you show on your delegate/event
article. That is, you'd still lock in the add and remove methods, just
not when raising the event.

I wouldn't worry so much about locking when copying the current event
reference when raising it, since as you've pointed out that's going to
be atomic and you're assured of getting consistent results. You'll get
the most up-to-date value at the time of copying the reference. Of
course, some other thread could replace that value with a newly
subscribed or unsubscribed value immediately after, but I don't see
that as a problem. That risk exists even if you lock when getting the
value so that you can raise the event, and if the exact order is
important there some other mechanism needs to be added to deal with
that.

But certainly subscribing and unsubscribing need to be synchronized to
avoid the problem you've mentioned here.

Pete

 

[Jon Skeet [C# MVP]]

Sorry, I should have been more clear. I meant using "volatile" _in
addition to_ the lock() semantics you show on your delegate/event
article. That is, you'd still lock in the add and remove methods, just
not when raising the event.

Right. Yes, that would do it.

I wouldn't worry so much about locking when copying the current event
reference when raising it, since as you've pointed out that's going to
be atomic and you're assured of getting consistent results. You'll get
the most up-to-date value at the time of copying the reference. Of
course, some other thread could replace that value with a newly
subscribed or unsubscribed value immediately after, but I don't see
that as a problem. That risk exists even if you lock when getting the
value so that you can raise the event, and if the exact order is
important there some other mechanism needs to be added to deal with
that.

Absolutely - it's an inherent race condition.

But certainly subscribing and unsubscribing need to be synchronized to
avoid the problem you've mentioned here.

Yup. One nice feature of anonymous methods I've started using is to
declare the delegate variable with an initializer:

private EventHandler foo = delegate { };

That means you can avoid the nullity check, so the complete pattern
becomes:


private readonly object padlock = new object();
private volatile EventHandler foo = delegate { };

public event EventHandler Foo
{
add
{
lock (padlock)
{
foo += value;
}
}
remove
{
lock (padlock)
{
foo -= value;
}
}
}

public void OnFoo()
{
foo(this, EventArgs.Empty);
}