is firing events threadsafe?

  • Thread starter Thread starter cody
  • Start date Start date
C

cody

the common used idiom is like this:

MyDelegate d = MyEvent;
// receiver of the event unsubscribes after this line in another thread
if (d!=null)
d(this, EventArgs.Empty);

Am I right if I think that it is possible that in a multithreaded
environment an object may receive an event *after* it has unsubscribed from
that event?

obj.MyEvent -= myMethod;
// is it possible that this object can still receive an event after this
line?
 
cody said:
the common used idiom is like this:

MyDelegate d = MyEvent;
// receiver of the event unsubscribes after this line in another thread
if (d!=null)
d(this, EventArgs.Empty);

Am I right if I think that it is possible that in a multithreaded
environment an object may receive an event *after* it has unsubscribed from
that event?
Yes.

obj.MyEvent -= myMethod;
// is it possible that this object can still receive an event after this
line?
Click to expand...

Absolutely. Indeed, the code above isn't thread-safe - see
http://www.pobox.com/~skeet/csharp/threads/lockchoice.shtml

Bear in mind that on a multi-processor machine, the two statements
could be executing at *exactly* the same time!
 
So at the end which method shall be used for threadsafe firing of
events?

protected virtual OnSomeEvent(EventArgs e)
{
lock (someEventLock)
{
if (someEvent != null)
{
someEvent(this, e);
}
}
}

this method may, according to your website, lead to a deadlock.

protected virtual OnSomeEvent(EventArgs e)
{
if (MyEvent != null)
{
MyEvent(this, e);
}
}

this may lead to an NullReferenceException..

protected virtual OnSomeEvent(EventArgs e)
{
Delegate someEvent = MyEvent;
if (someEvent != null)
someEvent (this, e);
}


and this method could cause that an object receives an event *after*
unsubscribing to it.

So what is the best way to do it? Is the following the best solution?

protected virtual OnSomeEvent(EventArgs e)
{
try
{
MyEvent(this, e);
}
catch (NullReferenceException)
{
}
}
 
cody manix said:
So at the end which method shall be used for threadsafe firing of
events?

protected virtual OnSomeEvent(EventArgs e)
{
lock (someEventLock)
{
if (someEvent != null)
{
someEvent(this, e);
}
}
}

this method may, according to your website, lead to a deadlock.
Yup.

protected virtual OnSomeEvent(EventArgs e)
{
if (MyEvent != null)
{
MyEvent(this, e);
}
}

this may lead to an NullReferenceException..
Yup.

protected virtual OnSomeEvent(EventArgs e)
{
Delegate someEvent = MyEvent;
if (someEvent != null)
someEvent (this, e);
}
Click to expand...

That's not thread-safe. Use (as per the website):

protected virtual OnSomeEvent(EventArgs e)
{
SomeEventHandler handler;
lock (someEventLock)
{
handler = someEvent;
}
if (handler != null)
{
handler (this, e);
}
}

There's no way of avoiding the possibility of an event being fired
after an object has unsubcribed from it.
and this method could cause that an object receives an event *after*
unsubscribing to it.

So what is the best way to do it? Is the following the best solution?

protected virtual OnSomeEvent(EventArgs e)
{
try
{
MyEvent(this, e);
}
catch (NullReferenceException)
{
}
}
Click to expand...

Absolutely not. That's not thread-safe, abuses exceptions, and is
generally nasty.

If you need to make sure that something doesn't happen after you've
unsubscribed, you should have a separate boolean variable within the
class which receives the event, and set it before you unsubscribe. Then
if that variable is set when the event is fired, just return.
 
There's no way of avoiding the possibility of an event being fired
after an object has unsubcribed from it.


Absolutely not. That's not thread-safe, abuses exceptions, and is
generally nasty.
Click to expand...

But it could work if I could declare the event as volatile which is
obviously not possible because it gives me

public volatile event EventHandler MyEvent;

"CS0106: The modifier 'volatile' is not valid for this item"
If you need to make sure that something doesn't happen after you've
unsubscribed, you should have a separate boolean variable within the
class which receives the event, and set it before you unsubscribe. Then
if that variable is set when the event is fired, just return.
Click to expand...

That would move the problem to the consumer of the event and is IMHO no
"pretty" solution.
 
cody said:
But it could work if I could declare the event as volatile which is
obviously not possible because it gives me

public volatile event EventHandler MyEvent;

"CS0106: The modifier 'volatile' is not valid for this item"
Click to expand...

Indeed. You can make the delegate itself volatile, but not the event. I
generally don't like volatile variables though - it's too easy to miss
the fact that it's volatile, whereas a lock statement makes it pretty
clear what's going on.
That would move the problem to the consumer of the event and is IMHO no
"pretty" solution.
Click to expand...

No, it's not particularly pretty - but it works and it's usually fairly
easy to implement, which is more than can be said for most other
potential solutions.

It's possible that by making sure that the event handler *always* has
at least one delegate (by providing a private "no-op" delegate which is
never removed) you *may* be able to get round the issue, but I'm not
entirely sure.
 
Jon Skeet said:
It's possible that by making sure that the event handler *always* has
at least one delegate (by providing a private "no-op" delegate which is
never removed) you *may* be able to get round the issue, but I'm not
entirely sure.
Click to expand...

On second thoughts, that definitely doesn't work.

Consider the case where you've got an event with 10,000 subscribers,
and the one you want to remove is the last one.

You could call foo.SomeEvent -= whatever; and the call could complete
and have removed the handler from the list which would be used next
time, but if the event is already in the middle of firing, the handler
will still get called, because the multicast delegate which is being
iterated through wouldn't be changed, just the value of the variable
itself, to another multicast delegate.
 
What about ISynchronizeInvoke objects?

This should make event raising thread-safe. No?

-Moty-
 
Back
Top