ProfiledDbConnection's methods after Dispose shouldn't all throw NullReferenceException

[duncansmart]

After I call MiniProfilerEF6.Initialize() then my automatic migrations fail. This is because when EF6 System.Data.Entity.Internal.RepositoryBase.CreateConnection (https://github.com/aspnet/EntityFramework6/blob/6.2.0/src/EntityFramework/Internal/RepositoryBase.cs#L35) is called, it checks the connections's State which, if the connection has been Disposed, throws a NullReferenceException because _connection gets null-ed on Dispose.

This isn't the right behaviour for a DbConnection. If you Close a SqlConnection for example, you can still call members such as State and they don't throw. I therefore propose the _connection is left as-is so that the wrapped methods still delegate to it without throwing NullReferenceExceptions.

[duncansmart]

Related: dotnet/ef6/issues/398 and #243.

[NickCraver]

Some behavior has shifted here - what version of EF Core are you on? I'm not against the change per-se, but need to think about it. cc @jdaigle

[duncansmart]

Not on EF Core, this is ye olde .NET Framework. Like I said, I think ProfiledDbConnection shouldn't change the behaviour of the DbConneciton it's wrapping.

[NickCraver]

Sorry, Core was my bad, I meant which version of EF 6?

[duncansmart]

6.2.0

[NickCraver]

Oh geez they still haven't updated NuGet? Poking that issue again. I don't agree with the fix here, since EF is the issue and it just happens to work in some cases (it's really depending on the GC not kicking in earlier in some others). Using a DbConnection (or anything really) after it's disposed is never valid. Checking the state after a .Close() or something: sure. But not a Dispose - that's violating the rules of .NET.

[duncansmart]

Surely ProfiledDbConnection's role is to invisibly wrap another connection "warts and all"? SqlConnection.State doesn't throw NRE after you've Disposed it. Whether it should or not is moot.

[NickCraver]

Its role is to also be responsible. If we don't release the connection when we're told to, that's a leak. Disposal is there for a reason and it very much matters at scale. For example, this change would likely take Stack Overflow offline under load. We can't make that change - it's an EF issue of using a disposed thing...and needs to be fixed there. I am unwilling to break everyone else for an EF 6.2 issue, that's a far worse option.

[duncansmart]

OK, I've created a workaround for EF 6.2 in the meantime:

/// <summary>
/// Call AddInterceptor from a DbConfiguration-derived class 
/// </summary>
class ProfiledDbConnectionDisposalInterceptor : System.Data.Entity.Infrastructure.Interception.IDbConnectionInterceptor
{
    public void Disposing(DbConnection connection, DbConnectionInterceptionContext interceptionContext)
    {
        if (connection is ProfiledDbConnection profiledConnection)
        {
            // don't dispose ProfiledDbConnection or checking its State, or calling any other method will throw NullReferenceException, which breaks migrations
            profiledConnection.WrappedConnection.Dispose();
            interceptionContext.SuppressExecution();
        }
    }

    // ... empty implementations for other IDbConnectionInterceptor members ...
}