From 84a4a785318b34fd40f59851f83a54e92e98b260 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Fri, 10 Apr 2026 15:38:52 -0300 Subject: [PATCH 1/4] Add tests confirming InvalidCastException race in TryOpenInner (#3314) Add functional tests that reproduce the root cause of issue #3314: when _innerConnection is DbConnectionClosedConnecting (a transitional state), the cast to SqlConnectionInternal in TryOpenInner throws InvalidCastException. This occurs when a SqlConnection is used concurrently from multiple threads. Tests: - Confirm DbConnectionClosedConnecting is not assignable to SqlConnectionInternal - Confirm the cast throws InvalidCastException - Confirm Connecting state is reported correctly - Confirm Open() on a connecting connection throws InvalidOperationException --- .../SqlConnectionTest.ConcurrentOpen.cs | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs new file mode 100644 index 0000000000..84841fae10 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs @@ -0,0 +1,130 @@ +// 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.Reflection; +using Xunit; + +namespace Microsoft.Data.SqlClient.Tests +{ + /// + /// Tests that confirm the race condition described in GitHub issue #3314: + /// If _innerConnection is set to DbConnectionClosedConnecting (a transitional state) + /// at the moment TryOpenInner casts it to SqlConnectionInternal, an InvalidCastException + /// is thrown. This simulates a concurrent Open() on the same SqlConnection instance. + /// + public partial class SqlConnectionTest + { + /// + /// Verifies that when _innerConnection holds DbConnectionClosedConnecting (a non-open + /// transitional state), casting it to SqlConnectionInternal throws InvalidCastException. + /// This is the exact exception observed in issue #3314. + /// + [Fact] + public void InnerConnection_CastToSqlConnectionInternal_ThrowsInvalidCast_WhenInConnectingState() + { + // Arrange: get the DbConnectionClosedConnecting singleton via reflection + Type closedConnectingType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true); + FieldInfo singletonField = closedConnectingType + .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public); + Assert.NotNull(singletonField); + object connectingSingleton = singletonField.GetValue(null); + Assert.NotNull(connectingSingleton); + + // Verify it is NOT SqlConnectionInternal — this is the root cause of the InvalidCastException + Type sqlConnectionInternalType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.SqlClient.Connection.SqlConnectionInternal", throwOnError: true); + Assert.False( + sqlConnectionInternalType.IsInstanceOfType(connectingSingleton), + "DbConnectionClosedConnecting must not be assignable to SqlConnectionInternal"); + + // Act: set a SqlConnection's _innerConnection to the Connecting state to simulate the race + var connection = new SqlConnection("Data Source=localhost"); + FieldInfo innerConnectionField = typeof(SqlConnection) + .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(innerConnectionField); + + innerConnectionField.SetValue(connection, connectingSingleton); + + // Read it back through InnerConnection and attempt the same cast that TryOpenInner does + PropertyInfo innerConnectionProperty = typeof(SqlConnection) + .GetProperty("InnerConnection", BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(innerConnectionProperty); + object innerConnection = innerConnectionProperty.GetValue(connection); + + // Assert: the runtime type is not assignable to SqlConnectionInternal + Assert.False( + sqlConnectionInternalType.IsAssignableFrom(innerConnection.GetType()), + $"Expected InnerConnection type '{innerConnection.GetType().FullName}' to NOT be assignable " + + $"to '{sqlConnectionInternalType.FullName}'. If it were, the cast in TryOpenInner would " + + "succeed and the race condition in issue #3314 would not manifest as InvalidCastException."); + + // Perform the exact cast that TryOpenInner does at SqlConnection.cs line 2228: + // var tdsInnerConnection = (SqlConnectionInternal)InnerConnection; + // This must throw InvalidCastException when _innerConnection is DbConnectionClosedConnecting. + Exception ex = Assert.ThrowsAny(() => + { + // Use Convert.ChangeType or direct cast via reflection to replicate + // the CLR's cast behavior for internal types we cannot reference directly. + Convert.ChangeType(innerConnection, sqlConnectionInternalType); + }); + Assert.True( + ex is InvalidCastException, + $"Expected InvalidCastException but got {ex.GetType().Name}: {ex.Message}"); + } + + /// + /// Verifies that a SqlConnection in the Connecting state reports ConnectionState.Connecting, + /// which is an unexpected state for post-open code to encounter. + /// + [Fact] + public void InnerConnection_InConnectingState_ReportsConnectingState() + { + // Arrange + Type closedConnectingType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true); + FieldInfo singletonField = closedConnectingType + .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public); + object connectingSingleton = singletonField.GetValue(null); + + var connection = new SqlConnection("Data Source=localhost"); + FieldInfo innerConnectionField = typeof(SqlConnection) + .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic); + innerConnectionField.SetValue(connection, connectingSingleton); + + // Act & Assert: the state should be Connecting, not Open + // This confirms the connection is in a transitional state where + // TryOpenInner's cast would fail + Assert.Equal(ConnectionState.Connecting, connection.State); + } + + /// + /// Verifies that calling Open() on a SqlConnection that is already in the Connecting state + /// throws InvalidOperationException ("already open"), confirming that concurrent Open() + /// calls on the same instance are not supported. + /// + [Fact] + public void Open_WhenAlreadyConnecting_ThrowsInvalidOperation() + { + // Arrange: force `_innerConnection` to DbConnectionClosedConnecting + Type closedConnectingType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true); + FieldInfo singletonField = closedConnectingType + .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public); + object connectingSingleton = singletonField.GetValue(null); + + var connection = new SqlConnection("Data Source=localhost"); + FieldInfo innerConnectionField = typeof(SqlConnection) + .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic); + innerConnectionField.SetValue(connection, connectingSingleton); + + // Act & Assert: Open() while connecting should throw + // DbConnectionClosedConnecting.TryOpenConnection throws "connection already open" + // when retry is null (synchronous path) + Assert.Throws(() => connection.Open()); + } + } +} From 6d92d4977692d84e37def87148b981d16d8ae826 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 29 Apr 2026 12:52:36 -0300 Subject: [PATCH 2/4] Fix TryOpenInner race cast and move regression tests to UnitTests --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 21 ++- .../SqlConnectionTest.ConcurrentOpen.cs | 130 ------------------ .../SqlConnectionConcurrentOpenTests.cs | 128 +++++++++++++++++ 3 files changed, 144 insertions(+), 135 deletions(-) delete mode 100644 src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index b96e6f654d..885682123b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -266,7 +266,7 @@ private SqlConnection(SqlConnection connection) internal static bool TryGetSystemColumnEncryptionKeyStoreProvider(string keyStoreName, out SqlColumnEncryptionKeyStoreProvider provider) { - return s_systemColumnEncryptionKeyStoreProviders.TryGetValue(keyStoreName, out provider); + return s_systemColumnEncryptionKeyStoreProviders.TryGetValue(keyStoreName, out provider); } /// @@ -1332,7 +1332,7 @@ public override void ChangeDatabase(string database) SqlStatistics statistics = null; RepairInnerConnection(); SqlClientEventSource.Log.TryCorrelationTraceEvent("SqlConnection.ChangeDatabase | API | Correlation | Object Id {0}, Activity Id {1}, Database {2}", ObjectID, ActivityCorrelator.Current, database); - + try { statistics = SqlStatistics.StartTimer(Statistics); @@ -1408,7 +1408,7 @@ public override void Close() SqlStatistics statistics = null; Exception e = null; - + try { statistics = SqlStatistics.StartTimer(Statistics); @@ -1901,7 +1901,7 @@ internal void Abort(Exception e) } /// - public override Task OpenAsync(CancellationToken cancellationToken) + public override Task OpenAsync(CancellationToken cancellationToken) => OpenAsync(SqlConnectionOverrides.None, cancellationToken); /// @@ -2224,7 +2224,18 @@ private bool TryOpenInner(TaskCompletionSource retry) } // does not require GC.KeepAlive(this) because of ReRegisterForFinalize below. - var tdsInnerConnection = (SqlConnectionInternal)InnerConnection; + // Capture InnerConnection once into a local to avoid a TOCTOU race: another thread + // concurrently calling Open() on the same SqlConnection instance can change + // _innerConnection to DbConnectionClosedConnecting between the TryOpenConnection() + // call above and the cast below. Without this local capture the second read of + // InnerConnection may return DbConnectionClosedConnecting, which is not assignable + // to SqlConnectionInternal and would produce an opaque InvalidCastException. + // See GitHub issue #3314. + var innerConnection = InnerConnection; + if (innerConnection is not SqlConnectionInternal tdsInnerConnection) + { + throw ADP.ConnectionAlreadyOpen(State); + } Debug.Assert(tdsInnerConnection.Parser != null, "Where's the parser?"); diff --git a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs b/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs deleted file mode 100644 index 84841fae10..0000000000 --- a/src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.ConcurrentOpen.cs +++ /dev/null @@ -1,130 +0,0 @@ -// 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.Reflection; -using Xunit; - -namespace Microsoft.Data.SqlClient.Tests -{ - /// - /// Tests that confirm the race condition described in GitHub issue #3314: - /// If _innerConnection is set to DbConnectionClosedConnecting (a transitional state) - /// at the moment TryOpenInner casts it to SqlConnectionInternal, an InvalidCastException - /// is thrown. This simulates a concurrent Open() on the same SqlConnection instance. - /// - public partial class SqlConnectionTest - { - /// - /// Verifies that when _innerConnection holds DbConnectionClosedConnecting (a non-open - /// transitional state), casting it to SqlConnectionInternal throws InvalidCastException. - /// This is the exact exception observed in issue #3314. - /// - [Fact] - public void InnerConnection_CastToSqlConnectionInternal_ThrowsInvalidCast_WhenInConnectingState() - { - // Arrange: get the DbConnectionClosedConnecting singleton via reflection - Type closedConnectingType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true); - FieldInfo singletonField = closedConnectingType - .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public); - Assert.NotNull(singletonField); - object connectingSingleton = singletonField.GetValue(null); - Assert.NotNull(connectingSingleton); - - // Verify it is NOT SqlConnectionInternal — this is the root cause of the InvalidCastException - Type sqlConnectionInternalType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.SqlClient.Connection.SqlConnectionInternal", throwOnError: true); - Assert.False( - sqlConnectionInternalType.IsInstanceOfType(connectingSingleton), - "DbConnectionClosedConnecting must not be assignable to SqlConnectionInternal"); - - // Act: set a SqlConnection's _innerConnection to the Connecting state to simulate the race - var connection = new SqlConnection("Data Source=localhost"); - FieldInfo innerConnectionField = typeof(SqlConnection) - .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic); - Assert.NotNull(innerConnectionField); - - innerConnectionField.SetValue(connection, connectingSingleton); - - // Read it back through InnerConnection and attempt the same cast that TryOpenInner does - PropertyInfo innerConnectionProperty = typeof(SqlConnection) - .GetProperty("InnerConnection", BindingFlags.Instance | BindingFlags.NonPublic); - Assert.NotNull(innerConnectionProperty); - object innerConnection = innerConnectionProperty.GetValue(connection); - - // Assert: the runtime type is not assignable to SqlConnectionInternal - Assert.False( - sqlConnectionInternalType.IsAssignableFrom(innerConnection.GetType()), - $"Expected InnerConnection type '{innerConnection.GetType().FullName}' to NOT be assignable " + - $"to '{sqlConnectionInternalType.FullName}'. If it were, the cast in TryOpenInner would " + - "succeed and the race condition in issue #3314 would not manifest as InvalidCastException."); - - // Perform the exact cast that TryOpenInner does at SqlConnection.cs line 2228: - // var tdsInnerConnection = (SqlConnectionInternal)InnerConnection; - // This must throw InvalidCastException when _innerConnection is DbConnectionClosedConnecting. - Exception ex = Assert.ThrowsAny(() => - { - // Use Convert.ChangeType or direct cast via reflection to replicate - // the CLR's cast behavior for internal types we cannot reference directly. - Convert.ChangeType(innerConnection, sqlConnectionInternalType); - }); - Assert.True( - ex is InvalidCastException, - $"Expected InvalidCastException but got {ex.GetType().Name}: {ex.Message}"); - } - - /// - /// Verifies that a SqlConnection in the Connecting state reports ConnectionState.Connecting, - /// which is an unexpected state for post-open code to encounter. - /// - [Fact] - public void InnerConnection_InConnectingState_ReportsConnectingState() - { - // Arrange - Type closedConnectingType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true); - FieldInfo singletonField = closedConnectingType - .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public); - object connectingSingleton = singletonField.GetValue(null); - - var connection = new SqlConnection("Data Source=localhost"); - FieldInfo innerConnectionField = typeof(SqlConnection) - .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic); - innerConnectionField.SetValue(connection, connectingSingleton); - - // Act & Assert: the state should be Connecting, not Open - // This confirms the connection is in a transitional state where - // TryOpenInner's cast would fail - Assert.Equal(ConnectionState.Connecting, connection.State); - } - - /// - /// Verifies that calling Open() on a SqlConnection that is already in the Connecting state - /// throws InvalidOperationException ("already open"), confirming that concurrent Open() - /// calls on the same instance are not supported. - /// - [Fact] - public void Open_WhenAlreadyConnecting_ThrowsInvalidOperation() - { - // Arrange: force `_innerConnection` to DbConnectionClosedConnecting - Type closedConnectingType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true); - FieldInfo singletonField = closedConnectingType - .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public); - object connectingSingleton = singletonField.GetValue(null); - - var connection = new SqlConnection("Data Source=localhost"); - FieldInfo innerConnectionField = typeof(SqlConnection) - .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic); - innerConnectionField.SetValue(connection, connectingSingleton); - - // Act & Assert: Open() while connecting should throw - // DbConnectionClosedConnecting.TryOpenConnection throws "connection already open" - // when retry is null (synchronous path) - Assert.Throws(() => connection.Open()); - } - } -} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs new file mode 100644 index 0000000000..d98a331ae1 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs @@ -0,0 +1,128 @@ +// 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.Reflection; +using System.Threading.Tasks; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.Microsoft.Data.SqlClient +{ + /// + /// Regression tests for GitHub issue #3314. + /// + /// Root cause: TryOpenInner() read InnerConnection twice - once for TryOpenConnection() and + /// again for the cast to SqlConnectionInternal. Between those two reads another thread could + /// change _innerConnection to DbConnectionClosedConnecting, which is not assignable to + /// SqlConnectionInternal, causing an opaque InvalidCastException. + /// + /// Fix: InnerConnection is now captured into a local variable once; if it is not a + /// SqlConnectionInternal an InvalidOperationException with a descriptive message is thrown + /// instead of an InvalidCastException. + /// + public class SqlConnectionConcurrentOpenTests + { + private static object GetConnectingSingleton() + { + Type closedConnectingType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true)!; + FieldInfo singletonField = closedConnectingType + .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public)!; + object? singletonInstance = singletonField.GetValue(null); + Assert.NotNull(singletonInstance); + return singletonInstance!; + } + + private static void ForceInnerConnection(SqlConnection connection, object innerConnectionValue) + { + FieldInfo innerConnectionField = typeof(SqlConnection) + .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic)!; + innerConnectionField.SetValue(connection, innerConnectionValue); + } + + [Fact] + public void InnerConnection_DbConnectionClosedConnecting_IsNotAssignableToSqlConnectionInternal() + { + object connectingSingleton = GetConnectingSingleton(); + + Type sqlConnectionInternalType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.SqlClient.Connection.SqlConnectionInternal", throwOnError: true)!; + + Assert.False( + sqlConnectionInternalType.IsInstanceOfType(connectingSingleton), + "DbConnectionClosedConnecting must NOT be assignable to SqlConnectionInternal. " + + "If it were, the race condition in #3314 would not manifest."); + } + + [Fact] + public void InnerConnection_InConnectingState_ReportsConnectingState() + { + object connectingSingleton = GetConnectingSingleton(); + + var connection = new SqlConnection("Data Source=localhost"); + ForceInnerConnection(connection, connectingSingleton); + + Assert.Equal(ConnectionState.Connecting, connection.State); + } + + [Fact] + public void Open_WhenAlreadyConnecting_ThrowsInvalidOperation() + { + object connectingSingleton = GetConnectingSingleton(); + + var connection = new SqlConnection("Data Source=localhost"); + ForceInnerConnection(connection, connectingSingleton); + + Assert.Throws(() => connection.Open()); + } + + [Fact] + public void TryOpenInner_WhenInnerConnectionRacesToConnectingState_ThrowsInvalidOperation_NotInvalidCast() + { + object connectingSingleton = GetConnectingSingleton(); + + Type openBusyType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.ProviderBase.DbConnectionOpenBusy", throwOnError: true)!; + FieldInfo openBusySingleton = openBusyType + .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public)!; + object? openBusyInstance = openBusySingleton.GetValue(null); + Assert.NotNull(openBusyInstance); + + var connection = new SqlConnection("Data Source=localhost"); + ForceInnerConnection(connection, connectingSingleton); + + Type dbConnectionInternalType = typeof(SqlConnection).Assembly + .GetType("Microsoft.Data.ProviderBase.DbConnectionInternal", throwOnError: true)!; + Type tcsType = typeof(TaskCompletionSource<>).MakeGenericType(dbConnectionInternalType); + object completedRetry = Activator.CreateInstance(tcsType)!; + MethodInfo setResultMethod = tcsType.GetMethod("SetResult")!; + setResultMethod.Invoke(completedRetry, new[] { openBusyInstance! }); + + MethodInfo tryOpenInner = typeof(SqlConnection) + .GetMethod("TryOpenInner", BindingFlags.Instance | BindingFlags.NonPublic)!; + Assert.NotNull(tryOpenInner); + + Exception ex = Assert.ThrowsAny(() => + { + try + { + tryOpenInner.Invoke(connection, new[] { completedRetry }); + } + catch (TargetInvocationException tie) when (tie.InnerException != null) + { + throw tie.InnerException; + } + }); + + Assert.True( + ex is InvalidOperationException, + $"Expected InvalidOperationException but got {ex.GetType().Name}: {ex.Message}. " + + "The fix for #3314 must throw InvalidOperationException (not InvalidCastException) " + + "when _innerConnection races to a non-SqlConnectionInternal state inside TryOpenInner."); + + Assert.Contains("connection", ex.Message, StringComparison.OrdinalIgnoreCase); + } + } +} \ No newline at end of file From 588706a29c43ca8c73213cde2f2f39730996927b Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 29 Apr 2026 13:01:16 -0300 Subject: [PATCH 3/4] Refactor concurrent-open unit tests to use internals directly --- .../SqlConnectionConcurrentOpenTests.cs | 75 ++++++++----------- 1 file changed, 30 insertions(+), 45 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs index d98a331ae1..06cb559e02 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs @@ -6,6 +6,8 @@ using System.Data; using System.Reflection; using System.Threading.Tasks; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.Connection; using Xunit; namespace Microsoft.Data.SqlClient.UnitTests.Microsoft.Data.SqlClient @@ -24,34 +26,38 @@ namespace Microsoft.Data.SqlClient.UnitTests.Microsoft.Data.SqlClient /// public class SqlConnectionConcurrentOpenTests { - private static object GetConnectingSingleton() + private static readonly MethodInfo s_tryOpenInner = typeof(SqlConnection) + .GetMethod("TryOpenInner", BindingFlags.Instance | BindingFlags.NonPublic)!; + + private static DbConnectionInternal GetConnectingSingleton() { - Type closedConnectingType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.ProviderBase.DbConnectionClosedConnecting", throwOnError: true)!; - FieldInfo singletonField = closedConnectingType - .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public)!; - object? singletonInstance = singletonField.GetValue(null); - Assert.NotNull(singletonInstance); - return singletonInstance!; + return DbConnectionClosedConnecting.SingletonInstance; } - private static void ForceInnerConnection(SqlConnection connection, object innerConnectionValue) + private static void ForceInnerConnection(SqlConnection connection, DbConnectionInternal innerConnectionValue) { - FieldInfo innerConnectionField = typeof(SqlConnection) - .GetField("_innerConnection", BindingFlags.Instance | BindingFlags.NonPublic)!; - innerConnectionField.SetValue(connection, innerConnectionValue); + connection.SetInnerConnectionTo(innerConnectionValue); + } + + private static bool InvokeTryOpenInner(SqlConnection connection, TaskCompletionSource retry) + { + try + { + return (bool)s_tryOpenInner.Invoke(connection, [retry])!; + } + catch (TargetInvocationException tie) when (tie.InnerException != null) + { + throw tie.InnerException; + } } [Fact] public void InnerConnection_DbConnectionClosedConnecting_IsNotAssignableToSqlConnectionInternal() { - object connectingSingleton = GetConnectingSingleton(); - - Type sqlConnectionInternalType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.SqlClient.Connection.SqlConnectionInternal", throwOnError: true)!; + DbConnectionInternal connectingSingleton = GetConnectingSingleton(); Assert.False( - sqlConnectionInternalType.IsInstanceOfType(connectingSingleton), + connectingSingleton is SqlConnectionInternal, "DbConnectionClosedConnecting must NOT be assignable to SqlConnectionInternal. " + "If it were, the race condition in #3314 would not manifest."); } @@ -59,7 +65,7 @@ public void InnerConnection_DbConnectionClosedConnecting_IsNotAssignableToSqlCon [Fact] public void InnerConnection_InConnectingState_ReportsConnectingState() { - object connectingSingleton = GetConnectingSingleton(); + DbConnectionInternal connectingSingleton = GetConnectingSingleton(); var connection = new SqlConnection("Data Source=localhost"); ForceInnerConnection(connection, connectingSingleton); @@ -70,7 +76,7 @@ public void InnerConnection_InConnectingState_ReportsConnectingState() [Fact] public void Open_WhenAlreadyConnecting_ThrowsInvalidOperation() { - object connectingSingleton = GetConnectingSingleton(); + DbConnectionInternal connectingSingleton = GetConnectingSingleton(); var connection = new SqlConnection("Data Source=localhost"); ForceInnerConnection(connection, connectingSingleton); @@ -81,39 +87,18 @@ public void Open_WhenAlreadyConnecting_ThrowsInvalidOperation() [Fact] public void TryOpenInner_WhenInnerConnectionRacesToConnectingState_ThrowsInvalidOperation_NotInvalidCast() { - object connectingSingleton = GetConnectingSingleton(); - - Type openBusyType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.ProviderBase.DbConnectionOpenBusy", throwOnError: true)!; - FieldInfo openBusySingleton = openBusyType - .GetField("SingletonInstance", BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public)!; - object? openBusyInstance = openBusySingleton.GetValue(null); - Assert.NotNull(openBusyInstance); + DbConnectionInternal connectingSingleton = GetConnectingSingleton(); + DbConnectionInternal openBusyInstance = DbConnectionOpenBusy.SingletonInstance; var connection = new SqlConnection("Data Source=localhost"); ForceInnerConnection(connection, connectingSingleton); - Type dbConnectionInternalType = typeof(SqlConnection).Assembly - .GetType("Microsoft.Data.ProviderBase.DbConnectionInternal", throwOnError: true)!; - Type tcsType = typeof(TaskCompletionSource<>).MakeGenericType(dbConnectionInternalType); - object completedRetry = Activator.CreateInstance(tcsType)!; - MethodInfo setResultMethod = tcsType.GetMethod("SetResult")!; - setResultMethod.Invoke(completedRetry, new[] { openBusyInstance! }); - - MethodInfo tryOpenInner = typeof(SqlConnection) - .GetMethod("TryOpenInner", BindingFlags.Instance | BindingFlags.NonPublic)!; - Assert.NotNull(tryOpenInner); + TaskCompletionSource completedRetry = new(); + completedRetry.SetResult(openBusyInstance); Exception ex = Assert.ThrowsAny(() => { - try - { - tryOpenInner.Invoke(connection, new[] { completedRetry }); - } - catch (TargetInvocationException tie) when (tie.InnerException != null) - { - throw tie.InnerException; - } + InvokeTryOpenInner(connection, completedRetry); }); Assert.True( From b07762815df38f668842b3f4eecb2b9e20be18b7 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 29 Apr 2026 15:10:20 -0300 Subject: [PATCH 4/4] Fix test naming clarity and exception snapshot consistency per Copilot review - Rename test to TryOpenInner_WhenInnerConnectionRacesToNonSqlConnectionInternalState_* to accurately reflect that the test exercises a non-SqlConnectionInternal state (DbConnectionOpenBusy), not a connecting state. - Rename local variables connectingSingleton -> initialConnectingState and openBusyInstance -> racedNonSqlConnectionInternalState for clarity. - Use captured innerConnection.State in exception path instead of re-reading State via property, ensuring the thrown message reflects the same snapshot as the type check (avoids concurrent read under contention). Tests: all 4 concurrent open unit tests pass on net9.0, net10.0. --- .../src/Microsoft/Data/SqlClient/SqlConnection.cs | 2 +- .../Data/SqlClient/SqlConnectionConcurrentOpenTests.cs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 885682123b..e0490ebdf5 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2234,7 +2234,7 @@ private bool TryOpenInner(TaskCompletionSource retry) var innerConnection = InnerConnection; if (innerConnection is not SqlConnectionInternal tdsInnerConnection) { - throw ADP.ConnectionAlreadyOpen(State); + throw ADP.ConnectionAlreadyOpen(innerConnection.State); } Debug.Assert(tdsInnerConnection.Parser != null, "Where's the parser?"); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs index 06cb559e02..c38a30d317 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs @@ -85,16 +85,16 @@ public void Open_WhenAlreadyConnecting_ThrowsInvalidOperation() } [Fact] - public void TryOpenInner_WhenInnerConnectionRacesToConnectingState_ThrowsInvalidOperation_NotInvalidCast() + public void TryOpenInner_WhenInnerConnectionRacesToNonSqlConnectionInternalState_ThrowsInvalidOperation_NotInvalidCast() { - DbConnectionInternal connectingSingleton = GetConnectingSingleton(); - DbConnectionInternal openBusyInstance = DbConnectionOpenBusy.SingletonInstance; + DbConnectionInternal initialConnectingState = GetConnectingSingleton(); + DbConnectionInternal racedNonSqlConnectionInternalState = DbConnectionOpenBusy.SingletonInstance; var connection = new SqlConnection("Data Source=localhost"); - ForceInnerConnection(connection, connectingSingleton); + ForceInnerConnection(connection, initialConnectingState); TaskCompletionSource completedRetry = new(); - completedRetry.SetResult(openBusyInstance); + completedRetry.SetResult(racedNonSqlConnectionInternalState); Exception ex = Assert.ThrowsAny(() => {