Exception thrown from Dispose or Finalize

ID

vbnet.correctness.exception_in_dispose_finalize

Severity

critical

Remediation Complexity

medium

Remediation Risk

medium

Remediation Effort

low

Resource

Error Handling

Language

VB.NET

Description

Reports a Throw New …​ statement or a bare Throw anywhere in the body of a Sub Dispose, including the Dispose(disposing As Boolean) overload of the dispose pattern, or of Sub Finalize. Rethrowing from a Catch clause inside one of those methods is reported too, since it propagates out of the cleanup method just the same. Throwing from an ordinary method of the same type, such as an explicit Flush or a check on a disposed object, is not reported, and neither is catching a cleanup failure inside Dispose and logging it.

Rationale

Cleanup code runs at the points where the caller is least able to react. A Using block calls Dispose on the way out, including the way out taken by an exception, and an exception thrown from Dispose at that moment replaces the exception that was already travelling - so a defensive check in Dispose converts a diagnosable failure into a misleading one, reporting that writes were unflushed while hiding the error that stopped them from being flushed. Finalize is worse: it runs on the finalizer thread, long after the code that created the object has moved on, with no caller and no handler in scope. An exception that escapes it is unhandled, brings the process down and takes every other object still awaiting finalization with it - and it does so at a point where the object’s fields cannot be trusted, because the objects it referenced may already have been collected.

The following code illustrates the pattern detected by this rule:

Public Sub Dispose() Implements IDisposable.Dispose
    If _pendingWrites > 0 Then
        ' FLAGGED: Exception thrown from Dispose or Finalize
        Throw New InvalidOperationException("unflushed writes remain")
    End If
    _stream.Dispose()
End Sub

Remediation

Make cleanup unable to fail. Check the state you need instead of asserting it - If _stream IsNot Nothing Then _stream.Dispose() - and wrap any release that can genuinely throw in a Try …​ Catch inside the method, logging the failure rather than propagating it. When callers must be told that releasing the resource failed, give them an explicit Close or Flush method that is allowed to throw and call it before disposal, leaving Dispose as the last-resort path that always succeeds. In Finalize, release unmanaged handles only, never touch other managed objects, and keep the whole body inside a Try …​ Catch; a finalizer that detects an object was never disposed should record that fact, not throw about it.

Configuration

This detector does not need any configuration.

References