SqlCommand retry after transaction abort can partially save data to database

[Chakrygin]

Describe the bug

When an exception occurs on SqlCommand execution, and the exception is related with transaction abort, the Transaction property of SqlCommand is set to null.

Execution retry of such SqlCommand can be successful, lead to partial change saving of transaction and data corruption.

I think, this behavior is dangerous, because it may be done out of ignorance.

For example, this problem occurs in the linq2db project, where one command is used in the retry policy for SqlServer several times:

https://github.com/linq2db/linq2db/blob/21380afa89df514e01e28946e64029766ec28485/Source/LinqToDB/Data/RetryPolicy/RetryingDbCommand.cs#L105

For compare, in the Npgsql project, command execution retry throws exception with message: "current transaction is aborted".

To reproduce

Start sqlserver in a docker container:

docker run --rm --detach ^
    --name SqlServer ^
    --publish 1433:1433 ^
    --env "ACCEPT_EULA=Y" ^
    --env "SA_PASSWORD=Password12!" ^
    mcr.microsoft.com/mssql/server:2017-CU20-ubuntu-16.04

Run following program (it simulates a deadlock and transaction abort):

public static class Program
{
    public static async Task Main(string[] args)
    {
        await ExecuteMaster($@"
            if db_id('Test') is not null
            begin
                alter database Test set single_user with rollback immediate;
                drop database Test;
            end");

        await ExecuteMaster($@"
            create database Test");

        await ExecuteTest($@"
            create table Item1 (Id int, Value int)");
        await ExecuteTest($@"
            create table Item2 (Id int, Value int)");

        await ExecuteTest($@"
            insert into Item1 (Id, Value) values (1, 0)");
        await ExecuteTest($@"
            insert into Item2 (Id, Value) values (1, 0)");

        var connectionString = CreateConnectionString("Test");

        var counter = 0;

        // This transaction updates the Item1 table, then Item2 table and set value "1" in both tables.
        var task1 = Task.Run(async () =>
        {
            await using var connection = new SqlConnection(connectionString);

            await connection.OpenAsync();

            await using var transaction = connection.BeginTransaction(IsolationLevel.ReadCommitted);
            
            await UpdateWithRetry(connection, transaction, table: "Item1", value: 1);

            Interlocked.Increment(ref counter);

            while (Volatile.Read(ref counter) < 2)
                await Task.Delay(1000);

            await UpdateWithRetry(connection, transaction, table: "Item2", value: 1);
          
            await transaction.CommitAsync();
        });

        // This transaction updates the Item2 table, then Item1 table and set value "2" in both tables.
        var task2 = Task.Run(async () =>
        {
            await using var connection = new SqlConnection(connectionString);

            await connection.OpenAsync();

            await using var transaction = connection.BeginTransaction(IsolationLevel.ReadCommitted);

            await UpdateWithRetry(connection, transaction, table: "Item2", value: 2);

            Interlocked.Increment(ref counter);

            while (Volatile.Read(ref counter) < 2)
                await Task.Delay(1000);
            
            await UpdateWithRetry(connection, transaction, table: "Item1", value: 2);

            await transaction.CommitAsync();
        });

        try
        {
            await Task.WhenAll(task1, task2);
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex);
        }
    }

    private static async Task UpdateWithRetry(SqlConnection connection, SqlTransaction transaction, string table, int value)
    {
        var query = $"update {table} set Value = {value} where Id = 1";
        await using var command = new SqlCommand(query);

        command.Connection = connection;
        command.Transaction = transaction;

        try
        {
            await command.ExecuteNonQueryAsync();
        }
        catch (Exception ex)
        {
            // Retry:
            await command.ExecuteNonQueryAsync();
        }
    }

    private static Task ExecuteMaster(string query) => Execute(query, initialCatalog: "master");
    private static Task ExecuteTest(string query) => Execute(query, initialCatalog: "Test");

    private static async Task Execute(string query, string initialCatalog)
    {
        var connectionString = CreateConnectionString(initialCatalog);

        await using var connection = new SqlConnection(connectionString);
        await using var command = new SqlCommand(query, connection);

        connection.Open();

        await command.ExecuteNonQueryAsync();
    }

    private static string CreateConnectionString(string initialCatalog)
    {
        var csb = new SqlConnectionStringBuilder
        {
            DataSource = "localhost",
            InitialCatalog = initialCatalog,
            UserID = "sa",
            Password = "Password12!",
            Encrypt = false,
        };

        return csb.ToString();
    }
}

Expected behavior

Only one transaction has been completed successfully. Both tables (Item1 and Item2) will contain the same value: 1 or 2.

Current behavior

One transaction will complete successfully. In the second transaction, the second update will also complete successfully.
One table contains the value 1 and the other table contains the value 2.

Further technical details

Microsoft.Data.SqlClient version: 4.1.0
.NET target: Core 3.1
SQL Server version: mcr.microsoft.com/mssql/server:2017-CU20-ubuntu-16.04

[DavoudEshtehari]

Thanks, I'll add it to the triage for more discussion.

[JRahnama]

@Chakrygin seems like a deadlock issue is happening on resources while another process tries to get access to them.

[Chakrygin]

@JRahnama

The problem is not deadlock. The problem is in a SqlCommand behavior after the deadlock. (The Transaction property reset to null).

Deadlocks just demonstrate the problem.

[thibault-reigner]

For the record I faced the issue as well roughly a year ago in an in-house ORM, and I can confirm the cause being a deadlock resulting in a zombified transaction, for the following scenario (which is a bit tricky) + the "fix" I have added to the ORM to circumvent the bug:

I have a set of objects to insert into different tables, which are linked through foreign keys, so I can follow some graph scheduling (insertion into A to be done before B, if a foreign key from B to A exists).

So overall :

• I trigger over a same DbConnection / DbTransaction several async queries (insertions into different tables, according to dependency order resulting from foreign keys), but over a single thread (so not actual parallelism / MARS is involved here). The implementation relies on a queue of Tasks being filled according to the graph order.
• if a query faces an failure due to a deadlock, subsequent queries having no dependencies over the failed query will be processed regardless, the issue is that these queries won't result in failure too, but simply in the zombified transaction being ignored, and data being inserted outside any transaction. (<- major issue here ⚠️ )

After investigation I noticed that the following code does not perform a check over the status of the supplied SqlTransaction, (despite being the most expected location for such assertion from a user point of view in my opinion) :

        var cmd = dbConnection.CreateCommand();
        cmd.Transaction =dbTransaction; // this should throw if the transaction is zombified yet it does not
        // the command will simply ignore this zombified transaction when running the next query.

I can't recall the detail but there is something like Yukon transaction being in zombie state in SqlClient internal code.

Following the documentation, I managed to identify a side behavior of DbTransaction where the DbConnection getter interestingly becomes null, if the transaction is not valid anymore, so I have added the following check before the command setup code mentioned above :

    public static void ThrowForNotValidTransaction(this IHasDbTransaction dbConnectionWrapper)
    {
        if (dbConnectionWrapper.DbTransaction != null)
            ThrowForNotValidTransaction(dbConnectionWrapper.DbTransaction);
    }

    public static void ThrowForNotValidTransaction(this DbTransaction dbTransaction)
    {
        // There is no easy way to check if the transaction is still usable, but DbTransaction documentation
        // suggests that if IDbTransaction.Connection is null, then the transaction is not usable anymore.
        // Hence we can perform an early check here instead of letting the ADO.NET provider throw later when
        // the command is executed.
        if (((IDbTransaction)dbTransaction).Connection == null)
        {
            throw new InvalidOperationException($"The transaction is no longer valid (IDbTransaction.Connection = null)");
        }
    }