Add tests confirming InvalidCastException race condition in TryOpenInner (#3314)#4179
Add tests confirming InvalidCastException race condition in TryOpenInner (#3314)#4179paulmedynski wants to merge 1 commit intomainfrom
Conversation
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
There was a problem hiding this comment.
Pull request overview
Adds new FunctionalTests that reproduce/confirm the InvalidCastException scenario described in #3314 by forcing SqlConnection’s internal _innerConnection into the DbConnectionClosedConnecting transitional state and validating resulting behavior.
Changes:
- Added reflection-based tests validating that the
DbConnectionClosedConnectingsingleton is not assignable toSqlConnectionInternal. - Added a test confirming
SqlConnection.StatereportsConnectionState.Connectingwhen_innerConnectionis the connecting singleton. - Added a test confirming
Open()throwsInvalidOperationExceptionwhen_innerConnectionis already in the connecting state.
| Exception ex = Assert.ThrowsAny<Exception>(() => | ||
| { | ||
| // 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); | ||
| }); |
There was a problem hiding this comment.
Convert.ChangeType(innerConnection, sqlConnectionInternalType) does not replicate the explicit CLR cast used in TryOpenInner (it uses IConvertible semantics and can throw InvalidCastException even when the runtime type is assignable). This makes the test misleading and potentially a false-positive. Consider generating an actual castclass (e.g., via Expression.Convert + compiled lambda or a small DynamicMethod) and then using Assert.Throws<InvalidCastException> directly.
| 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); |
There was a problem hiding this comment.
This test assumes reflection lookups always succeed, but it doesn't assert singletonField, connectingSingleton, or innerConnectionField are non-null before dereferencing/using them. If internal names change, this will fail with a NullReferenceException instead of a clear assertion failure; please add explicit Assert.NotNull(...) checks (as done in the first test).
| 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); |
There was a problem hiding this comment.
Same as above: reflection results (singletonField, connectingSingleton, innerConnectionField) are used without null assertions. Adding Assert.NotNull(...) checks will make failures deterministic and easier to diagnose if internal member names move/are trimmed.
Summary
This` PR is currently a proof-of-concept. It should be updated to fix the actual bug before being promoted to Open.
Adds functional tests that reproduce and confirm the root cause of #3314.
When
_innerConnectionis in theDbConnectionClosedConnectingtransitional state at the momentTryOpenInnercasts it toSqlConnectionInternal, anInvalidCastExceptionis thrown. This happens when aSqlConnectionis used concurrently from multiple threads (violating its thread-safety contract).Root Cause
In
SqlConnection.TryOpenInner()(SqlConnection.cs L2228):InnerConnectionis read a second time (unsynchronized) afterTryOpenConnection()has already returned. Between these two reads, another thread can change_innerConnectiontoDbConnectionClosedConnecting— a singleton state marker that is not assignable toSqlConnectionInternal.Tests Added
InnerConnection_CastToSqlConnectionInternal_ThrowsInvalidCast_WhenInConnectingStateDbConnectionClosedConnecting→SqlConnectionInternalcast throwsInvalidCastExceptionInnerConnection_InConnectingState_ReportsConnectingStateConnectingstate when in this transitional stateOpen_WhenAlreadyConnecting_ThrowsInvalidOperationOpen()on a connecting connection throwsInvalidOperationExceptionAll tests run without a SQL Server instance (functional tests using reflection).
Fixes #3314