Wednesday, February 28, 2007

Creating and Raising Events - The Right Way

Editors Note : There is a follow up post to this one with critical information. If you read this article, please read the follow up as well.

I've seen quite a few people new to C# who haven't grasped the correct/best way to declare and raise events, particularly when writing objects you expect other developers to inherit from.

The most common mistake I see made is pre-fixing the word On to event names, i.e OnProgressChanged.

This is a no-no because because every event (at least in a class that may be inherited from) should have a protected method with that name (i.e OnEventName). For example, if you wanted to have an event that occurred when progress of a long-running operation changed, you'd declare the following (or something like it);

public event EventHandler<ProgressChangedEventArgs> ProgressChanged;

protected virtual OnProgressChanged(ProgressChangedEventArgs e)
{
if (ProgressChanged != null)
ProgressChanged(e);
}

Editors Note : The above sample code has a race condition in it. See the follow up post for the correct way to do this.

The reason for the protected On method is to allow class deriving from this one to raise the event. In .NET you can only raise an event from within the class that declared it, so if you tried to say

if (ProgressChanged != null)
ProgressChanged(e);

in a derived class, you would receive a compile error. The reason for this is that good design dictates only the object that owns the event should know when to raise it, and so only it should be able to. However, since code in derived classes are really part of the object they should also be allowed this privilege. By providing this method to raise the event, and making it protected, we ensure that only the root class, and any derived classes but no external code, can raise the event.

The virtual keyword in the On method declaration is not required, but I prefer to use it as it allows derived classes to know when the event is about to be raised and either prevent it, alter the event arguments, or raise other events/execute other code before or after the event is raised.

Another, less common problem I've seen is code that fails to check if the event delegate is null before invoking it. In .NET you raise an event by invoking a multi-cast delegate, which is a fancy way of saying you just write the event name, i.e

DisplayProgress(e);

will invoke the DisplayProgress event - BUT- if there is no one connected to the event yet a null reference exception will occur, which is why you should always put this first;

if (EventName != null)

Even if you wanted an exception thrown when no one was connected to your event, I would suggest throwing a custom exception with a more meaningful message, rather than allowing the generic null reference exception to be thrown.

Finally, I often see code that explicitly defines a delegate for each event. For example;

public delegate void MyEventDelegate(object sender, MyEventArgs e);

public event MyEventDelegate MyEvent;

This is fine, and there is nothing inherently wrong with it, but Microsoft have (thankfully) provided us with a shortcut, using a generic delegate for all events. The above code can be simplified to;

public event EventHandler<MyEventArgs> MyEvent;

This is not only less typing, but means there's fewer declarations floating around polluting your intellisense/namespaces.

One last thing, some times I've seen code that declares events using delegates that don't conform to normal event handler signatures, i.e

public delegate void MyEventDelegate(int myValue);
public event MyEventDelegate MyEvent;

The problem here is the delegate used doesn't have a sender parameter (declared as an object), nor does it use an object inheriting from EventArgs to provide the myValue integer value to the event handlers.

This is bad practice. All events in the .NET framework and associated libraries (or at least all the ones I've seen) conform to the object sender, eventargs e syntax and it's going to be expected from most people coding against your events. Also, by not using an object inheriting from the EventArgs class to pass your event parameters you're going to have to do a lot more refactoring if you want to add or remove parameters (and any third party code written against the old signature will need to refactoring too). Finally, you haven't provided a reference to the object raising the event, so if someone has a single event handler receiving event notifications from multiple sources, they'll have no idea where the event came from.

The only time it *might* be acceptable to break from this standard is when you don't want to pass any parameters at all, and are 100% sure you never will. In which case it might be ok to pass only a reference to the sender and no EventArgs. I must admit that having to create/receive an instance EventArgs always seems fairly pointless and stupid to me, but the benefit is (again) that you don't have to refactor so much if you add parameters later.

Invoke This !

Often when you're multi-threading you'll want to invoke back to another thread (usually the main thread in your program). This isn't very hard to do, and often leads to code like this;

this.Invoke(new DisplayProgressDelegate(this.DisplayProgress), percentageComplete);

This is fine, but there are a few occasions when this is annoying;

1. Sometimes you may not be sure if you were called from another thread or not. Perhaps you are sometimes called by your own code and sometimes called from an event or via a callback that may or may not be on a background thread. If you're not on another thread, you may not want the overhead of the delegate creation/invoke call.

2. It's a lot more typing that just

DisplayProgress(percentageComplete);

3. It looks untidy, especially if repeated often in your code.

4. You may be about to call someone else's code, either through a call back or an event, and you are unsure if they are multi-threading aware, i.e you're about to raise an event from a background thread and untrusted 3rd party plug-ins may or may not try to access user-interface objects in their event handlers and crash as a result. You'd like protect yourself from this and avoid the problem even if the 3rd party coders didn't do the right thing.

The good news is; there is a fiendishly simple way to overcome these issues. Let's say you want to be able to call a method called DisplayProgress and have it invoked only if needed. All you need to do is alter your DisplayProgress routine so it looks like so;

public void DisplayProgress(int percentageComplete)
{
if (myMainForm.InvokeRequired)
myMainForm.Invoke(new DisplayProgressDelegate(this.DisplayProgress), percentageComplete);
else
{
//TODO: Put your normal code for this procedure here.
}
}

and then you can call it from anywhere, like normal;

DisplayProgress(50);

Ok, let's examine what's going on here. The first line in the DisplayProgress method checks to see if you're on the right thread by calling InvokeRequired on a form (which was presumably created on your user interface thread).

If you're on the same thread as the form and no invoke is required, the else condition is executed and no invoke occurs.

If you're on a different thread, the function invokes itself (recursion is cool :) ). When the method enters itself for the second time, it's on the right thread so the else condition executes. When the code is finished the function returns to it's previous call (the invoke), which has no code following it because all the work is done in the else condition, so the method simply ends and returns to the calling procedure.

The end result is that it doesn't matter which thread you call the method from, it will always execute on the thread that myMainForm was created on.

This can be used for any method, including OnEventName methods that raise events, ensuring that everyone who receives the event is running on the thread that is safe for user-interface access, protecting newbie developers writing plug-ins for your code from the hairy world of multi-threading.

There are a few variations on this syntax. You can, for example, create the delegate pointing to the method as a module level variable and re-use the same instance on each call, which reduces the number of objects created/collected by the .NET framework and gives you a small performance boost and reduces memory pressure if the function may be called many times in quick succession.

One caveat using this code; this code assume that you only ever want to cast to a single thread (the one that myMainForm was created on in this example). If you wanted to cast to different threads then you'd need to pass in a form or control object to the method and use it's InvokeRequired/Invoke members.