Efcore: Implement Basic Dispose Pattern

[zlstringham]

The current implementation of IDisposable objects do not follow the Basic Dispose Pattern. This is particularly important because classes (such as DbContext) are _not_ sealed, and may be inherited from. If derived classes also need to implement Dispose() logic, the standard paradigm is for them to override the protected virtual void Dispose(bool) method. Here are some issues with the current implementation, from the article:

DO declare a protected virtual void Dispose(bool disposing) method to centralize all logic related to releasing unmanaged resources.

There is no such method in the current implementations.

DO NOT make the parameterless Dispose method virtual.

The current Dispose method is virtual.

It is worth noting that EF 6 _does_ follow the pattern.

[ajcvickers]

@zlstringham Can you describe what you are attempting to do that is not working? Or even what you might hypothetically want to do that won't work?

[smitpatel]

Given that DbContext is not Finalizable type, not sure if we need to strictly adhere to Basic Dispose pattern.

[zlstringham]

@ajcvickers
There isn't something that doesn't work; it's a matter of adhering to the standard pattern. DbContext works.

Suppose I want to derive a class from DbContext, and this new class also needs to perform Dispose functionality. In implementing this class, per the MSDN-established standard for implementing IDisposable, I would override the protected virtual void Dispose(bool) method---which doesn't exist.

Suppose I also want, in this derived class, specific functionality to trigger based on whether it was invoked by calling Dispose() or a finalizer. I lose that flexibility. The only alternative is to implement the basic dispose pattern in the derived class:

public override void Dispose()
{
    Dispose(true);
    base.Dispose();
}

Which should not be necessary as this is the kind of design that belongs in the base class.

@smitpatel
It again helps to think in terms of _derived_ classes, not just the base class, when considering this pattern.

[ajcvickers]

@zlstringham We have discussed this at length in the past and we made a conscious decision not to follow this pattern. We would need to have a pretty strong argument that something is broken in order to change that decision. I'm going to close this for now, but if there is something that is really broken, then please feel free to re-open with details.

[tldzyx]

@ajcvickers @smitpatel
Could you provide more detail about the reasons for not implement the basic dispose pattern?

I'm confused. if DbContext don't need implement the basic dispose pattern, why it implement the IDisposable interface?

Usually, if a object be called the Dispose method, we should use it again never, and a IsDisposed boolean is pretty.

[bricelam]

@tldzyx Wouldn't calling obj.IsDisposed be using the object? :trollface:

[bricelam]

The Visual Basic snipped provides the best guidance for IDisposable:

#Region "IDisposable Support"
    Private disposedValue As Boolean ' To detect redundant calls

    ' IDisposable
    Protected Overridable Sub Dispose(disposing As Boolean)
        If Not disposedValue Then
            If disposing Then
                ' TODO: dispose managed state (managed objects).
            End If

            ' TODO: free unmanaged resources (unmanaged objects) and override Finalize() below.
            ' TODO: set large fields to null.
        End If
        disposedValue = True
    End Sub

    ' TODO: override Finalize() only if Dispose(disposing As Boolean) above has code to free unmanaged resources.
    'Protected Overrides Sub Finalize()
    '    ' Do not change this code.  Put cleanup code in Dispose(disposing As Boolean) above.
    '    Dispose(False)
    '    MyBase.Finalize()
    'End Sub

    ' This code added by Visual Basic to correctly implement the disposable pattern.
    Public Sub Dispose() Implements IDisposable.Dispose
        ' Do not change this code.  Put cleanup code in Dispose(disposing As Boolean) above.
        Dispose(True)
        ' TODO: uncomment the following line if Finalize() is overridden above.
        ' GC.SuppressFinalize(Me)
    End Sub
#End Region

If you never anticipate having unmanaged resources, the Boolean overload of Dispose provides no value.

[cfauchere]

I agree the recommended IDisposable pattern should be followed. Why DbContext should be special? What is the message sent by Microsoft here? We recommend a pattern but it does not work for our scenario. Either the pattern is broken or your implementation of DbContext is but regardless one of them should be fixed.
@ajcvickers is stating that it was a conscious decision without explaining why.

[ajcvickers]

@cfauchere The guidance is wrong.

[qsdfplkj]

IDisposable quote "Provides a mechanism for releasing unmanaged resources." I assume there aren't any unmanaged resources and this call is only used to proxy the call to the serviceScope. When you look at the pattern there are two differences. Dispose doesn't call GC.SuppressFinalize(this) so the dbcontext remains on the finalizerqueue but the finalizer doesn't do anything so this seems fine. The dispose method itself is virtual which is also different from the pattern. Therefore the dispose can be overriden to implement the full idisposable pattern on derived classes. All in all it just seems a minimal setup for conforming to the rule that a class (dbcontext) that has idisposable members (a servicescope) should implement idisposable itself.

[sharwell]

The full IDisposable guidance adds a bunch of complexity for the sole purpose of supporting user-defined finalizers. Based on my experience in this space, users are _overwhelmingly_ more likely to misuse a finalizer than to use it correctly, so taking a stronger position of "do not create a user-defined finalizer" is more likely to result in better code. If no one is creating finalizers, the full IDisposable pattern is unnecessary and you can use the form seen here instead.

Also note that the full IDisposable pattern produces a big test gap in normal code: with no finalizers in place to test the Dispose(false) case, it is common for developers to incorrectly account for this argument in Dispose implementations. Eliminating the parameter necessarily eliminates this mistake.

In the end, the simple form is both simpler and more likely to result in a correct application.

[qsdfplkj]

@sharewell you still need it for releasing unmanaged resources. Better teach them in stead of prohobit.

[sharwell]

you still need it for releasing unmanaged resources

This is not correct¹. Unmanaged resources should be encapsulated in an object derived from SafeHandle, at which point they are treated as a managed resource for the purpose of implementing IDisposable.

¹ It may be possible that some code needs to treat a resource as unmanaged. However, I've been looking for such code for over 6 years and have not found it yet.

[qsdfplkj]

SafeHandle itself implements IDisposable. I don't think you can avoid it entirely because you are still left with this "cascade dispose rule": https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#cascade-dispose-calls