The Devil Is In The Details

In quite few programs I see following pattern:

void OnMyEvent(EventArgs e) {
    if (this.MyEvent != null) {
        this.MyEvent(this, e);
    }
}

I must confess that I use same pattern in quite a few pieces of my own code. Nevertheless, this is wrong.

Issue here is that MyEvent can change between check and call statements. If change consists of adding one more delegate everything is fine. If change is removing all existing delegates you are in trouble.

Here is scenario. First line will check whether MyEvent is different than null (which it is at that point in time) and, exactly at that time, control switches to another thread and makes MyEvent null. Once our thread resumes it will use MyEvent which is now null. Exception!

This issue is highly timing sensitive and it is near impossible to reproduce it. You may have that bug for couple of years and never hit it. But it is there.

There are two distinct paths to remove this problem. One is just putting everything into lock statement. This will fix issue but it will also introduce overhead, performance problems and even potential deadlocks. It all depends on what exactly is done in called method. It is not a pattern I would be comfortable with.

Another path is to exploit fact that event delegates are immutable. Every instance has all data needed and if you copy it to local variable there is no need to worry about it changing value. With simple change we now have correct code:

void OnMyEvent(EventArgs e) {
    var ev = this.MyEvent;
    if (ev != null) {
        ev(this, e);
    }
}