Add creation and disposal to connection interception #27920
Conversation
src/EFCore.Relational/Diagnostics/ConnectionCreatedEventData.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Diagnostics/IRelationalConnectionDiagnosticsLogger.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Diagnostics/IRelationalConnectionDiagnosticsLogger.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Diagnostics/IRelationalConnectionDiagnosticsLogger.cs
Outdated
Show resolved
Hide resolved
| <comment>Debug RelationalEventId.ConnectionCreated int</comment> | ||
| </data> | ||
| <data name="LogConnectionCreating" xml:space="preserve"> | ||
| <value>Creating DbConnection.</value> |
There was a problem hiding this comment.
Should we add '{database}' and '{server}' to these too?
There was a problem hiding this comment.
We could, but sometimes we won't know because we don't have a connection string. We could have two different messages. Or we could say it's okay to not include it here and look at the open events for this when it is always known and correct.
test/EFCore.SqlServer.FunctionalTests/ConnectionInterceptionSqlServerTest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Andriy Svyryd <[email protected]>
Co-authored-by: Andriy Svyryd <[email protected]>
Co-authored-by: Andriy Svyryd <[email protected]>
Co-authored-by: Andriy Svyryd <[email protected]>
Co-authored-by: Andriy Svyryd <[email protected]>
…ticsLogger.cs Co-authored-by: Andriy Svyryd <[email protected]>
Co-authored-by: Andriy Svyryd <[email protected]>
…ticsLogger.cs Co-authored-by: Andriy Svyryd <[email protected]>
…ticsLogger.cs Co-authored-by: Andriy Svyryd <[email protected]>
ajcvickers
force-pushed
the
TheRainbowConnection0426
branch
from
61a48eb
to
6e67b4a
Compare
| Assert.False(interceptor.DisposedCalled); | ||
|
|
||
| var connection = context.Database.GetDbConnection(); | ||
| connection.Disposed += (_, __) => connectionDisposed = true; |
There was a problem hiding this comment.
@ajcvickers FYI in Npgsql we don't fire this event, for better perf (and because it's mostly an artifact of DbConnection extending Component, and is ridiculous!). Nobody's ever complained until now 😉
Filed npgsql/efcore.pg#2368 to see what to do (someday:tm:)
There was a problem hiding this comment.
FYI, this also broke the Pomelo/MySqlConnector tests: PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1718
Is depending on Component.Disposed really the right choice here?
There was a problem hiding this comment.
Arguably, this is a regression in providers that override DisposeAsync; see https://mysql-net.github.io/AdoNetResults/#Dispose_raises_Disposed.
Because of https://github.com/dotnet/runtime/blob/v6.0.10/src/libraries/System.Data.Common/src/System/Data/Common/DbConnection.cs#L105-L109, the base DbConnection implementation delegates to Component.Dispose, which calls Component.Dispose(true), which raises the event. Once DisposeAsync (which is introduced at DbConnection) is overridden, this default behaviour no longer occurs.
If Component followed the DisposeAsync pattern (which I'm sure it can't, due to significant ripple effects throughout the ecosystem), then it could take care of raising the event in both sync and async code paths. Since it doesn't... maybe providers should call down to base.Dispose(true) in their DisposeAsync implementation to run the (sync parts of the) base class's Dispose?
There was a problem hiding this comment.
@roji @bgrainger @lauxjpn Filed #29473.
Fixes #23087.
Part of #626.