Safely firing an event
This came up on an internal alias, and I thought I should spread it around.
If you’re going to fire an event, you may have code like this:
void F()
{
if (SomeEvent != null)
{
SomeEvent();
}
}
There’s a race condition here. If another thread removes the last handler between the if() and the call, then it’ll crash.
The safe way is to use the copy-and-test pattern:
void FireSomeEvent()
{
D temp = SomeEvent;
if (temp != null)
{
temp();
}
}
Comments
- Anonymous
March 19, 2004
Yikes. So the event field is not thread safe, but the reference to it is? How does that work "under the covers"? - Anonymous
March 19, 2004
You can always safely fire a temp, no need to make an event out of it. It's the FTE's you have to be careful with. - Anonymous
March 19, 2004
Something to look out for. - Anonymous
March 19, 2004
Wow that is bad! Yet another "leaky abstraction" problem.
I have been wondering about a similar problem related to the WeakReferences. What happens if the reference is collected between the call to IsAlive and getting the Target (and using it)? - Anonymous
March 19, 2004
Maybe the MSDN samples should be improved, all the ones I've seen use the straightforward/dangerous implementation.
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpguide/html/cpconprovidingeventfunctionality.asp - Anonymous
March 19, 2004
Why isn't this built into the event handler? Why should I care if I have any listeners or not? Even worse, why should the removal of the last handler/listener nullify my object?
I find it peculiar at best that:
// SomeEvent == null;
SomeEvent += new SomeEvent(...);
// SomeEvent != null
SomeEvent -= someHandler;
// SomeEvent is sometimes null ??
I think it is a design flaw that event invoking is not always safe. My object should control the lifetime of the event, so the following should always be safe:
SomeEvent e = new SomeEvent();
// clients can do as many +=, -=, AddHandler, RemoveHandler() as they want
// I don't care as I expect this to always be safe:
e(); // or e.Invoke();
What were they thinking? - Anonymous
March 19, 2004
Event Delegates are "Thread-Safe"; but users are not necessarily "Thread-Safe."
If a class/type is stamped "Thread-Safe" then these types of issues need to be addressed by the author. This issue just shows the complexity of concurrency -- the complexity of multi-threaded applications.
BTW, non-static methods of Delegate instances are not guaranteed to be thread-safe. - Anonymous
March 20, 2004
Thanks for the good comments on this post, folks.
Thomas: I think this was an oversight in the design of the language. Today the language designers agree that a threadsafe Invoke is the desired behavior. If I remember correctly, they didn't want to change the behavior now, just in case some code is depending on it.
I'm pretty sure that VB is changing their behavior in this way, but I think VB users expect that more than C# users do.
We also considered adding an 'invoke' keyword to the language, so you could do 'invoke(e)'. But when we showed it to customers, they said it wasn't worth it for this minor issue.
Dumky: thanks for the pointer to broken help. - Anonymous
March 20, 2004
Jay, did somebody reproduce this at Microsoft and that's how the internal alias message got passed around? If so, it would be cool if you could post that sample code. - Anonymous
March 20, 2004
Sean: the discussion was more abstract. In the course of the thread, I posted the guidance you see here about thread safety, and then decided to blog about it, too.
Recently EricGu said to us that any time we explain something to somebody, we should consider if it would make sense to post it on a blog. I've tried to take that advice to heart. - Anonymous
March 21, 2006
PingBack from http://www.conchoid.com/2006/03/21/dealing-with-lazy-design-habits/