Reset session isolation level on pool return (fixes #96)#4330
Draft
priyankatiwari08 wants to merge 3 commits into
Draft
Reset session isolation level on pool return (fixes #96)#4330priyankatiwari08 wants to merge 3 commits into
priyankatiwari08 wants to merge 3 commits into
Conversation
SqlTransaction and TransactionScope leave the underlying SQL Server session with the elevated isolation level after Commit/Rollback. sp_reset_connection does not reset it, so the next user of the pooled physical connection silently inherits it. This change tracks when a non-default isolation level was set via the TM Begin path and, on pool return, issues a 'SET TRANSACTION ISOLATION LEVEL READ COMMITTED' batch right before sp_reset_connection (piggybacked on the same TDS header, no extra round trip). On failure the connection is doomed instead of returned to the pool. A new AppContext switch 'Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior' preserves previous behavior for callers that depend on it (default: false). Adds IsolationLevelLeakTest under ManualTests covering the two repro scenarios plus a negative test for the kill switch.
priyankatiwari08
force-pushed
the
feature/sqltransaction-isolation-leak
branch
from
4e8ed47 to
9f67fb1
Compare
Comment on lines
+31
to
+38
| private static string PooledMaxOneConnString => | ||
| new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) | ||
| { | ||
| Pooling = true, | ||
| MaxPoolSize = 1, | ||
| MultipleActiveResultSets = false, | ||
| ApplicationName = "IsoLeakTest" | ||
| }.ConnectionString; |
Comment on lines
+106
to
+109
| const string Switch = "Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior"; | ||
| AppContext.SetSwitch(Switch, true); | ||
| try | ||
| { |
Comment on lines
+136
to
+140
| finally | ||
| { | ||
| AppContext.SetSwitch(Switch, false); | ||
| SqlConnection.ClearAllPools(); | ||
| } |
apoorvdeshmukh
requested changes
Jun 1, 2026
Contributor
apoorvdeshmukh
left a comment
apoorvdeshmukh
left a comment
There was a problem hiding this comment.
Can you look into this test failure?
Seems related to the changes
github-project-automation
Bot
moved this from To triage
to Waiting for customer
in SqlClient Board
- IsolationLevelLeakTest: switch to LocalAppContextSwitchesHelper so the cached LocalAppContextSwitches value is forced, not just set via AppContext.SetSwitch (production code reads the cache after first use, which made the negative test order-dependent and caused the CI failure). - Helper restores the previous value on dispose, so global state does not leak to other tests. - TransactionScope test: set Enlist=true explicitly on the pooled connection string so the test does not depend on the user's local DataTestUtility.TCPConnectionString defaults. - LocalAppContextSwitchesHelper: expose UseLegacyIsolationLevelBehavior get/set + capture/restore wired through the standard reflection helpers.
apoorvdeshmukh
requested changes
Jun 3, 2026
Contributor
apoorvdeshmukh
left a comment
apoorvdeshmukh
left a comment
There was a problem hiding this comment.
Can you look into this failure for LegacySwitch_PreservesOldLeakBehavior?
Tests are mostly failing with this one. You can put the PR into draft till this is addressed.
Is this happening only in CI or do you observe it locally under VS as well?
github-project-automation
Bot
moved this from In progress
to Waiting for customer
in SqlClient Board
apoorvdeshmukh
added
the
Author attention needed
The leak only manifests on on-prem SQL Server: Azure SQL DB resets the session isolation level inside sp_reset_connection, so the legacy switch becomes a no-op there and the assertion (expects 'Serializable' to leak) fails on Azure CI legs.
Comment on lines
+5
to
+9
| using System; | ||
| using System.Data; | ||
| using System.Transactions; | ||
| using Microsoft.Data.SqlClient.Tests.Common; | ||
| using Xunit; |
Comment on lines
+462
to
+476
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| public static bool UseLegacyIsolationLevelBehavior => | ||
| AcquireAndReturn( | ||
| UseLegacyIsolationLevelBehaviorString, | ||
| defaultValue: false, | ||
| ref s_useLegacyIsolationLevelBehavior); |
edwardneal
reviewed
Jun 3, 2026
| isoLevel != TdsEnums.TransactionManagerIsolationLevel.Unspecified && | ||
| isoLevel != TdsEnums.TransactionManagerIsolationLevel.ReadCommitted) | ||
| { | ||
| _isolationLevelDirty = true; |
Contributor
There was a problem hiding this comment.
It's worth noting that Azure Synapse Analytics does things slightly differently - it only supports the SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED statement, and the default isolation level can vary based upon a database option.
I think this might also present an issue for #4335.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #96.
Problem
SqlTransactionandTransactionScopeleave the underlying SQL Serversession with the elevated isolation level after Commit/Rollback. Because
sp_reset_connectiondoes not reset the session isolation level, the nextuser of the pooled physical connection silently inherits it.
Repro
using var c = new SqlConnection(cs); c.Open();using var tx = c.BeginTransaction(IsolationLevel.Serializable); tx.Rollback();sys.dm_exec_sessions.transaction_isolation_levelis still 4 (Serializable).Reproduced on 5.2.2 and 7.0.1 against SQL Server, on both net8.0 and net472.
Fix
SqlInternalConnectionTdswhen a non-default isolation level was set via the TMBeginpath (_isolationLevelDirty).ResetConnection()(called fromDeactivateon pool return), if dirty, issueSET TRANSACTION ISOLATION LEVEL READ COMMITTED;viaTdsExecuteSQLBatch. The batch piggybacks the queuedsp_reset_connectionbyte on the same TDS header, so there is no extra round trip vs. the legacy reset.Kill switch
Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior(defaultfalse) restores the previous behavior.Tests
Adds
IsolationLevelLeakTestunderManualTests/SQL/TransactionTest/:SqlTransaction_SerializableDoesNotLeakAcrossPoolTransactionScope_SerializableDoesNotLeakAcrossPoolLegacySwitch_PreservesOldLeakBehavior(negative test)Validation
Built clean for net462 / net8.0 / net9.0. Standalone repro run end-to-end against local SQL Server:
[After]isolation is nowReadCommittedon both scenarios with the same pooled SPID. With the legacy switch enabled,[After]isSerializable(kill-switch verified).