Connection Pooling results in unpredictable isolation levels

[alexdawes]

Description

A connections default isolation level is "read commited". However, once a transaction has been committed or rolled back, the isolation level remains as it was set by the transaction, until the connection is closed or disposed. (see note under https://docs.microsoft.com/en-us/dotnet/api/microsoft.data.sqlclient.sqlconnection.begintransaction?view=sqlclient-dotnet-core-2.1)

However because SqlClient reuses connections in a pool, connections are not actually closed between uses of different SqlConnection instances. Because of this, after using a SqlConnection and setting the isolation level via a transaction, another SqlConnection created elsewhere can inherit this isolation level.

This can cause unexpected increases in isolation level (especially when using Serializable and Repeatable Read) that can have knock on consequences such as deadlocks. In the other direction, you can get dirty reads if a connection inherits a "read uncommitted" isolation level from a previous use of the connection.

This strikes me as potentially very dangerous behaviour, as unless you explicitly set the isolation level for every use of the connection, you can get essentially a random level.

To reproduce

Code tested with Microsoft.Data.SqlClient 2.1.3 (and System.Data.SqlClient 4.4.0), and uses Dapper (2.0.90) to execute the SQL (though Dapper is not related to the problem).

using System;
using System.Data;
using System.Linq;
using System.Threading.Tasks;
using Dapper;
using Microsoft.Data.SqlClient;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            MainAsync(args).GetAwaiter().GetResult();
        }

        static async Task<string> GetIsolationLevel(IDbConnection connection, IDbTransaction transaction = null)
        {
            string result = null;
            await connection.QueryAsync<string, string, string>("DBCC USEROPTIONS",(option, value) =>
            {
                if (option == "isolation level")
                {
                    result = value;
                }
                return option;
            }, transaction: transaction, splitOn: "Value");
            return result;
        }
        
        static async Task MainAsync(string[] args)
        {
            var connStr = "Server=[redacted];Database=[redacted];User Id=[redacted];Password=[redacted];";

            Console.WriteLine("Using no transaction");

            (await Task.WhenAll(Enumerable.Range(1, 10).Select(async i =>
            {
                await using var conn = new SqlConnection(connStr);
                await conn.OpenAsync();
                return (Test: i, IsolationLevel: await GetIsolationLevel(conn));
            }))).ToList().ForEach((t) => Console.WriteLine($"Test {t.Test}: {t.IsolationLevel}"));

            Console.WriteLine();
            Console.WriteLine("Using serializable iso level transaction");

            (await Task.WhenAll(Enumerable.Range(11, 10).Select(async i =>
            {
                await using var conn = new SqlConnection(connStr);
                await conn.OpenAsync();
                await using var tran =
                    conn.BeginTransaction(i % 2 == 0 ? IsolationLevel.Serializable : IsolationLevel.ReadUncommitted);
                var result = (Test: i, IsolationLevel: await GetIsolationLevel(conn, tran));
                tran.Commit();
                return result;
            }))).ToList().ForEach((t) => Console.WriteLine($"Test {t.Test}: {t.IsolationLevel}"));

            Console.WriteLine();
            Console.WriteLine("Using no transaction");

            (await Task.WhenAll(Enumerable.Range(21, 10).Select(async i =>
            {
                await using var conn = new SqlConnection(connStr);
                await conn.OpenAsync();
                return (Test: i, IsolationLevel: await GetIsolationLevel(conn));
            }))).ToList().ForEach((t) => Console.WriteLine($"Test {t.Test}: {t.IsolationLevel}"));
        }
    }
}

Using no transaction
Test 1: read committed snapshot
Test 2: read committed snapshot
Test 3: read committed snapshot
Test 4: read committed snapshot
Test 5: read committed snapshot
Test 6: read committed snapshot
Test 7: read committed snapshot
Test 8: read committed snapshot
Test 9: read committed snapshot
Test 10: read committed snapshot

Using serializable iso level transaction
Test 11: read uncommitted
Test 12: serializable
Test 13: read uncommitted
Test 14: serializable
Test 15: read uncommitted
Test 16: serializable
Test 17: read uncommitted
Test 18: serializable
Test 19: read uncommitted
Test 20: serializable

Using no transaction
Test 21: serializable
Test 22: read uncommitted
Test 23: serializable
Test 24: read uncommitted
Test 25: read committed snapshot
Test 26: read committed snapshot
Test 27: read committed snapshot
Test 28: read uncommitted
Test 29: serializable
Test 30: read uncommitted

Expected behavior

First batch of 10 tests show "read committed snapshot" isolation level
Second batch of 10 oscillate between "serializable" and "read uncommitted"
Third batch of 10 tests all show "read committed snapshot" again

Actual behavior

Third batch of 10 is a mismatch of "read committed snapshot", "read uncommitted" and "serializable"

Further technical details

Microsoft.Data.SqlClient version: 2.1.3
.NET target: Core 3.1
SQL Server version: SQL Server 2017
Operating system: Windows, Linux

[alexdawes]

Note - when using this (rather crude) wrapper around the SqlConnection that sets the iso level to READ COMMITTED when the Open method is called, the results change to be as expected.

using System.Data;

namespace ConsoleApp1
{
    public sealed class SafeConnection : IDbConnection
    {
        private readonly IDbConnection _connection;

        public SafeConnection(IDbConnection connection)
        {
            _connection = connection;
        }

        public void Dispose()
        {
            _connection.Dispose();
        }

        public IDbTransaction BeginTransaction()
        {
            return _connection.BeginTransaction();
        }

        public IDbTransaction BeginTransaction(IsolationLevel il)
        {
            return _connection.BeginTransaction(il);
        }

        public void ChangeDatabase(string databaseName)
        {
            _connection.ChangeDatabase(databaseName);
        }

        public void Close()
        {
            _connection.Close();
        }

        public IDbCommand CreateCommand()
        {
            return _connection.CreateCommand();
        }

        public void Open()
        {
            _connection.Open();
            var command = _connection.CreateCommand();
            command.CommandText = "SET TRANSACTION ISOLATION LEVEL READ COMMITTED;";
            command.ExecuteNonQuery();
        }

        public string ConnectionString
        {
            get => _connection.ConnectionString;
            set => _connection.ConnectionString = value;
        }

        public int ConnectionTimeout => _connection.ConnectionTimeout;

        public string Database => _connection.Database;

        public ConnectionState State => _connection.State;
    }
}

[cheenamalhotra]

Similar to #96.
Root cause explained: #96 (comment)

[alexdawes]

Thank you for your quick response - I had not spotted that other ticket.

Given it is 4 years old, am I right in assuming a fix is not likely to be on the horizon? I understand that the isolation level persisting for a connection after transaction rollback or commit is likely an intended behaviour within SQL Server's implementation, but when combined with connection pooling in the SqlClient it produces behaviour that seems both unexpected and potentially dangerous.

Connection pooling is a useful abstraction to avoid having to manage actual connections manually, but I think this abstraction heavily implies that separate SqlConnections will be independent, regardless of whether they use the same underlying shared connection. After all, SqlConnection instances are opened, closed, and disposed, with the state of the actual connection being completely abstracted away behind these actions.

Resetting the isolation level (as well as any other state on a connection) when a connection is returned to the pool (or alternatively when it is selected from the pool) would mean new SqlConnections are homogenous, and all behave identically, regardless of whether the connection has been used before and in what way.

I have no idea how complicated implementing this would be, but I assume its non-trivial as the behaviour has been known about for several years. I would definitely add my +1 to any attempt to fix this though, as unexpected locking and dirty reads feels pretty dangerous.

[cheenamalhotra]

I will close this as duplicate, and you may track this topic with #96.