Do some reasonable connection cleanup when returning a context with a DbConnection to the pool

[alexander-kucherov]

I'm migrating my application from .net 5.0 and EF Core 5.0.13 to .net 6.0 and EF Core 6.0.1
In my code I use raw ADO syntax (Database.GetDbConnection()) and TransactionScope for ambient transaction.
I update counter in transaction and throw Exception inside uncommited transaction.
I expect that my counter won't change.
But it happens.
When I use DbContext without pool - everything is OK.
Also context.Database.EnlistTransaction(Transaction.Current) will fix problem.
It also helps to downgrade to EF Core 5.0.13

My simplified application

DDL

CREATE TABLE TestTable(
    Id  BIGINT IDENTITY (1, 1) NOT NULL,    
    SomeValue INT NOT NULL DEFAULT(0),
    CONSTRAINT Test PRIMARY KEY CLUSTERED (ID ASC)
)
INSERT INTO TestTable(SomeValue) Values(0)
--SELECT * from TestTable

My project file

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>true</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <!--<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="5.0.13" />  will fix issue-->
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="6.0.1" />
  </ItemGroup>

</Project>

using System.Data;
using System.Transactions;
using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;
using IsolationLevel = System.Transactions.IsolationLevel;

var connectionString = "Data Source=localhost;Initial Catalog=MyDb;Integrated Security=True;";
var services = new ServiceCollection();
services.AddDbContextPool<MyDbContext>(builder => builder.UseSqlServer(connectionString));
//services.AddSqlServer<MyDbContext>(connectionString); // will fix issue
await using var sp = services.BuildServiceProvider();
for (var i = 0; i < 4; i++)
{
    await using var scope = sp.CreateAsyncScope();
    try
    {
        await UpdateAsync(scope.ServiceProvider);
    }
    catch (Exception e)
    {
        Console.WriteLine(e.Message);
    }
    await PrintValueAsync(sp);
}

static async Task UpdateAsync(IServiceProvider serviceProvider)
{
    var context = serviceProvider.GetRequiredService<MyDbContext>();
    using var ts = new TransactionScope(TransactionScopeOption.Required,
        new TransactionOptions()
        {
            IsolationLevel = IsolationLevel.ReadCommitted
        },
        TransactionScopeAsyncFlowOption.Enabled);
    var connection = context.Database.GetDbConnection();
    if (connection.State == ConnectionState.Closed)
    {
        await connection.OpenAsync();
    }
    //context.Database.EnlistTransaction(Transaction.Current); //will fix issue
    await using var command = connection.CreateCommand();
    command.CommandText = @"
MERGE dbo.TestTable AS target 
    USING (VALUES(@Id)) 
    AS source (Id)
ON (target.Id = source.Id)
WHEN MATCHED THEN
    UPDATE SET SomeValue = SomeValue + 1
OUTPUT inserted.SomeValue;
";
    command.Parameters.Add(new SqlParameter("Id", SqlDbType.BigInt)
    {
        Value = 1
    });
 
    var result = await command.ExecuteScalarAsync();
    Console.WriteLine($"result inside transaction {result}");
    throw new Exception("Some exception");
    ts.Complete();
}

static async Task PrintValueAsync(IServiceProvider serviceProvider)
{
    var context = serviceProvider.GetRequiredService<MyDbContext>();
    var connection = context.Database.GetDbConnection();
    if (connection.State == ConnectionState.Closed)
    {
        await connection.OpenAsync();
    }
    await using var command = connection.CreateCommand();
    command.CommandText = "SELECT SomeValue FROM TestTable WHERE Id=1";
    var result = await command.ExecuteScalarAsync();
    Console.WriteLine($"result without transaction {result}");
}

public sealed class MyDbContext : DbContext
{
    public MyDbContext(DbContextOptions options): base(options){}
}

my app output

result inside transaction 1
Some exception
result without transaction 0
result inside transaction 1
Some exception
result without transaction 1
result inside transaction 2
Some exception
result without transaction 2
result inside transaction 3
Some exception
result without transaction 3

SQL Profiler

but i expect the following output

result inside transaction 1
Some exception
result without transaction 0
result inside transaction 1
Some exception
result without transaction 0
result inside transaction 1
Some exception
result without transaction 0
result inside transaction 1
Some exception
result without transaction 0

Include provider and version information

EF Core version: 6.0.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: NET 6.0
Operating system: Windows 10 version 1909
IDE: Visual Studio 2022 17.0.5
dotnet --list-sdks
3.1.202 [C:\Program Files\dotnet\sdk]
5.0.403 [C:\Program Files\dotnet\sdk]
6.0.101 [C:\Program Files\dotnet\sdk]

[ajcvickers]

/cc @AndriySvyryd

[alexander-kucherov]

I've noticed that RelationConnection class hold transaction states and when I use raw SqlConnection RelationConnection doens't know anything about transaction and can't invoke ClearTransactions and HandleAmbientTransactions methods.
Opening connectiion by context.Database.OpenConnectionAsync() solves my problem.

[AndriySvyryd]

@alexander-kucherov The context keeps the same connection when using context pooling, so it remains open and enlisted in the first transaction. If you are opening the connection make sure to also close it when an exception is thrown.

[alexander-kucherov]

@AndriySvyryd , thanks for your answer.
Why does it work, when connection opened by Database.OpenConnectionAsync() and I dont't close it.
In previous version (5.0.13) this behavior was allowed.
It's breaking change.

[AndriySvyryd]

Why does it work, when connection opened by Database.OpenConnectionAsync() and I dont't close it.

This is tracked by the context, so when it's returned to the pool it closes the connection.

In previous version (5.0.13) this behavior was allowed.

By accident. The context instance wasn't returned to the pool in this case due to a bug

It's breaking change.

Strictly speaking - yes. If we get another report we'll add it to https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-6.0/breaking-changes

[ajcvickers]

Note from triage: In non-pooling scenarios when the DbConnection is owned by the context, it will be disposed when the context is disposed. When using a pool, the context could do some reasonable cleanup on the connection to account for cases where the connection state has been mutated externally to EF usages.

[roji]

Thinking about this a bit more... Note that whatever we do here is going to be partial. For example, users could modify the connection string on the DbConnection (e.g. to point at another database), and I don't think we should attempt to cover all cases.

It's probably OK to at least take care of the common case (close open connections), though there's also something to be said for establish a clear responsibility boundary: anything you do via EF Core APIs is reset, anything you do at the ADO.NET layer is your responsibility.

[matt-psaltis]

Just adding the additional report so this goes on the breaking changes list please!

[ferdikosasih]

give extra report so this goes to the breaking changes

[matt-psaltis]

@ajcvickers Will this be backported to a service release on .net 6?

[ajcvickers]

@matt-psaltis There are no plans to change this in a patch. Applications are expected to close a connection that they open. This change makes EF more robust when applications have bugs. But if you know your code is hitting this, then the appropriate thing to do is to make sure that the connection is closed.