Adding one event handler to another

Question

I have a class which wraps another class and exposes several events from the class it's wrapping. (The instance it wraps can change)

I used the following code:

public event EventHandler AnEvent;

public OtherClass Inner {
    get { /* ... */ }
    set {
        //...
        if(value != null)
            value.AnEvent += AnEvent;
        //...
    }
}

However, the events were raised inconsistently.

What's wrong with this code?

Answer 1

The problem is that Delegates are immutable.

If you add a handler to an event, it creates a new Delegate instance which contains the old handlers and the newly added handler. The old Delegate is not modified and is discarded.

When I write, value.AnEvent += AnEvent, it adds the Delegate containing the current handlers (if any) to the inner class's event. However, changes to the outer class's event are ignored because they don't change the Delegate instance that I added to the inner classes event. Similarly, if I remove a handler after setting the Inner property, the handler isn't removed from the inner class's event.

---

There are two correct ways to do this.

I can make my own handler that invokes the wrapper's event, like this:

public event EventHandler AnEvent;

public OtherClass Inner {
    get { /* ... */ }
    set {
        if(Inner != null)
            Inner.AnEvent -= Inner_AnEvent;

        //...

        if(value != null)
            value.AnEvent += Inner_AnEvent;

        //...
    }
}

void Inner_AnEvent(object sender, EventArgs e) { 
    var handler = AnEvent;
    if (handler != null) handler(sender, e);
}

---

The other way is to make a custom event in the wrapper that adds its handlers to the inner class's event, like this:

EventHandler anEventDelegates

public OtherClass Inner {
    get { /* ... */ }
    set {
        //...
        if(value != null)
            value.AnEvent += anEventDelegates;
        //...
    }
}
public event EventHandler AnEvent {
    add {
        anEventDelegates += value;
        if (Inner != null) Inner.AnEvent += value;
    }
    remove {
        anEventDelegates -= value;
        if(Inner != null) Inner -= value;
    }
}

Note that this is not entirely thread-safe.

I solved this problem myself and am posting the question & answer for the benefit of people with similar problems.

Answer 2

The your answer - there are two problems here...

First: in both cases, you are raising the outer event with the wrong sender. Someone subscribing to an event on the outer class would expect those classes to be raised with a sender of that outer class.

This is particularly important in things like winform Controls, or binding-list implementations, where the sender is used to identify the object between many that share a handler.

This should instead be something like:

void Inner_AnEvent(object sender, EventArgs e) { 
    var handler = AnEvent;
    if (handler != null) handler(this, e);
}

The second (much more minor) issue is that you are currently taking out an event on the inner class even if the outer class has no subscribers. You can fix this with a bit more custom handling...

private EventHandler anEvent;
public event EventHandler AnEvent {
    add { // note: not synchronized
        bool first = anEvent == null;
        anEvent += value;
        if(first && anEvent != null && inner != null) {
            inner.SomeEvent += Inner_AnEvent;
        }
    }
    remove { // note: not synchronized
        bool hadValue = anEvent != null;
        anEvent -= value;
        if(hadValue && anEvent == null && inner != null) {
            inner.SomeEvent -= Inner_AnEvent;
        }
    }
}

(and similar code in the Inner get/set to only subscribe if we have listeners...

if(value != null && anEvent != null)
    value.AnEvent += Inner_AnEvent;

This might be a big saver if you have lots of instances of the outer-class, but rarely use the event.