Objects need to be disposed in a specific order: is this a code smell?

Question

The context

I'm working with a Winforms application (.NET 3.5 SP1) which has the concept of workspaces, which can contain n number of panels. Each panel (derives from Panel) has view:

    .-----------------------.
    |Workspace              |
    |.--------.  .--------. |
    ||Panel1  |  |Panel2  | |
    ||.-----. |  |.-----. | |
    |||View1| |  ||View2| | |
    ||'-----' |  |'-----' | |
    |'--------'  '--------' |
    '-----------------------'

All panels get added to the this.Controls collection of the Workspace class (which derives from an UltraTabPageControl, an Infragistics GUI control). Each view is added to the Controls collection of their parent. So, when Dispose is called on the Workspace, the panels and views automatically get disposed, which is completely expected and desired.

We have another concept called a ViewManager. It keeps track of all the View controls in the workspace, and is in charge of maintaining a single "master" view. Whenever a View is created, it registers itself with this manager. This adds the View to a list, and then runs some logic to determine the new "master" view and then calls a Synchronize() method on each View.

The current design is that, whenever View.Dispose() is called, it unregisters itself from the ViewManager. This removes the View from the list, and then runs corresponding logic that checks for a new master, out of the remaining views.

The twist

When we are closing an entire workspace, there is one special Panel type that needs to be closed before other panels. So we have code in our Dispose method that looks like this:

protected override void Dispose(bool disposing)
{
    var theSpecialPanel = GetSpecialPanel();
    if (theSpecialPanel != null)
    {
        theSpecialPanel.Dispose();
    }
    base.Dispose(disposing);
}

If we take out that code, then other panels may be disposed before theSpecialPanel. This causes the logic that checks for a new master panel to be run, calls Synchronize() on each View including this special panel. This throws an

"InvalidComObjectException: COM object that has been separated from its underlying RCW cannot be used."

The question

Is this design is a code smell? Is it strange to enforce that a specific object gets disposed before others?

It sure looks bad. What happens if the client code doesn't call Dispose? Does that crash the finalizer thread with this exception? That's an undebuggable exception. Read this: http://blogs.msdn.com/b/visualstudio/archive/2010/03/01/marshal-releasecomobject-considered-dangerous.aspx?wa=wsignin1.0 — Hans Passant, Mar 12 '11 at 01:33
That was going to be my next question:) - what would cause this exception? Thanks for the pointer; exactly what I was looking for. — bentsai, Mar 12 '11 at 03:34
Note: your Dispose code does not follow good practice - you are accessing other managed objects if disposing==false. Could be confusing if you have your own non-standard implementation of destructors or plain wrong if finalizer calls Dispose(false). See http://msdn.microsoft.com/en-us/library/aa720161(v=VS.71).aspx — Alexei Levenkov, Mar 12 '11 at 04:02
@AlexeiLevenkov I've intentionally not followed Microsoft's oft-quoted Dispose pattern. We will not be implementing a finalizer, nor will derived classes do so, so we can [forgo much of that cruft](http://bentsai.wordpress.com/2011/02/21/idisposable-the-contentious-dispose-pattern/). Thanks for the heads up. — bentsai, Mar 14 '11 at 15:00

score 2 · Answer 1 · answered Mar 11 '11 at 23:32

It is entirely reasonable to require that certain objects be disposed in a particular sequence. Note that a major limitation of finalizers is that they cannot guarantee anything about disposal order. Consider a Froboz9000Connection object (used for managing a connection to a Froboz 9000 computer) which in turns holds a SerialPort that's used for actual communication. It's necessary that before the program ends, it must send a certain command sequence to the remote machine. The proper sequence of events is for the Frobozz9000Connection object's Dispose method will send the necessary command sequence and then Dispose the SerialPort. If the SerialPort gets disposed first, the Frobozz9000Connection object won't be able to send the proper command sequence to notify the remote machine that its services are no longer required.

This issue, btw, is yet another (one of many) reasons I dislike finalizers. While there are times finalizers can certainly be useful, I think it's far more important in the vast majority of cases to simply make sure Dispose is used properly.

score 2 · Answer 2 · answered Mar 11 '11 at 23:57

Does the SpecialPane require explicit disposal? Or do you just want to ensure that the call to Synchronize() is always correct? I'm assuming here that your View derives from Control.

public void Synchronize()
{
    if (this.IsDisposed || this.Disposing) return; // or return a 'remove me' flag
    ... 
    // sync
}

Without knowing the rest of your code, I'm assuming that this should work relatively well, without relying on Finalizers. You make each View responsible for knowing whether or not they can perform the particular action without requiring the form container to know about implementation details of your system.

I believe it will also help maintenance. Relying on order of disposal can be tricky, and if another developer were to come along, they may miss that altogether. It will also work if the View is disposed by other means, rather than being called from the Form disposal method.

Oh, and if View doesn't extend Control, then give View a reference to it's parent, and check Disposal state on the parent control.

I think your advice is good in general. I've tried adding this check in the View, but in my case, it does not work. I have an `AxWindowsMediaPlayer` object which is getting finalized before Dispose happens, resulting in the `InvalidComObjectException`. At the point of the exception, `IsDisposed` and `Disposing` are both false. I'm trying to determine why this is happening. — bentsai, Mar 14 '11 at 17:05

score 1 · Accepted Answer · answered Mar 11 '11 at 23:28

1

In your code snippet, I didn't see where you're disposing of the other panels.

If you dispose of all panels, I don't see any problem with deciding on any order of disposal that you wish. If I've followed you correctly, you can do:

foreach (panel in Panels.Where(p => p != theSpecialPanel))
{
   panel.Dispose();
}
theSpecialPanel.Dispose();

answered Mar 11 '11 at 23:28

Ilya Kogan

21,995
15
85
141

The other panels get disposed by virtue of being in the `Controls` collection of the workspace. I'm not explicitly disposing the other panels. Does that change things? [You have the order wrong (I want to dispose theSpecialPanel first), but you have the right idea in your example.] – bentsai Mar 11 '11 at 23:32
I see. So the loop comes after `theSpecialPanel.Dispose();` and it is implicit inside the `base.Dispose(disposing);` call. It still seems all right to me, the semantics are similar. You can require that a view would be disposed before the panel, so why can't you require that one panel would be disposed before another? – Ilya Kogan Mar 11 '11 at 23:34

Objects need to be disposed in a specific order: is this a code smell?

3 Answers3

Linked