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 (14)

  1. Yikes. So the event field is not thread safe, but the reference to it is? How does that work "under the covers"?

  2. Matt Warren says:

    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.

  3. Something to look out for.

  4. Dumky says:

    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)?

  5. Dumky says:

    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

  6. Thomas Eyde says:

    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?

  7. Derek LaZard says:

    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.

  8. 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.

  9. 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.

  10. 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.