Should a disposed control be able to safely ignore event callbacks?

Question

I have a class which is a disposable UI Control. It subscribes to changes of a moldel object to redraw it's content.

On the other hand, under circumstances, some special change of the same model object instructs the view containing this control to remove and dispose it (the control).

As a result a change in a model, depending on subscription order - first causes the control disposal and afterwards it's method invocation - which ends up with ObjectDisposedException.

Question: Should the control be designed to safely ignore event callbacks or should we try to prevent this kind of invocation from other layers?

For those who prefer to see more code then words I have prepared a very simplified example:

//############################################
class View
{
    private Control m_Control;

    public View(Logic logic, Model model)
    {
        m_Control = new Control(model);
        logic.Changed += LogicChanged;
    }

    private void LogicChanged(object sender, EventArgs e)
    {
        m_Control.Dispose();
        m_Control = null;
    }
}

//############################################
class Control : IDisposable
{
    private readonly Model m_Model;

    public Control(Model model)
    {
        m_Model = model;
        m_Model.Changed += ModelOnChanged;
    }

    public bool IsDisposed { get; private set; }

    public void Dispose()
    {
        m_Model.Changed -= ModelOnChanged;
        IsDisposed = true;
    }

    private void ModelOnChanged(object sender, EventArgs e)
    {
        if (IsDisposed)
        {
            throw new ObjectDisposedException(ToString());
        }
        //Do something
    }
}

//############################################
class Model
{
    public event EventHandler<EventArgs> Changed;

    private void OnChanged(EventArgs e)
    {
        EventHandler<EventArgs> handler = Changed;
        if (handler != null)
            handler(this, e);
    }

    public void Change()
    {
        OnChanged(null);
    }
}

//############################################
class Logic
{
    private readonly Model m_Model;

    public Logic(Model model)
    {
        m_Model = model;
        m_Model.Changed += ModelOnChanged;
    }

    private void ModelOnChanged(object sender, EventArgs e)
    {
        OnChanged(null);
    }

    public event EventHandler<EventArgs> Changed;

    private void OnChanged(EventArgs e)
    {
        EventHandler<EventArgs> handler = Changed;
        if (handler != null)
            handler(this, e);
    }
}

//############################################
class Program
{
    private static void Main(string[] args)
    {
        var model = new Model();
        var logic = new Logic(model);

        var view = new View(logic, model);
        model.Change();
        //And crash!
    }
}

Where would you propose a fix in given example? Model and Logic classes are just doing their business without knowing about the order of event subscription. I see also no design flaw in View and Control implementations.

Imagine there are three different teams implementing Model, Logic and UI and there are not just these four components, but hundreds of them. The issue can occur everywhere.

What I am looking for is not a local fix in this particular case, but I want find a pattern to prevent that. For instance: "Controls must gracefully ignore event calls on disposed instances" or "Logic must prevent subscriptions on model, only UI is allowed to do so." etc.

---

In addition to accpeted answer

Yes, disposed object event callback should not throw an exception. Even more generally :

... event handlers are required to be robust in the face of being called even after the event has been unsubscribed.

There are number of reasons for that - see Eric Lippert’s wonderful article Events and Races

Answer 1

One pattern I've seen is the following. If you are disposed, just do nothing rather than throw an exception.

private void ModelOnChanged(object sender, EventArgs e)
{
    if (IsDisposed) { return; } // i.e. Do nothing

    //Do something
}

One of the biggest problems with the IDisposable pattern is that it's trying to be deterministic and non-deterministic memory management at the same time. You can call Dispose(), or GC can do it for you. It creates all the mess with Finalizers and such.

Unlike languages that simply keep a reference counter - calling the 'destructor' when the last reference goes away - .NET chooses an approach where an object's memory may have been released but references still exist to that object. So you have to make sure that your code don't access an object in an invalid state. This usually takes one of two forms:

1. Check IsDisposed on everything Public, throwing an ObjectDisposedException if it is disposed
2. Implicitly do nothing if the object is disposed (early return)

The first option is less likely to bite you in the long run since you know that you've made a mistake right away. However, if the behavior isn't predictable and you have temporal coupling issues, you'll end up having to handle the ObjectDisposedException all over your program. In which case, you might go for the 'do-nothing' approach so you have less fluff all over your program. Unfortunately it has potential to bite you since it looks like the method you called did it's job.

Another option I hadn't considered until now is to subscribe to the Disposed event objects that you have class level references to that are IDisposable. When the object is disposed, set the field to null. Similarly, you could check IsDisposed (if it is exposed) before doing something with an object (ask before you jump approach).

Public Class Foo
  Private _disposableObject As IDisposableFoo

  Private Sub OnBarDisposed(sender As Object, e As EventArgs) Handles IDisposableFoo
    _disposableObject = Nothing 
    'Hmm, now we'll get null-references everywhere
  End Sub

And...

Public Sub DoesStuffWithIDisposableObject()
  If Me.DisposableObjectReference.IsDisposed Then Exit Sub

  'Yay, valid reference! Let's get stuff done!
End Sub

Still probably not the best option, but unfortunately the design of the language makes such sort of cruft inevitable.

Answer 2

In deciding whether to throw an ObjectDisposedException, one should consider a couple of factors:

1. Can the object satisfy the contract for the particular method call, without having to make use resources which are no longer available?
2. If the function returns successfully, will the caller likely expect to do something which can't be done without the necessary resource, meaning that since failure is inevitable anyhow one should throw early rather than late, or will the caller likely do the right thing even if no exception is thrown?

In many cases, especially in "update-event" scenarios, the caller doesn't particularly care what the called method does; the semantics of the call are basically "Do whatever you think needs doing". Such an operation can successfully be performed by any object even if it is disposed, simply by deciding that nothing needs doing. If the callback pattern one is using doesn't allow the event handler to notify the event publisher that it does not wish to receive further events (Microsoft's normal event pattern doesn't provide any such facility) there's a possibility that a Disposed object might be continuing to receive callbacks because the event publisher was left in an erroneous state, but throwing an exception probably wouldn't do much to help that problem and would likely create more.

Answer 3

I think I'd throw an ObjectDisposedException if something tries to do something with a disposed control. Diposing the control means you're done with it, so nothing should be trying to use it anymore. If something is, I would take that as an error in the program that needs to be fixed.

Answer 4

In a guideline about diposed objects i read:

There are a few rules about disposed objects. First, calling Dispose on an already disposed object should do nothing. Second, calling any other property or method on an already disposed object should throw an ObjectDisposedException.

Another guideline:

Once an object has been disposed, it should be considered off-limits. By convention, a disposable object should throw an exception if any of its methods are called after Dispose is called. There is a built-in exception type called ObjectDisposedException in the System namespace that was added to the Framework Class Library for this exact purpose.

Following these rules your code is completely ok (although it throws an exception).

Both quotes say that a disposed object should throw an exception if any of its methods or properties are called. I would prefer to say "throw an exception if any of its public methods or properties are called".

If a private method is called I think it is safe to do nothing. So the answer to your question is: "Yes, a disposed control should be able to safely ignore event callbacks".

BTW: Maybe the question could also be: "Who is responsible for disposing an object?"

In the given example you can let the control dispose itself in the ModelOnChanged method. I didn't find a general guideline for that but a few suggestions.