SqlTransaction and TransactionScope leak isolation level

[madelson]

The TransactionScope class seems to "leak" it's isolation level to future queries on the same (pooled) connection.

I would expect the isolation level of the connection to be restored when the transaction ends or is disposed.

Here is a snippet which reproduces the behavior:

SqlConnection.ClearAllPools();
var conn = new SqlConnection(new SqlConnectionStringBuilder { DataSource = @".\sqlexpress", IntegratedSecurity = true }.ConnectionString);

Action printIsolationLevel = () =>
{
    var cmd = conn.CreateCommand();
    cmd.CommandText = @"SELECT CASE transaction_isolation_level 
                        WHEN 0 THEN 'Unspecified' 
                        WHEN 1 THEN 'ReadUncommitted' 
                        WHEN 2 THEN 'ReadCommitted' 
                        WHEN 3 THEN 'Repeatable' 
                        WHEN 4 THEN 'Serializable' 
                        WHEN 5 THEN 'Snapshot' END AS TRANSACTION_ISOLATION_LEVEL 
                        FROM sys.dm_exec_sessions 
                        where session_id = @@SPID";
    Console.WriteLine(cmd.ExecuteScalar());
};

conn.Open();
printIsolationLevel(); // "ReadCommitted"
conn.Close();

using (var scope = new TransactionScope(
                    TransactionScopeOption.RequiresNew,
                    new TransactionOptions { IsolationLevel = System.Transactions.IsolationLevel.Serializable }))
{
    conn.Open();
    printIsolationLevel(); // "Serializable"
    conn.Close();
}

conn.Open();
printIsolationLevel(); // "Serializable" !!?
conn.Close();

[danmoseley]

TransactionsScope is in S.Transactions. Of course I don't know where the bug is..

[madelson]

Further investigations suggest that this may be a problem with SqlTransaction as well.

[jimcarley]

This may be a Sql connection pooling issue causing retention of the isolation level that was obtained from the transaction that was created and completed by the TransactionScope.

From: Dan Moseley [mailto:notifications@github.com] Sent: Wednesday, April 12, 2017 5:19 PM To: dotnet/corefx corefx@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [dotnet/corefx] TransactionScope leaks isolation level (#18277)

TransactionsScope is in S.Transactions. Of course I don't know where the bug is..

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcorefx%2Fissues%2F18277%23issuecomment-293742713&data=02%7C01%7Cjcarley%40microsoft.com%7Cdc1af23791ba4c8bfb2a08d48202bc91%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276395643083008&sdata=dFoWFD%2BpU7st62p6b%2BHwT%2BR%2F6cQwe9sZTjTslBkrpzU%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAMw2-Gl4lfajvOBedRdZ-ej2kfkpBXdLks5rvWoKgaJpZM4M7d7l&data=02%7C01%7Cjcarley%40microsoft.com%7Cdc1af23791ba4c8bfb2a08d48202bc91%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276395643083008&sdata=E%2FxOU%2BlEEKUw2%2FdjCGqp6Z4KPLE5Q0NCYYhF3PqrqcU%3D&reserved=0.

[madelson]

The fact that the isolation level change goes on to pollute the connection pool makes the much more worrisome. However, even on a single connection we can see weird behavior where the transaction isolation level leaks out of the transaction scope:

SqlConnection.ClearAllPools();
var conn = new SqlConnection(new SqlConnectionStringBuilder { DataSource = @".\sqlexpress", IntegratedSecurity = true }.ConnectionString);

Action<SqlTransaction> printIsolationLevel = (SqlTransaction transaction) =>
{
    var cmd = conn.CreateCommand();
    cmd.Transaction = transaction;
    cmd.CommandText = @"SELECT CASE transaction_isolation_level 
                        WHEN 0 THEN 'Unspecified' 
                        WHEN 1 THEN 'ReadUncommitted' 
                        WHEN 2 THEN 'ReadCommitted' 
                        WHEN 3 THEN 'Repeatable' 
                        WHEN 4 THEN 'Serializable' 
                        WHEN 5 THEN 'Snapshot' END AS TRANSACTION_ISOLATION_LEVEL 
                        FROM sys.dm_exec_sessions 
                        where session_id = @@SPID";
    Console.WriteLine(cmd.ExecuteScalar());
};

conn.Open();

printIsolationLevel(null); // ReadCommitted

using (var transaction = conn.BeginTransaction(System.Data.IsolationLevel.Serializable))
{
    printIsolationLevel(transaction); // Serializable
}

printIsolationLevel(null); // Serializable

conn.Close();

[jimcarley]

@saurabh500 Including you because it looks like you were the last person to touch SqlConnection.

This looks like a question for SqlConnection or SqlCommand.

[danmoseley]

@corivera ?

[saurabh500]

Right now SqlClient doesn't support TransactionScope in .Net Core . We are looking into SqlTransaction right now.

[geleems]

I performed some investigation, and realized this behavior is by design. Here is my test code:

public static void Test()
{
    string connString = "Server=tcp:<hostname>,1433;User ID=<id>;Password=<pw>;Connect Timeout=600;";

    SqlConnection.ClearAllPools();
    var conn = new SqlConnection(new SqlConnectionStringBuilder(connString).ConnectionString);

    Action<SqlTransaction> printIsolationLevel = (SqlTransaction transaction) =>
    {
        var cmd = conn.CreateCommand();
        cmd.Transaction = transaction;
        cmd.CommandText = @"SELECT CASE transaction_isolation_level 
                WHEN 0 THEN 'Unspecified' 
                WHEN 1 THEN 'ReadUncommitted' 
                WHEN 2 THEN 'ReadCommitted' 
                WHEN 3 THEN 'Repeatable' 
                WHEN 4 THEN 'Serializable' 
                WHEN 5 THEN 'Snapshot' END AS TRANSACTION_ISOLATION_LEVEL 
                FROM sys.dm_exec_sessions 
                where session_id = @@SPID";
        Console.WriteLine(cmd.ExecuteScalar());
    };

    conn.Open();

    printIsolationLevel(null); // ReadCommitted

    using (var transaction = conn.BeginTransaction(System.Data.IsolationLevel.Serializable))
    {
        printIsolationLevel(transaction); // Serializable
    }

    printIsolationLevel(null); // Serializable

    using (var transaction = conn.BeginTransaction(System.Data.IsolationLevel.ReadUncommitted))
    {
        printIsolationLevel(transaction); // ReadUncommitted
    }

    printIsolationLevel(null); // ReadUncommitted

    conn.Close();
}

Default isolation level is ReadCommitted, and it is replaced to new isolation level value when a transaction object is created. The isolation level value remains in connection of server side (client side does not store anything after transaction is disposed) even after transaction is finished because there is no need to reset it to default since isolation level is only consumed by transaction, and the value will be replaced to new value anyway when new transaction is started.

This behavior is intended and described in TDS protocol design document (https://msdn.microsoft.com/en-us/library/dd339887.aspx). When a transaction object is disposed, it internally executes ROLLBACK, which sends TM_ROLLBACK_XACT request to server.

Detail payload of the TM_ROLLBACK_XACT request is shown below:

 fBeginXact         =   BIT

 XACT_FLAGS         =   fBeginXact
                        7FRESERVEDBIT

 ISOLATION_LEVEL    =   BYTE

 XACT_NAME          =   B_VARBYTE
 BEGIN_XACT_NAME    =   B_VARBYTE

 RequestPayload     =   XACT_NAME
                        XACT_FLAGS
                        [ISOLATION_LEVEL
                        BEGIN_XACT_NAME]

If fBeginXact is 1, then ISOLATION_LEVEL can specify the isolation level to use to start the new transaction, according to the definition at the end of this table. If fBeginXact is 0, then ISOLATION_LEVEL SHOULD NOT be present.

fBeginXact bit is obviously set to 0 when transaction object is disposed, and RequestPayload does not have ISOLATION_LEVEL value intentionally.

You can see this payload implementation at: https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs#L6374

ISOLATION_LEVEL Value: 0x00 Description: No isolation level change requested. Use current.

According to the spec document, ROLLBACK request does not change isolation level intentionally, and let it use current. So, this behavior is intended rather than a bug.

[madelson]

@geleems it isn't true that isolation level only affects transaction. As long as SQL is in autocommit mode (the default), each statement executes in an implicit transaction using the current isolation level. This is what caused me to report the bug.

This is particularly problematic because of connection pooling, since the isolation levels can leak and affect any other part of the application.

[geleems]

@madelson

since isolation level is only consumed by transaction

I meant isolation level parameter is only consumed by method that creates transaction, which is SqlConnection.BeginTransaction(). Once isolation level is set to certain value, it will remain at that setting through the session as intended until you change it. If you want to just change the isolation level setting, and don't want transaction, you can do as following:

SqlConnection.BeginTransaction(IsolationLevel.ReadCommitted).Dispose();

However, I identified it is a problem that isolation level is not cleaned up when connection moves back to pool.

[madelson]

@geleems I agree that the behavior with pooling represents the biggest problem here; that's what burned us.

Maybe it's a separate issue, but I do think that most callers to the API would expect the isolation level to reset on dispose despite the documentation that says otherwise. It seems weird that the recommended usage pattern would be to follow each transaction with another empty transaction just to get reasonable behavior. Much like the scoping of the foreach var (which changed in C#5), this seems like it could be one of those cases where despite the current behavior being intentional it is so confusing that it might be worth changing.

[madelson]

Here's a link with some additional context on this issue: https://social.msdn.microsoft.com/Forums/sqlserver/en-US/916b3d8a-c464-4ad5-8901-6f845a2a3447/sql-server-2014-reseting-isolation-level?forum=sqldatabaseengine

[divega]

As recently announced in the .NET Blog, focus on new SqlClient features an improvements is moving to the new Microsoft.Data.SqlClient package. For this reason, we are moving this issue to the new repo at https://github.com/dotnet/SqlClient. We will still use https://github.com/dotnet/corefx to track issues on other providers like System.Data.Odbc and System.Data.OleDB, and general ADO.NET and .NET data access issues.

[madelson]

Thanks @divega . Hopefully this new package will provide a chance to fix wonky behaviors like this one!

[nycdotnet]

This behavior is surprising and the sort of thing that is hard to notice in happy-path testing, but causes issues in prod. The way we've worked around it is to change the application name for any connections that opt-in to use snapshot isolation. This has the practical effect of ADO.NET creating two connection pools (my-service and my-service-snapshot), so when the isolation mode sticks to the connections in the snapshot pool it doesn't hurt anything. It also helps with troubleshooting on the server-side because we can see which isolation mode the application was intending to use.

[szalapski]

I am encountering this now too, and I am surprised it is still unresolved. Any update?

[cheenamalhotra]

Additional repro: https://github.com/dotnet/SqlClient/issues/1098

[justinbhopper]

in the snapshot pool it doesn't hurt anything.

Actually, it can hurt if you use that connection to run stored procedures that start their own transaction. The stored procedure may not have used SET TRANSACTION ISOLATION LEVEL, expecting the default isolation level to be utilized.

I think the confusion of all of this comes from the fact that the isolation level can be set via the construction of a disposable object (the transaction), rather than just a call directly on the connection.

Confusing:

using (var transaction = connection.BeginTransaction(IsolationLevel.Snapshot))
{
  ...
} // By now the transaction and isolation level should be reverted

Not confusing:

using (var transaction = connection.BeginTransaction())
{
  connection.SetIsolationLevel(IsolationLevel.Snapshot);
  ...
} // Now it is more clear that the isolation level was not related to the transaction's lifecycle

[nycdotnet]

@justinbhopper agree that is also a problem, but that's a whole separate issue which would cause pain even if the isolation level did not stick to a disposed connection.

[tbolon]

Also related: #146

[rgroenewoudt]

We are still running into this problem, without using TransactionScope.

[szalapski]

Agreed, I think this should be fixed, at least with an option at connection instantiation time to reset the "new" (reused) connection to the database-default isolation level if necessary. It isn't confined to usages of TransactionScope. The desired example code:

SqlConnection sqlConnection = new(connectionString) {ResetIsolationLevelOnReuse = true};

Or perhaps the setting should cause a reset at "close" time, after any commit or rollback, so that a network traffic penalty isn't incurred before every query?

SqlConnection sqlConnection = new(connectionString) {ResetIsolationLevelOnClose = true};

An alternative might be to have an option in the TransactionScope. I had written some sample code for that, but I've now edited this to remove it, as I don't think it adequately solves the problem because the problem exists even without any explicit transactions.

Without any of these, I am methodically going through my code and adding this T-SQL workaround at the end of every query that changes the isolation level. The GO is important to ensure the SET after it runs even if there is an error before it.

GO; SET TRANSACTION ISOLATION LEVEL READ COMMITTED; /* workaround for https://github.com/dotnet/SqlClient/issues/96 */

@DavoudEshtehari or @JRahnama what would it take to get this into High Priority or otherwise put it up for reconsideration? It is easily reproduced and perhaps one of the solutions above is elegant and simple?

[roji]

FWIW this seems like a bug to me, rather than something that the user should have to fix via some special opt-in flag. If there's worry of the fix affecting people who are relying on the current (buggy) behavior, I'd suggest an appcontext switch to maintain the current behavior at most...