Extension methods for safely firing events

Question

I wrote a set of extension methods for the EventHandler class that add the method Fire, which raises the event after creating a local reference for thread safety, and checking to make sure the event had subscribers.

public static class EventExtensions
{
 /// <summary>
 /// Raises an event thread-safely if the event has subscribers. 
 /// </summary>
 /// <param name="me"> The event handler to raise. </param>
 /// <param name="sender"> The object that sent this event. </param>
 /// <param name="args"> The event args. </param>
 [SuppressMessage("Microsoft.Design",
 "CA1030:UseEventsWhereAppropriate",
 Justification = "This warning comes up when you use the word `Fire` in a method name. This method specifically raises events, and so does not need changing.")]
 public static void Fire(this EventHandler me, object sender, EventArgs args)
 {
 var handler = me;
 if (handler != null)
 {
 handler(sender, args);
 }
 }
 /// <summary>
 /// Raises an event thread-safely if the event has subscribers. 
 /// </summary>
 /// <typeparam name="T"> The type of EventArgs the event takes. </typeparam>
 /// <param name="me"> The event handler to raise. </param>
 /// <param name="sender"> The object that sent this event. </param>
 /// <param name="args"> The event args. </param>
 [SuppressMessage("Microsoft.Design",
 "CA1030:UseEventsWhereAppropriate",
 Justification = "This warning comes up when you use the word `Fire` in a method name. This method specifically raises events, and so does not need changing.")]
 public static void Fire<T>(this EventHandler<T> me, object sender, T args) where T: EventArgs
 {
 var handler = me;
 if (handler != null)
 {
 handler(sender, args);
 }
 }
 /// <summary>
 /// Raises a static event thread-safely if the event has subscribers. 
 /// </summary>
 /// <param name="me"> The event handler to raise. </param>
 /// <param name="args"> The event args. </param>
 [SuppressMessage("Microsoft.Design",
 "CA1030:UseEventsWhereAppropriate",
 Justification = "This warning comes up when you use the word `Fire` in a method name. This method specifically raises events, and so does not need changing.")]
 public static void Fire(this EventHandler me, EventArgs args)
 {
 me.Fire(null, args);
 }
 /// <summary>
 /// Raises a event thread-safely if the event has subscribers. 
 /// </summary>
 /// <typeparam name="T"> The type of EventArgs the event takes. </typeparam>
 /// <param name="me"> The event handler to raise. </param>
 /// <param name="args"> The event args. </param>
 [SuppressMessage("Microsoft.Design",
 "CA1030:UseEventsWhereAppropriate",
 Justification = "This warning comes up when you use the word `Fire` in a method name. This method specifically raises events, and so does not need changing.")]
 public static void Fire<T>(this EventHandler<T> me, T args) where T: EventArgs
 {
 me.Fire(null, args);
 }
}

The two top methods are used on instance events, and the two bottom methods on static events, which specifically need to pass a null value for sender.

Usage:

public event EventHandler Clicked;
public event EventHandler<LoadedEventArgs> Loaded;
public void Click()
{
 Clicked.Fire(this, EventArgs.Empty);
}
public void Load()
{
 Loaded.Fire(this, new LoadedEventArgs("data!"));
}

And for statics:

public static event EventHandler Clicked;
public static event EventHandler<LoadedEventArgs> Loaded;
public void Click()
{
 Clicked.Fire(EventArgs.Empty);
}
public void Load()
{
 Loaded.Fire(new LoadedEventArgs("data!"));
}

I'm looking for general comments on the entire thing, but particularly thread safety and security concerns.

• Magus: For the last two cases, the .Fire can be removed with no difference in functionality, so these two methods aren't getting you much. The ones with the checks actually do something potentially useful.
• craftworkgames: I have these exact same methods in my code base, except I called them Raise. Btw, the class should probably be called EventHandlerExtensions
• Nick Udell: @Magus I mainly added those because I found myself writing a class with a fair number of static events, and writing null for the sender each time was getting old fast. You're right that they could easily be removed, but while there's no difference in functionality, the couple of keypresses each time saved were worth it to me. I shall probably remove them from the my central code repository, and keep them local to that specific project, thanks!
• Nick Udell: @craftworkgames Thanks, good catch on the naming.
• svick: The pattern I like to use to avoid the null check is: public event EventHandler Clicked = delegate { };. This makes sure the even is never null, so the check is not needed.

Answer 1

var handler = me;

is not needed - you are already guaranteed that the passed in instance can never change.

So if is null it will stay null, but if it is non-null you are also guaranteed it will stay non-null even under any kind of multithreading situation. (note that the null-check itself is still needed!)

---

For some further explanation:

This assignment was only needed in "pre-extension-method" times when it was common to have separate EventRaisers directly in the defining class:

event EventHandler Foo;
void OnFoo()
{
 if (Foo != null) Foo(this, EventArgs.Empty);
}

Clearly, if another thread coincidentally executed Foo -= LastHandler; just after the if block but before the Foo() call, you would run into a NullReferenceException, which was commonly avoided by adding the following assignment:

event EventHandler Foo;
void OnFoo()
{
 var temp = Foo;
 if (temp != null) temp(this, EventArgs.Empty);
}

I know that immutable reference types can be a bit mind-boggling, but it should be clear that your passed in "me" parameter can never "spontaneously" become null, even if another thread performed Foo -= LastHandler; in this mentioned "worst-case-moment".

• Nick Udell: Thank you, I added the local copy because I wasn't sure if the reference was actually copied for an extension method or not, good to see it isn't! The null check, is also my own foolishness, and force of habit beating logic there. Thanks for clearing things up!
• RaphM: It also took me quite some time to grasp why the assignment is not needed with the extension method approach but was needed with the "classic" approach - after all its a bit tricky to understand how += and -= work although delegates are immutable.
• Nikita B: I think you should elaborate on "parameter can never get null" part. Right now it sounds like "remove dat null check!" :) While, obviously, parameter will be null, if given event has 0 subscribers. And you still need that null check, to avoid the exception.
• RaphM: Ok I changed my answer to be more clear about that.

Answer 2

Your class looks fine to me. I think every project have a variation of it. A few minor issues i see:

• EventHandler me - "me" is not very descriptive name for a parameter.
• I think you might want to also add an overloaded method which does not take event args (and uses EventArgs.Empty instead)
• If you want to cover all common cases, you can also add extensions for Action and Action<T>. Because, lets face it, no sane person would use EventHandler in internal code :)
• I think passing null to sender is a really bad practice. If sender is irrelevant and you do not want to pass it, then you should use different delegate type.

• Nick Udell: I find naming the self-referential variable in extension methods to always be a problem (since "this" is sadly reserved), do you have any recommendations? As for passing null to sender for static events, it's explicitly recommended by Microsoft: msdn.microsoft.com/en-gb/library/vstudio/… and using EventHandler where possible is also recommended: msdn.microsoft.com/en-us/library/edzehd2t(v=vs.110).aspx
• Nikita B: @NickUdell, emm, how about "eventHandler"? :)
• Nikita B: @NickUdell, I believe Microsoft guidelines apply to public API's. So if you are developing a library, then you should stick to EventHandlers for public events. I see nothing wrong with using other delegates in internal code though, if they are a better fit in given context. Spawning hundreds of EventArgs classes and passing senders all the time can get old really fast, as you mentioned yourself. :) As for passing null to static events being a recommended practice: i did not know that, thx.