The Event Handler That Cried Wolf

I ran into an interesting and unexpected behavior in ASP.NET AJAX event handling the other day. Once I figured out what was going on, I was almost positive it was a bug, so I started looking into what a solution would be. But just out of curiosity, I looked at what the behavior was for a server-side event. Much to my surprise, the behavior was the same. The behavior then was consistent with the server-side behavior, not a bug. But is it the "correct" behavior? Tell me what you think...

To put it in the most shocking way possible: Removing a handler from an event does not guarantee that handler will not be called when the event fires!

SACREBLEU!!

But it's true. Take a look at this code...

public class Bar {
    public event EventHandler SomeEvent;
 
    public void RaiseEvent() {
        if (SomeEvent != null) {
            SomeEvent(this, null);
        }
    }
}
 
public class Foo {
    private Bar _bar;
    private EventHandler _handler;
    private ArrayList _list;
 
    public void Initialize(Bar bar) {
        _bar = bar;
        _list = new ArrayList();
        // listen to the event on bar
        _handler = new EventHandler(OnSomeEvent);
        _bar.SomeEvent += _handler;
    }
 
    private void OnSomeEvent(object sender, EventArgs args) {
        // event was raised, do something
        // this cant happen if Stop() was called.... right?
        _list.Add("blah");
        Console.WriteLine("OnSomeEvent");
    }
 
    public void Stop() {
        // tidy up, stop listening to the event and clear the list
        _bar.SomeEvent -= _handler;
        _list = null;
    }
}

Now, this is a totally contrived example, so the code here isn't exactly useful. But the basic idea can occur in a real program. Here, there's a Bar component that exposes an event, SomeEvent. Normally a component raises it's event when appropriate and doesn't have an actual RaiseEvent() method to raise it, but for demo purposes, it's got one so we can fire the event later on.

The Foo component has two public methods -- Initialize, and Stop. Initialize takes a Bar component, and it adds an event listener to the event. Stop removes the event listener, since after all, it is important to clean up after yourself when it comes to events. If the handler was never removed, the Bar component will forever have a reference to the Foo, and Foo will live as long as Bar lives.

When the SomeEvent fires, Foo adds to an ArrayList which it has internally created. Notice in Stop() it sets the ArrayList to null. The event handler OnSomeEvent adds to the list without checking it for null.

If somehow the event handler were called even after Stop(), we'd have a null reference exception! Can't happen, right? When you think about a typical use of these components, it doesn't appear to be possible... Here would be one way of raising the event and removing the handler.

public class Program {
    public static void Main(string[] args) {
        Bar bar = new Bar();
        Foo foo = new Foo();
 
        foo.Initialize(bar);
        bar.RaiseEvent();
        foo.Stop();
 
        Console.ReadLine();
    }
}

image

No problem there. But what if an event handler for SomeEvent removes the event handler of a separate component? Something like this...

public static void Main(string[] args) {
    Bar bar = new Bar();
    Foo foo = new Foo();
 
    bar.SomeEvent +=
        delegate(object sender, EventArgs a) {
            foo.Stop();
        };
    foo.Initialize(bar);
    bar.RaiseEvent();
    foo.Stop();
 
    Console.ReadLine();
}

The result? BOOM...

NullReferenceException

Even though Foo has removed its listener, the listener is called. The reason is because its listener was removed after the event was already underway -- and, I suppose for multi-threaded reasons, once an event begins, the listeners that existed at the time will fire even if they are removed in the process.

Again, this is a very contrived example. But it could happen more innocently and not be so obvious. Perhaps when SomeEvent occurs, under a certain condition, a listener in the application decides to shut down some container which contains a Foo component, and that shutdown process calls Stop() on Foo. The first listener doesn't know that will happen. And Foo doesn't know Stop() was called as a result of an earlier listener to the very event it is unhooking from. Who is supposed to know what is going on?

So be careful, I suppose is the lesson to be learned. If a component may end up removing an even listener as a result of code external to it, it is entirely possible that it is the very event it is removing the listener from that is currently executing and causing it. In that scenario, you shouldn't assume you are safe from the listener being called.

18 Comments

  • yyeeeaaaaaa... and no...
    I mean, you initialized it your self before the official initialization method, I can see the point though

    good stuff as always

    any chance to open up the full rss feed?

  • The delegate executed when you raised the event was a composite delegate A with two event handlers: your anonymous delegate and Foo's OnSomeEvent method.

    Removing the OnSomeEvent handler from the composite delegate creates a new composite delegate A' with just the anonymous delegate, rather than changing the composite delegate already in use. After the anonymous delegate completes, the next delegate in A executes and tries to access a null _list.

    If you had tried to add a SomeEvent event handler instead of removing one in Foo.Stop, it wouldn't execute until the next time you called Bar.RaiseEvent().

  • Joe, yeah thats what I'm illustrating here, but my point is it isnt always obvious that this might happen to you. If you developed the Foo component separately from this application, I dont think you would have expected your handler to get called after you remove it.

  • I don't get it exactly ...

    You have two objects: bar and foo.

    You add an EventHandler to bar (the delegate foo.Stop() ).

    Then you call Initiliaze on foo and in that method you ADD another EventHandler to bar (OnSomeEvent).

    Then you call RaiseEvent on the bar object. This will call the two EventHandlers you just added, first foo.Stop() and then OnSomeEvent. These handlers are queued.

    The first handler removes the second handler, but it will still get called as it is in the event queue.

    You receive an error. This is all quite logical to me. in foo.Stop() _list is set to null and then the second handler tries to add to it.

    It's easy to see that even though you remove the EventHandler does not remove the already queued Event. All that is achieved that the next time RaiseEvent() is called SomeEvent will not be queued.

    When you move the line foo.Initiliaze(bar) a line up you will see the code fail a bit further down at foo.Stop(). That will try to remove the SomeEvent() EventHandler that already has been removed.

    The biggest problem with the code is that you are removing EventHandlers from within EventHandlers (and no null checking, but that was for demonstration purposes I assume). This can lead to unpredictable behavior.
    Just as you demonstrated.

    Nice puzzle ;-)

  • Rob -- yeah, yes, and yup. It's all very 'duh' in the example. My point though seems to be missed. Perhaps I develop a component that looks like Foo, in isolation. I do not know that when someone calls my public method Stop that they might be doing so within a handler to SomeEvent that happens to come before mine. Just look at it from Foo's perspective. It removes the handler. Then it gets called. I dont think most developers would expect that is even a possibility, and could cause errors like the null reference exception in this example.

  • Is it true that the framework doesn't make any guarantees about which event handler gets called first? If so sometimes your example app would work and sometimes it would hit the exception.

  • Matt -- events dont have to use multicast delegates though, so you really shouldnt depend on the behavior. Its also likely that the circumstances around how the event handlers are added is sensitive and could change order. For example, in an ASP.NET application, it might depend on which order the controls are in the page, and what event in the lifecycle they add the handler in.

  • Effectively the call to SomeEvent(this, null) in the desktop FX is roughly analogous to:

    Delegate[] handlers = SomeEvent.GetInvocationList() // static list of handlers
    for (var i = 0, l = handlers.Length; i < l; i++) {
    handlers[i].DynamicInvoke(new object[] { this, null });
    }

    Removing a handler from SomeEvent changes the internal invocation list, not the copy.

    In the Ajax FX, the EventHandlerList object functions in a similar fashion:

    var handler = this.get_events().getHandler("SomeEvent");
    if (handler) handler(this, null);

    internally getHandler creates a copy of the invocation list (see Sys$EventHandlerList$getHandler in MicrosoftAjax.debug.js)

    var evt = this._getEvent(id);
    if (!evt || evt.length == 0) return null;
    evt = Array.clone(evt); // <- static list of delegates
    return function(source, args) {
    for (var i = 0, l = evt.length; i < l; i++) {
    evt[i](source, args);
    }
    }

  • Ron... yup... thats what it does. I think everyone is missing the point of my post entirely :)

  • I think it is correct - and expected - behavior, the so called "bug" lies in your code, not in the way delegates are handled. Once the event is raised the invocation list is walked and every handler in that list is executed.
    The thing you're probably missing is the fact that delegates are immutable. Meaning that when you remove a handler during the event, you will end up with a completely new invocation list (a new one; now there are two invocation lists). The walk over the original invocation list will still containg a reference to the "second" handler. The next time the event is raised the new invocation list will be walked.
    (Or did I miss your point too?)

  • Victor -- like I said earlier, imagine you were only developing Foo, in complete isolation. You have no idea how your component will be used within the application. Do you think there's a bug in your code?

  • Victor -- this example is clearly single threaded, and I was able to cause the error. So your 'no' case is incorrect.

    And this is the whole point of this post -- it seems to me its very unlikely for the developer of Foo to even imagine this happening. If they aren't thread safe, sure, but in the context of this blog, which is mostly ASP.NET, single threaded pages the vast majority of scenarios.

  • Oh and I forgot the simplest of Yes-es:
    Yes, it's a bug in your code: Because there are two public access points accessing _list: bar.SomeEvent & foo.Stop(). None of wich you have any absolute control over. So a reference check around _list is needed in Foo's case.

  • ha, same wordpress theme as on my blog :)
    p.s. nice articles...cheers

  • What you have pointed out did surprise me. But now I know about this so thanks!

    So in hindsight I think I would argue the problem _is_ with the Foo class developers code. His/her code is vulnerable to a null reference exception due to a perfectly legitimate usage of the Foo class.

    So the real question is how _should_ Foo be coded? In this case its pretty clear that the var _list should be checked for being null. But what about a more general case? Is there a pattern we can apply?

  • I think the right code would be like:

    public void RaiseEvent() {
    EventHandler temp = SomeEvent;
    if (temp != null) {
    temp(this, null);
    }
    }

  • I would argue that the problem lies with the author of the Main method. He doesn't understand the semantics of the Foo.Stop() method. He doesn't understand that calling Stop() renders Foo incapable of handling events raised by Bar, thus Stop() shouldn't be called in an event handler for a Bar event.

    As for whether the demonstrated behaviour is "correct" I would say no. If you don't understand the subtleties of the .NET event construct pointed out in the comments above, it is quite reasonable to expect the second Main implementation to cause only the anonymous delegate to be invoked.

    But hey, nobody ever said that the .NET event construct is a perfect implementation of what is meant by an "event" outside of computer science circles :-)

  • How can we avoid this in the general case? It looks like the heart of the problem is that Foo's implementation needs its functions called in a certain order but cannot guarantee that because all of them can be called from outside.

    So technically the writer of Main is violating Foo's contract by calling the functions out of order. However, we're more concerned about how to make Foo robust so that it handles misuse gracefully; doing so will prevent us from having to make bug fixes down the road.

    This is an interesting problem because hooking up an event handler causes it to become pseudo-public in that it can be called from outside the class. Wouldn't that make it susceptible to multithreading and order-related issues just like all other public functions?

    Maybe a good rule of thumb, then, is that in .NET, all public functions and event handlers should be written so that they can be called in any order. I think that would prevent this problem in general.

Comments have been disabled for this content.