diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 295286b1e5..52c7ef2f08 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -281,6 +281,14 @@ internal class SqlConnectionInternal : DbConnectionInternal, IDisposable // @TODO: Rename to match naming conventions (remove f prefix) private readonly bool _fResetConnection; + // Tracks whether a SQL Server session transaction isolation level + // change has been issued on this connection (via SqlTransaction or + // System.Transactions enlistment) but not yet undone. SQL Server's + // sp_reset_connection does not reset the session isolation level, so + // without compensation a pooled connection leaks the level to its + // next user. + private bool _isolationLevelDirty; + // @TODO: Rename to match naming conventions @@ -2679,6 +2687,16 @@ private void ExecuteTransaction2005( internalTransaction, stateObj, isDelegateControlRequest); + + // A successful Begin with a non-default isolation level mutates + // SQL Server's session transaction_isolation_level. sp_reset_connection + // will not undo this, so remember it and reset on deactivate. + if (requestType == TdsEnums.TransactionManagerRequestType.Begin && + isoLevel != TdsEnums.TransactionManagerIsolationLevel.Unspecified && + isoLevel != TdsEnums.TransactionManagerIsolationLevel.ReadCommitted) + { + _isolationLevelDirty = true; + } } finally { @@ -3894,6 +3912,42 @@ internal override void ResetConnection() // Reset dictionary values, since calling reset will not send us env_changes. CurrentDatabase = _originalDatabase; _currentLanguage = _originalLanguage; + + // sp_reset_connection (set up by PrepareResetConnection above) does not reset the + // session transaction isolation level. When we know a previous Begin changed the + // level, issue an explicit SET so the next user of this pooled connection starts at + // READ COMMITTED. The batch piggybacks the queued sp_reset_connection on its TDS + // header, so the round trip is shared and no separate reset packet is sent later. + // Gated by an app-context switch for back-compat. + if (_isolationLevelDirty && !LocalAppContextSwitches.UseLegacyIsolationLevelBehavior) + { + ResetSessionIsolationLevel(); + } + } + } + + // Issues "SET TRANSACTION ISOLATION LEVEL READ COMMITTED;" on the physical state object + // so the next user of this pooled connection observes the default session isolation level. + // Called only from ResetConnection when _isolationLevelDirty is set; runs synchronously + // because Deactivate is a synchronous pool-return path. + private void ResetSessionIsolationLevel() + { + try + { + _parser.TdsExecuteSQLBatch( + text: "SET TRANSACTION ISOLATION LEVEL READ COMMITTED;", + timeout: 0, + notificationRequest: null, + stateObj: _parser._physicalStateObj, + sync: true); + _parser.Run(RunBehavior.UntilDone, null, null, null, _parser._physicalStateObj); + _isolationLevelDirty = false; + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + // If we cannot scrub the session state, the connection is not safe to pool. + // Dooming forces Deactivate's outer logic to destroy it instead of pooling it. + DoomThisConnection(); } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs index df762ed7ae..0849d87f81 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs @@ -65,6 +65,14 @@ internal static class LocalAppContextSwitches private const string UseLegacyFailoverAlternationOnLoginSqlErrorsString = "Switch.Microsoft.Data.SqlClient.UseLegacyFailoverAlternationOnLoginSqlErrors"; + /// + /// The name of the app context switch that controls whether pooled connections + /// preserve the legacy behavior of leaking the SQL Server session + /// transaction isolation level across pool reuse. + /// + private const string UseLegacyIsolationLevelBehaviorString = + "Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior"; + /// /// The name of the app context switch that controls whether to preserve /// legacy behavior where Timestamp/RowVersion fields return empty byte @@ -201,6 +209,11 @@ private enum SwitchValue : byte /// private static SwitchValue s_useLegacyFailoverAlternationOnLoginSqlErrors = SwitchValue.None; + /// + /// The cached value of the UseLegacyIsolationLevelBehavior switch. + /// + private static SwitchValue s_useLegacyIsolationLevelBehavior = SwitchValue.None; + /// /// The cached value of the LegacyRowVersionNullBehavior switch. /// @@ -446,6 +459,22 @@ public static bool GlobalizationInvariantMode defaultValue: false, ref s_useLegacyFailoverAlternationOnLoginSqlErrors); + /// + /// When set to true, pooled connections preserve the legacy behavior where + /// the SQL Server session transaction isolation level is not reset on + /// return to the pool. As a result, the next user of the pooled connection + /// may inherit a non-default isolation level. + /// + /// The default value of this switch is false, meaning the driver will + /// reset the isolation level to READ COMMITTED before the next checkout + /// when it knows the level was changed. + /// + public static bool UseLegacyIsolationLevelBehavior => + AcquireAndReturn( + UseLegacyIsolationLevelBehaviorString, + defaultValue: false, + ref s_useLegacyIsolationLevelBehavior); + /// /// In System.Data.SqlClient and Microsoft.Data.SqlClient prior to 3.0.0 a /// field with type Timestamp/RowVersion would return an empty byte array. diff --git a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs index 430d6645a8..c68bde6c81 100644 --- a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs +++ b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs @@ -46,6 +46,7 @@ public sealed class LocalAppContextSwitchesHelper : IDisposable #endif private readonly bool? _ignoreServerProvidedFailoverPartnerOriginal; private readonly bool? _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal; + private readonly bool? _useLegacyIsolationLevelBehaviorOriginal; private readonly bool? _legacyRowVersionNullBehaviorOriginal; private readonly bool? _legacyVarTimeZeroScaleBehaviourOriginal; private readonly bool? _makeReadAsyncBlockingOriginal; @@ -100,6 +101,8 @@ public LocalAppContextSwitchesHelper() GetSwitchValue("s_ignoreServerProvidedFailoverPartner"); _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal = GetSwitchValue("s_useLegacyFailoverAlternationOnLoginSqlErrors"); + _useLegacyIsolationLevelBehaviorOriginal = + GetSwitchValue("s_useLegacyIsolationLevelBehavior"); _legacyRowVersionNullBehaviorOriginal = GetSwitchValue("s_legacyRowVersionNullBehavior"); _legacyVarTimeZeroScaleBehaviourOriginal = @@ -161,6 +164,9 @@ public void Dispose() SetSwitchValue( "s_useLegacyFailoverAlternationOnLoginSqlErrors", _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal); + SetSwitchValue( + "s_useLegacyIsolationLevelBehavior", + _useLegacyIsolationLevelBehaviorOriginal); SetSwitchValue( "s_legacyRowVersionNullBehavior", _legacyRowVersionNullBehaviorOriginal); @@ -261,6 +267,15 @@ public bool? UseLegacyFailoverAlternationOnLoginSqlErrors set => SetSwitchValue("s_useLegacyFailoverAlternationOnLoginSqlErrors", value); } + /// + /// Get or set the UseLegacyIsolationLevelBehavior switch value. + /// + public bool? UseLegacyIsolationLevelBehavior + { + get => GetSwitchValue("s_useLegacyIsolationLevelBehavior"); + set => SetSwitchValue("s_useLegacyIsolationLevelBehavior", value); + } + /// /// Get or set the LegacyRowVersionNullBehavior switch value. /// diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj index 1145fa4790..a011352d38 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj @@ -233,6 +233,7 @@ + diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs new file mode 100644 index 0000000000..055d7e4439 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs @@ -0,0 +1,150 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Data; +using System.Transactions; +using Microsoft.Data.SqlClient.Tests.Common; +using Xunit; +using IsolationLevel = System.Data.IsolationLevel; + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + // SqlTransaction / TransactionScope used to leave the pooled connection + // with the elevated session isolation level. The next user of the pooled + // connection would silently inherit it. The fix resets the session + // isolation level to READ COMMITTED when the connection is returned to + // the pool. + public static class IsolationLevelLeakTest + { + private const string GetIsoSql = @" +SELECT CASE transaction_isolation_level + WHEN 0 THEN 'Unspecified' + WHEN 1 THEN 'ReadUncommitted' + WHEN 2 THEN 'ReadCommitted' + WHEN 3 THEN 'RepeatableRead' + WHEN 4 THEN 'Serializable' + WHEN 5 THEN 'Snapshot' + END +FROM sys.dm_exec_sessions WHERE session_id = @@SPID;"; + + private static string BuildPooledConnString(string appName) => + new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + Pooling = true, + MaxPoolSize = 1, + MultipleActiveResultSets = false, + Enlist = true, + ApplicationName = appName + }.ConnectionString; + + private static int GetSpid(SqlConnection c) + { + using SqlCommand cmd = new SqlCommand("SELECT @@SPID;", c); + return Convert.ToInt32(cmd.ExecuteScalar()); + } + + private static string GetIso(SqlConnection c) + { + using SqlCommand cmd = new SqlCommand(GetIsoSql, c); + return (string)cmd.ExecuteScalar(); + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static void SqlTransaction_SerializableDoesNotLeakAcrossPool() + { + string cs = BuildPooledConnString("IsoLeakTest-SqlTx"); + int spid1; + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + spid1 = GetSpid(c); + using SqlTransaction tx = c.BeginTransaction(IsolationLevel.Serializable); + Assert.Equal("Serializable", GetIsoOnTx(c, tx)); + tx.Rollback(); + } + + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + Assert.Equal(spid1, GetSpid(c)); // pool reuse + Assert.Equal("ReadCommitted", GetIso(c)); + } + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static void TransactionScope_SerializableDoesNotLeakAcrossPool() + { + string cs = BuildPooledConnString("IsoLeakTest-TxScope"); + int spid1; + using (var scope = new TransactionScope( + TransactionScopeOption.RequiresNew, + new TransactionOptions { IsolationLevel = System.Transactions.IsolationLevel.Serializable }, + TransactionScopeAsyncFlowOption.Enabled)) + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + spid1 = GetSpid(c); + Assert.Equal("Serializable", GetIso(c)); + scope.Complete(); + } + + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + Assert.Equal(spid1, GetSpid(c)); + Assert.Equal("ReadCommitted", GetIso(c)); + } + } + + // Negative test: legacy switch ON brings the old leak back. + // Only meaningful on on-prem SQL Server: Azure SQL DB resets the + // session isolation level inside sp_reset_connection, so the leak + // never materializes there regardless of the switch. + // Uses LocalAppContextSwitchesHelper to force the cached switch value + // in LocalAppContextSwitches (production code reads the cache, not + // AppContext, after first use). The helper restores the previous + // value on dispose. + [ConditionalFact( + typeof(DataTestUtility), + nameof(DataTestUtility.AreConnStringsSetup), + nameof(DataTestUtility.IsNotAzureServer))] + public static void LegacySwitch_PreservesOldLeakBehavior() + { + using LocalAppContextSwitchesHelper switchesHelper = new(); + switchesHelper.UseLegacyIsolationLevelBehavior = true; + + // Distinct app name so this test gets its own pool group. + string cs = BuildPooledConnString("IsoLeakTest-Legacy"); + try + { + int spid1; + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + spid1 = GetSpid(c); + using SqlTransaction tx = c.BeginTransaction(IsolationLevel.Serializable); + tx.Rollback(); + } + + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + Assert.Equal(spid1, GetSpid(c)); + Assert.Equal("Serializable", GetIso(c)); // legacy: leaks + } + } + finally + { + SqlConnection.ClearAllPools(); + } + } + + private static string GetIsoOnTx(SqlConnection c, SqlTransaction tx) + { + using SqlCommand cmd = new SqlCommand(GetIsoSql, c, tx); + return (string)cmd.ExecuteScalar(); + } + } +}