How to handle exception thrown from Dispose?

Question

Recently, I was researching some tricky bugs about object not disposed.

I found some pattern in code. It is reported that some m_foo is not disposed, while it seems all instances of SomeClass has been disposed.

public class SomeClass: IDisposable
{
    void Dispose()
    {
       if (m_foo != null)
       {
          m_foo.Dispose();
       }
       if (m_bar != null)
       {
          m_bar.Dispose();
       }   
    }

    private Foo m_foo;

    private Bar m_bar;

}

I suspects that Foo.Dispose might throw a exception, so that following code is not executed so m_bar is not disposed.

Since Foo/Bar might be from third party, so it is not guaranteed to not throwing exception.

If just wrap all Dispose invocation with try-catch, the code will turn to be clumsy.

What's best practice to handle this?

Solution 1

Its true that it can be pretty bad to leak out an exception of your dispose method, especially since stuff that implements IDisposable will usually specify a finalizer that will call Dispose.

The problem is that sweeping the problem under of the carpet by handling an exception may leave you with some very hard-to-debug situations. What if your IDisposable allocated a critical section of sorts that only gets released after dispose. If you ignore the fact that the exception happened, you may end up in deadlock central. I think failures in Dispose should be one of those cases where you want to fail early, so you can fix the bug as soon as its discovered.

Of course it all depends on the object being disposed, for some objects you may be able to recover, others not. As a general rule of thumb, Dispose should not throw exceptions when used correctly and you should not have to code defensively around exceptions in nested Dispose methods you are calling.

Do you really do not want to sweep an OutOfMemoryException under the carpet?

If I had a dodgy 3rd party component that arbitrarily threw exceptions on Dispose I would get it fixed AND host it in a separate process that I could tear down when it started playing up.

Solution 2

If Dispose() is called inside of a finalization context and it throws an exception, your process will be terminated.

If you suspect that Foo.Dispose() is throwing an exception, I would dispose of it last if possible, and wrap it in try/catch. Do everything you can to get rid of it in the catch - set the reference to null. It is very bad to throw exceptions from Dispose() and should be avoided.

Unfortunately, if this is buggy third-party code, your best bet would be to get them to fix it. You shouldn't have to manually clean up after it.

Hope that helps.

Solution 3

According to Design Rules :

"A IDisposable.Dispose method should not throw an exception."

So if your program crashes because of unhandled exception from Dispose() - refer Official Solution

Solution 4

Because you don't have to allocate the variables in a using() statement - why not use 'stacked' using statements for this?

void Dispose()
{
    // the example in the question didn't use the full protected Dispose(bool) pattern
    // but most code should have if (!disposed) { if (disposing) { ...

    using (m_foo)
    using (m_bar)  
    {
        // no work, using statements will check null 
        // and call Dispose() on each object
    }

    m_bar = null;
    m_foo = null;
}

The 'stacked' using statements are expanded like this:

using (m_foo)
{
    using (m_bar) { /* do nothing but call Dispose */ }
}

So the Dispose() calls are put in seperate finally blocks:

try {
    try { // do nothing but call Dispose
    }
    finally { 
        if (m_bar != null)
            m_bar.Dispose(); 
    }
finally { 
    if (m_foo != null)
        m_foo.Dispose();
}

I had a hard time finding a reference for this in one place. The 'stacked' using statements are found in an old Joe Duffy blog post (see section 'C# and VB Using Statement, C++ Stack Semantics'). The Joe Duffy post is referenced by many StackOverflow answers on IDisposable. I also found a recent question where stacked using statements for local variables appear to be common. I couldn't find the chaining of finally blocks anywhere but the C# language spec (section 8.13 in C# 3.0 spec), and only for multiple varaibles within a single 'using' block, which isn't exactly what I'm proposing, but if you disasemble the IL you'll find the try/finally blocks are nested. On the null check, also from the C# spec: 'If a null resource is acquired, then no call to Dispose is made, and no exception is thrown.'

Solution 5

Dispose is not supposed to throw any exceptions. If it does—it’s not well written, so…

try { some.Dispose(); } catch {}

should be enough.

• http://blogs.msdn.com/b/clyon/archive/2004/09/23/233464.aspx

View more solutions

Comments

[Sam Saffron]

@womp, its lose lose, if you hide/handle an arbitrary exception that is thrown from dispose you may end up leaking memory or handles or os locks. All of which can wreak havoc with your process.

[nuiun]

Very true. I was assuming that he wanted to keep the process going at all cost, but I probably should have stated that. His best bet would definitely be to throw away the control or get the third party to fix it. Your comments about "hard to debug" is an excellent point.

[supercat]

It's true that one should when possible design the semantics of Dispose so that it won't fail, but the real world isn't always so nice. Disposing a file, for example, is supposed to ensure that any pending data actually gets written. If the data doesn't get written, that's a problem, and it needs to get communicated somehow. I would suggest that exceptions shouldn't throw unless something is really really wrong, but if something is really really wrong, it shouldn't be ignored.

[LukeSw]

I see your point. Nevertheless, Dispose is not supposed to throw any exceptions. Pending data can be forced to write in a Flush method or similar. What I want to say is that, you always have possibility to write code in way that ensures that Dispose won’t throw anything. But if you have to deal with external code which throws an exception in Dispose, I would wrap it in other method, like TryDispose, and rewrite Dispose in a proper way.

[supercat]

@LukeSw: If code makes assumptions about the state of the system following a Dispose, and those assumptions are not met, Dispose should throw an exception. Unfortunately, there's no good way of knowing what assumptions outside code is going to be making, nor is there any good way of knowing what exception (if any) is pending when a disposal failure happens. If some code expects that a database transaction will either succeed or leave the database in a known-unaltered state, and for whatever reason an exception occurs but the database can't do a rollback and confirm that it worked...

[supercat]

@LukeSw: ...the proper course of action would be to throw an exception which is different from the "original" exception, but which contains the original exception as an InnerException. Code may be prepared to deal with the original exception and continue processing, but that doesn't mean it's prepared to deal with a database that's in an unknown state. What's really needed is a static PendingException method which could be used to supply the InnerException in the DatabaseRollbackException. Unfortunately, there's no clean way to get it.

[LukeSw]

@supercat: I agree. There should be an exception thrown informing about disposal failure and application unknown state (which could be potentially dangerous) but only for critical exceptions. See here: stackoverflow.com/questions/577607/…

[supercat]

@LukeSW: I wonder if MS will ever add something like a PendingException method? It would make things a lot cleaner.