Throwing exception in finalizer to enforce Dispose calls:

Question

Here is the typical IDisposable implementation that I believe is recommended:

~SomeClass() {
    Dispose(false);
}

public void Dispose() {
    GC.SuppressFinalize(this);
    Dispose(true);
}

protected virtual void Dispose(bool isDisposing) {
    if(isDisposing) {
        // Dispose managed resources that implement IDisposable.
    }
    // Release unmanaged resources.
}

Now, to my understanding, the idea behind the finalizer there is that if I don't call Dispose, my unmanaged resources will be released properly still. However, to my knowledge, it is generally agreed upon that not calling Dispose on an object that implements IDisposable is probably a bug.

Is there a particular reason not to fully embrace this and do this instead?

~SomeClass() {
    throw new NotImplementedException("Dispose should always be called on this object.");
}

public virtual void Dispose() {
    GC.SuppressFinalize(this);

    // Dispose managed resources that implement IDisposable.

    // Release unmanaged resources.
}

If you do that, you'll bring down the whole process with no way to handle the exception. Why not use a `Debug.Fail` statement instead? — Richard Deeming, Dec 03 '13 at 18:09
I do not want to handle the exception. Not calling Dispose should be a big broken thing that immediately is fixed. — Kelsie, Dec 03 '13 at 18:12
I'm tempted to close it as opinion based: I'd expect about 50/50 split on terminate on unexpected failure vs. be nice to users of your library even if they failed to use API appropriately. — Alexei Levenkov, Dec 03 '13 at 18:12
I'd really like to hear if there are any opinions I'm not aware of though? — Kelsie, Dec 03 '13 at 18:13
Consider Debug.Assert instead of an exception. Paired with some way to force GC it will give you relatively reliable way to trigger the issue while debugging and keep nice behavior in release builds. — Alexei Levenkov, Dec 03 '13 at 18:23

score 9 · Accepted Answer · answered Dec 03 '13 at 18:09

9

From .NET 2.0 and on, An unhandled Exception thrown in a Finalizer causes the process to terminate if the default policy is not overridden.

To my understanding, a Finalizer is not an expected location where an Exception should be thrown. I think it is possible for a Dispose() method not to have been called for an unexpected reason (Thread abort, ...) from which a clean recovery is still possible, provided that the Finalizer executes properly.

answered Dec 03 '13 at 18:09

Gabriel Boya

761
8
17

Will the debugger not halt on it at least? – Kelsie Dec 03 '13 at 18:11
1

Okay. And what if that's desirable? If there's a problem this would draw attention to it, rather than sweeping it under the rug. Seems appropriate enough. – Servy Dec 03 '13 at 18:11
2

I certainly don't want it to keep working and nobody realize it's broken. – Kelsie Dec 03 '13 at 18:11
@Kelsie, don't rely on a finalizer to tell you something is wrong, the finalizer is not guaranteed to be executed. – dcastro Dec 03 '13 at 18:14
A finalizer is certainly not guaranteed to be executed at any particular time, but are you saying that in the lifecycle of an application, it might just completely not do it at all? – Kelsie Dec 03 '13 at 18:14
If so, that would be a solid reason not to do this, of course. – Kelsie Dec 03 '13 at 18:16
@Kelsie - on other fatal exceptions when process terminates (or if it terminates from native layer or killed externally) finalizers will not be called... http://stackoverflow.com/questions/5200848/finalizer-not-called – Alexei Levenkov Dec 03 '13 at 18:16
http://stackoverflow.com/questions/9941688/arent-destructors-guaranteed-to-finish-running – dcastro Dec 03 '13 at 18:17
Right, of course. I wouldn't care in those situtions; I'm concerned with normal correct operation, for the debugger to signal that something is wrong. However, I just read the edit on the above answer, and that's a pretty convincing argument. – Kelsie Dec 03 '13 at 18:18
@Kelsie : btw, Code Analysis gathers a list of locations from which Exceptions are not expected to be thrown : http://msdn.microsoft.com/en-us/library/bb386039.aspx – Gabriel Boya Dec 03 '13 at 18:21

score 3 · Answer 2 · edited Jan 15 '16 at 16:38

Throwing an exception in a finalizer is almost always a bad idea.

Even though there may be situations where it would in fact achieve the desired result (causing the programmer to be notified of a problem without messing up anything else), a finalizer would have no way of knowing when that would be the case.

A better approach would be to have a method which will indicate (perhaps by throwing any exception) whether any objects have been wrongfully abandoned; one may call that method at the end of the program, or perhaps at times when new objects are created. This might cause an exception to get thrown in a part of the code which had nothing to do with the particular instance that was abandoned, but that would still likely be better than throwing the exception in a finalizer context.

score 2 · Answer 3 · answered Feb 25 '14 at 20:39

2

Throwing an exception from a finalizer is one of the worst things you could ever do. It can cause several different kinds of behavior. The exception could cause the ExecutionEngine to crash thereby bringing down your entire process, you could block the finalizer causing an OutOfMemory (OOM) crash, etc. Don't forget that the finalizer runs on a single thread and is one of the most important threads - you don't want it to get screwed up.

answered Feb 25 '14 at 20:39

Dave Black

7,305
2
52
41

What if this is what you actually want ? – Olivier Giniaux Jan 12 '21 at 12:17
@OlivierGiniaux Why would you want to do something that would cause undefined behavior in the runtime of the framework your application is running on? – Dave Black Jan 13 '21 at 16:39
Because you may prefer a crash with an explicit issue that you can easily fix over a memory leak that would silently cripple your app over months or years. – Olivier Giniaux Jan 14 '21 at 09:58
@OlivierGiniaux how are you going to diagnose, let alone, "easily fix" something that causes *undefined behavior* in the framework? It may crash, it may not crash, it may take 4 weeks before it crashes, it could corrupt memory in an unrelated part of the application, etc. The point is that you don't know how it's going to behave. – Dave Black Jan 19 '21 at 18:04

score 2 · Answer 4 · answered Oct 01 '15 at 08:28

2

Since Developer tend to become lazy and ignore whatever warning can be ignored and furthermore IIS7.5 applications completely ignore Debug.Fail by default I use the same pattern too. With one exception:

I do this only in debug build since it will not have any benefit to a release build. Furthermore it removes the slow finalizer from classes that do not hold unmanaged resources directly.

public void Dispose()
{
    // Do your dispose stuff here
#if DEBUG
    GC.SuppressFinalize(this);
}
~MyClass()
{   throw new ObjectNotDisposedException(); // This class is not intended to be used without Dispose.
#endif
}

There are cases where a missing dispose could really hog the application, e.g. a remote transaction context. Termination is nothing worse in this case and will get on lazy developers nerves. I always prefer to bring problems back to their source.

answered Oct 01 '15 at 08:28

Marcel Müller

21
1

1

Even if one wants to ensure that someone gets notified if objects are abandoned, I would think keeping track of what objects are outstanding and showing a warning if any remain when an application shuts down would be a cleaner way of going about it. – supercat Oct 01 '15 at 22:51
@supercat Would it be OK if that tracking _resurrected_ the object that was abandoned, as in `~MyClass() { StaticClass.GlobalListOfObjectsAbandoned.Add(this); Dispose(false); }`, in your opinion? – Jeppe Stig Nielsen Aug 16 '18 at 09:05
1

@JeppeStigNielsen: What would one be planning to do with the list? If one would be planning to serialize and log some information about the objects therein, I think it would be better to do collect the serialized forms. Note also that finalizers run in an unknown threading context, so one should use use some form of thread-safe collection like a `ConcurrentBag – supercat Aug 16 '18 at 20:00

Throwing exception in finalizer to enforce Dispose calls:

4 Answers4

Related