From 3638a991687c3b55e915441dab88d9ce44fd2b77 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 2 Jul 2026 09:55:16 -0300 Subject: [PATCH 01/10] Add SNIClose deadlock regression tests (ADO #43847 / ICM 775308542) Adds gated-server repro tests that close/dispose a connection while an async network read is in flight, using the in-proc TDS server with a query engine that stalls the response. Findings: - On managed SNI (Linux) these tests PASS: disposing the socket completes the pending ReadAsync with ObjectDisposedException, so there is no deadlock in the managed teardown path. - The ICM deadlock is specific to the native SNIClose path on Windows, which cannot be exercised from a Linux host. These tests are intended to reproduce that hang on Windows/native SNI and serve as bounded-wait regression guards elsewhere. No fix applied yet. --- .../SNICloseDeadlockTest.cs | 189 ++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs new file mode 100644 index 0000000000..d8aabcbbcd --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs @@ -0,0 +1,189 @@ +// 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.Threading; +using System.Threading.Tasks; +using Microsoft.SqlServer.TDS; +using Microsoft.SqlServer.TDS.EndPoint; +using Microsoft.SqlServer.TDS.Servers; +using Microsoft.SqlServer.TDS.SQLBatch; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; + +/// +/// Regression tests for ADO.Net work item 43847 / ICM 775308542: +/// "SNIClose can deadlock with in-flight async I/O during connection close". +/// +/// +/// These tests reproduce the ordering hazard where a connection is closed or +/// disposed while an asynchronous network read is still pending. The close +/// path must drain (or cancel) the in-flight async I/O before releasing the +/// SNI handle; if it releases the handle first, the closing thread and the +/// pending completion callback can wait on each other and deadlock. +/// +/// +/// +/// Each test uses an in-process TDS server whose query engine deliberately +/// stalls after receiving the client's SQL batch. This guarantees that the +/// client's async read for the response is genuinely in flight at the moment +/// the connection is closed. The close is performed on a worker thread and +/// wrapped in a bounded wait: a deadlock manifests as the wait timing out, +/// which fails the test. After the fix, the close completes promptly. +/// +/// +/// +/// Expected state: these tests are expected to FAIL (time out) against +/// the current, unfixed close path and PASS once the drain-before-close fix is +/// implemented. +/// +/// +public class SNICloseDeadlockTest +{ + /// + /// Upper bound for how long a non-deadlocked close should take. Close of a + /// healthy connection is effectively instantaneous; this generous budget + /// exists only to distinguish "completed" from "deadlocked" on slow CI + /// agents without hanging the whole test run. + /// + private static readonly TimeSpan CloseBudget = TimeSpan.FromSeconds(30); + + /// + /// Upper bound for waiting on the server to receive the client's batch, + /// i.e. for the client's async read to become pending. + /// + private static readonly TimeSpan HandshakeBudget = TimeSpan.FromSeconds(30); + + /// + /// A query engine that signals when a SQL batch has arrived and then blocks + /// (withholding the response) until the test releases it. While it is + /// blocked, the client's async read for the response remains in flight. + /// + private sealed class StallingQueryEngine : QueryEngine + { + private readonly ManualResetEventSlim _batchReceived; + private readonly ManualResetEventSlim _releaseResponse; + + public StallingQueryEngine( + TdsServerArguments arguments, + ManualResetEventSlim batchReceived, + ManualResetEventSlim releaseResponse) + : base(arguments) + { + _batchReceived = batchReceived; + _releaseResponse = releaseResponse; + } + + protected override TDSMessageCollection CreateQueryResponse( + ITDSServerSession session, + TDSSQLBatchToken batchRequest) + { + // The client has sent the batch and posted an async receive for the + // response by the time we get here. Signal the test, then stall so + // the client's read stays in flight while the connection is closed. + _batchReceived.Set(); + _releaseResponse.Wait(); + return base.CreateQueryResponse(session, batchRequest); + } + } + + [Fact] + public void CloseConnection_WithPendingAsyncRead_DoesNotDeadlock() + { + RunPendingAsyncReadCloseScenario(disposeInsteadOfClose: false); + } + + [Fact] + public void DisposeConnection_WithPendingAsyncRead_DoesNotDeadlock() + { + RunPendingAsyncReadCloseScenario(disposeInsteadOfClose: true); + } + + private static void RunPendingAsyncReadCloseScenario(bool disposeInsteadOfClose) + { + using ManualResetEventSlim batchReceived = new(false); + using ManualResetEventSlim releaseResponse = new(false); + + TdsServerArguments arguments = new(); + using TdsServer server = new( + new StallingQueryEngine(arguments, batchReceived, releaseResponse), + arguments); + server.Start(); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"localhost,{server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + // Disable pooling so Close()/Dispose() tears down the physical + // connection (and reaches SNIClose) instead of returning it to the + // pool. + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + connection.Open(); + + SqlCommand command = new("SELECT 1", connection); + + // Start an async read that will pend: the server stalls in + // CreateQueryResponse, so this Task will not complete until we either + // release the server or tear down the connection. + Task readTask = command.ExecuteReaderAsync(); + + // Ensure the async read is genuinely in flight before we close. + Assert.True( + batchReceived.Wait(HandshakeBudget), + "The server never received the SQL batch, so the async read was " + + "not in flight; the test cannot exercise the close-with-pending-IO path."); + + // Close/dispose the connection on a worker thread while the async read + // is in flight. This is the operation that can deadlock in SNIClose. + Task closeTask = Task.Run(() => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }); + + bool closedInTime = closeTask.Wait(CloseBudget); + + // Always release the stalled server thread so it can unwind and the + // server can be disposed cleanly, regardless of the outcome above. + releaseResponse.Set(); + + // Observe the read task so its (expected) failure is not unobserved. + try + { + readTask.Wait(CloseBudget); + } + catch + { + // The pending read is expected to fault or cancel once the + // connection is torn down. That is not what this test asserts. + } + finally + { + command.Dispose(); + if (!disposeInsteadOfClose) + { + connection.Dispose(); + } + } + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + + $"within {CloseBudget.TotalSeconds:N0}s while an async read was in flight. " + + "This indicates the SNIClose deadlock (ADO.Net #43847 / ICM 775308542)."); + } +} From 9f8f7de8599fa4e791a82d7c8e4f0ed9a47cd4a6 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:00:48 -0300 Subject: [PATCH 02/10] Add SNIClose close-during-IO regression tests (pre-login, TLS handshake, MARS) Expands the ADO #43847 / ICM 775308542 investigation into whether SNIClose can deadlock with in-flight async I/O during connection close. UnitTests (SNICloseDeadlockTest.cs), in-process TDS server: - Strengthen the existing post-login query-read scenarios to assert the async read is genuinely pending at close (readTask not completed, connection Open). - Add pre-login stall scenarios (bare TcpListener withholds the pre-login response) for Close and Dispose. - Add faithful TLS-over-TDS handshake scenarios: the listener advertises ENCRYPT_ON, reads the client's ClientHello, then withholds the ServerHello so the client is inside SslStream.AuthenticateAsClient with a pending SNI read at close. ManualTests (MarsCloseDeadlockTest.cs), live SQL Server: - Add MARS (smux logical-session) Close/Dispose scenarios using WAITFOR DELAY to keep an async read pending; the in-process TDS server does not support MARS. All scenarios pass on net9.0 and net462: current native SNI drains the pending async I/O and closes promptly in every path (post-login, pre-login, TLS handshake, and MARS). --- .../SQL/AsyncTest/MarsCloseDeadlockTest.cs | 136 +++++++ .../SNICloseDeadlockTest.cs | 361 ++++++++++++++++++ 2 files changed, 497 insertions(+) create mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs new file mode 100644 index 0000000000..5284d9a689 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs @@ -0,0 +1,136 @@ +// 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.Threading.Tasks; +using Xunit; + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + /// + /// Live-server regression test for ADO.Net work item 43847 / ICM 775308542: + /// "SNIClose can deadlock with in-flight async I/O during connection close". + /// + /// + /// This is the MARS-enabled counterpart to the in-process + /// SNICloseDeadlockTest unit tests. The native Dispose() path + /// in TdsParserStateObjectNative carries a long-standing "UNDONE" + /// note about needing to block for pending callbacks on logical + /// (MARS) connections during close. The in-process TDS test server does not + /// support MARS, so exercising the SMUX logical-session close path requires + /// a real SQL Server. + /// + /// + /// + /// The test enables MARS and starts an asynchronous read of a batch that the + /// server deliberately withholds (WAITFOR DELAY). This guarantees the + /// client's async read is genuinely in flight on a MARS logical session at + /// the moment the connection is closed. The close is performed on a worker + /// thread under a bounded wait: a deadlock manifests as the wait timing out, + /// which fails the test. A healthy close aborts the in-flight command and + /// returns promptly. + /// + /// + [Trait("Set", "1")] + public class MarsCloseDeadlockTest + { + /// + /// How long the server withholds the response. Must comfortably exceed + /// so that a prompt close is attributable to + /// the close path aborting the command, not to the query completing on + /// its own. + /// + private const string StallDelay = "00:00:30"; + + /// + /// Upper bound for how long a non-deadlocked close should take. Close of + /// a connection with an in-flight command is effectively instantaneous; + /// this budget only distinguishes "completed" from "deadlocked" without + /// hanging the run on a slow agent. + /// + private static readonly TimeSpan CloseBudget = TimeSpan.FromSeconds(15); + + /// + /// How long to wait to confirm the async read is genuinely pending (not + /// completed) before closing. Small relative to . + /// + private static readonly TimeSpan PendingConfirmDelay = TimeSpan.FromSeconds(3); + + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + [InlineData(false)] + [InlineData(true)] + public void CloseOrDispose_WithPendingMarsAsyncRead_DoesNotDeadlock(bool disposeInsteadOfClose) + { + // Enable MARS so the async read runs over a SMUX logical session, and + // disable pooling so Close()/Dispose() tears down the physical + // connection (reaching SNIClose) instead of returning it to the pool. + string connectionString = + new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + MultipleActiveResultSets = true, + Pooling = false, + }.ConnectionString; + + SqlConnection connection = new(connectionString); + connection.Open(); + + SqlCommand command = + new($"WAITFOR DELAY '{StallDelay}'; SELECT 1;", connection); + + // Start an async read that will pend: the server withholds the + // response for StallDelay, so this Task will not complete until we + // either wait out the delay or tear down the connection. + Task readTask = command.ExecuteReaderAsync(); + + // Confirm the read is genuinely in flight before we close: it must + // not have completed and the connection must still be open. + Assert.False( + readTask.Wait(PendingConfirmDelay), + "The async read completed before the server released the " + + "response; the read was not in flight at close time."); + Assert.Equal(ConnectionState.Open, connection.State); + + // Close/dispose the connection on a worker thread while the MARS + // async read is in flight. This is the operation that can deadlock + // in SNIClose. + Task closeTask = Task.Run(() => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }); + + bool closedInTime = closeTask.Wait(CloseBudget); + + // Observe the read task so its (expected) failure is not unobserved. + try + { + readTask.Wait(CloseBudget); + } + catch + { + // The pending read is expected to fault or cancel once the + // connection is torn down. That is not what this test asserts. + } + finally + { + command.Dispose(); + connection.Dispose(); + } + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not " + + $"complete within {CloseBudget.TotalSeconds:N0}s while a MARS " + + "async read was in flight. This indicates the SNIClose deadlock " + + "(ADO.Net #43847 / ICM 775308542)."); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs index d8aabcbbcd..e882fd27b1 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System; +using System.Net; +using System.Net.Sockets; using System.Threading; using System.Threading.Tasks; using Microsoft.SqlServer.TDS; @@ -141,6 +143,17 @@ private static void RunPendingAsyncReadCloseScenario(bool disposeInsteadOfClose) "The server never received the SQL batch, so the async read was " + "not in flight; the test cannot exercise the close-with-pending-IO path."); + // The server is stalled withholding the response, so the client's async + // read must still be pending: the Task cannot have completed and the + // connection must still be open. If either of these is false, the read + // is not actually in flight and the test is not exercising the + // close-with-pending-IO path. + Assert.False( + readTask.IsCompleted, + "The async read completed before the server released the response; " + + "the read was not in flight at close time."); + Assert.Equal(System.Data.ConnectionState.Open, connection.State); + // Close/dispose the connection on a worker thread while the async read // is in flight. This is the operation that can deadlock in SNIClose. Task closeTask = Task.Run(() => @@ -186,4 +199,352 @@ private static void RunPendingAsyncReadCloseScenario(bool disposeInsteadOfClose) $"within {CloseBudget.TotalSeconds:N0}s while an async read was in flight. " + "This indicates the SNIClose deadlock (ADO.Net #43847 / ICM 775308542)."); } + + [Fact] + public void CloseConnection_DuringPreLoginHandshake_DoesNotDeadlock() + { + RunPendingHandshakeCloseScenario(disposeInsteadOfClose: false); + } + + [Fact] + public void DisposeConnection_DuringPreLoginHandshake_DoesNotDeadlock() + { + RunPendingHandshakeCloseScenario(disposeInsteadOfClose: true); + } + + /// + /// Reproduces the ICM scenario more faithfully: the connection is torn down + /// while it is still in the middle of connection establishment (the + /// pre-login / TLS handshake), rather than after a successful login. + /// + /// + /// A bare TCP listener accepts the socket and reads the client's pre-login + /// packet but deliberately never sends a response. This leaves the client's + /// async read for the pre-login/handshake response in flight. The close is + /// performed on a worker thread under a bounded wait; a deadlock in the + /// handshake-phase close path manifests as the wait timing out. + /// + /// + private static void RunPendingHandshakeCloseScenario(bool disposeInsteadOfClose) + { + using ManualResetEventSlim clientConnected = new(false); + using ManualResetEventSlim releaseServer = new(false); + + TcpListener listener = new(IPAddress.Loopback, 0); + listener.Start(); + int port = ((IPEndPoint)listener.LocalEndpoint).Port; + + // Server: accept the socket, read the client's pre-login bytes, then + // withhold any response so the client's handshake read stays pending. + Task serverTask = Task.Run(() => + { + using TcpClient acceptedClient = listener.AcceptTcpClient(); + using NetworkStream stream = acceptedClient.GetStream(); + + try + { + // Read one byte of the client's pre-login packet. Once this + // returns the client has sent its pre-login and posted its + // async read for the response. We only need to know data + // arrived, so a single byte is sufficient. + stream.ReadByte(); + } + catch + { + // The client may tear down the socket; ignore. + } + + clientConnected.Set(); + + // Hold the socket open (withholding the response) until the test + // releases us, so the client's read cannot complete naturally. + releaseServer.Wait(); + }); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"127.0.0.1,{port}", + // Force TLS negotiation intent during the handshake, matching the + // ICM scenario. + Encrypt = SqlConnectionEncryptOption.Mandatory, + TrustServerCertificate = true, + // Long timeout so the connect attempt does not abort on its own + // before we get a chance to close it. + ConnectTimeout = 60, + ConnectRetryCount = 0, + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + + // Begin connection establishment. This will not complete because the + // server never answers the pre-login handshake. + Task openTask = connection.OpenAsync(); + + // Wait until the server has received the client's pre-login, i.e. the + // client's handshake read is in flight. + Assert.True( + clientConnected.Wait(HandshakeBudget), + "The server never received the client's pre-login packet, so the " + + "handshake read was not in flight; the test cannot exercise the " + + "close-during-handshake path."); + + // Close/dispose the connection on a worker thread while the handshake + // read is in flight. This is the operation that can deadlock. + Task closeTask = Task.Run(() => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }); + + bool closedInTime = closeTask.Wait(CloseBudget); + + // Release the server thread so it can unwind cleanly regardless of the + // outcome above. + releaseServer.Set(); + + // Observe the open task so its (expected) failure is not unobserved. + try + { + openTask.Wait(CloseBudget); + } + catch + { + // OpenAsync is expected to fault or cancel once the connection is + // torn down. That is not what this test asserts. + } + finally + { + if (!disposeInsteadOfClose) + { + connection.Dispose(); + } + + try + { + serverTask.Wait(CloseBudget); + } + catch + { + // Ignore server teardown faults. + } + + listener.Stop(); + } + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + + $"within {CloseBudget.TotalSeconds:N0}s while a pre-login response " + + "read was in flight. This indicates the SNIClose deadlock " + + "(ADO.Net #43847 / ICM 775308542)."); + } + + /// + /// A minimal, protocol-valid TDS PRELOGIN response advertising ENCRYPT_ON + /// so that a client which requested encryption proceeds into the TLS + /// handshake. Offsets are relative to the start of the payload (immediately + /// after the 8-byte TDS packet header): + /// + /// + /// Option table: + /// VERSION token 0x00, offset 11 (0x000B), length 6 + /// ENCRYPTION token 0x01, offset 17 (0x0011), length 1 + /// TERMINATOR 0xFF + /// Data: + /// VERSION 6 bytes (17.0.0.0) + /// ENCRYPTION 1 byte (0x01 = ENCRYPT_ON) + /// + /// + private static readonly byte[] s_preLoginEncryptOnResponse = + { + // ---- TDS packet header (8 bytes) ---- + 0x12, // Type: PRELOGIN + 0x01, // Status: EOM + 0x00, 0x1A, // Length: 26 (8 header + 18 payload), big-endian + 0x00, 0x00, // SPID + 0x01, // PacketID + 0x00, // Window + // ---- PRELOGIN option table ---- + 0x00, 0x00, 0x0B, 0x00, 0x06, // VERSION: offset 11, length 6 + 0x01, 0x00, 0x11, 0x00, 0x01, // ENCRYPTION: offset 17, length 1 + 0xFF, // TERMINATOR + // ---- Option data ---- + 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, // VERSION 17.0.0.0 + 0x01, // ENCRYPTION = ENCRYPT_ON + }; + + [Fact] + public void CloseConnection_DuringTlsHandshake_DoesNotDeadlock() + { + RunPendingTlsHandshakeCloseScenario(disposeInsteadOfClose: false); + } + + [Fact] + public void DisposeConnection_DuringTlsHandshake_DoesNotDeadlock() + { + RunPendingTlsHandshakeCloseScenario(disposeInsteadOfClose: true); + } + + /// + /// Reproduces the ICM scenario faithfully: the connection is torn down while + /// the client is inside the TLS handshake over TDS, with an SNI read pending + /// for the server's handshake response. + /// + /// + /// A bare TCP listener speaks just enough of the TDS pre-login exchange to + /// drive the client into the TLS handshake: it reads the client's PRELOGIN + /// packet, replies with a PRELOGIN response advertising ENCRYPT_ON, then + /// reads the client's TLS ClientHello (delivered as a TDS pre-login packet) + /// and deliberately never sends a ServerHello. At that point the client is + /// blocked inside SslStream.AuthenticateAsClient with an SNI read in + /// flight on the SSL-over-TDS transport. The close is performed on a worker + /// thread under a bounded wait; a deadlock in the handshake-phase close path + /// manifests as the wait timing out. + /// + /// + private static void RunPendingTlsHandshakeCloseScenario(bool disposeInsteadOfClose) + { + using ManualResetEventSlim handshakeInFlight = new(false); + using ManualResetEventSlim releaseServer = new(false); + + TcpListener listener = new(IPAddress.Loopback, 0); + listener.Start(); + int port = ((IPEndPoint)listener.LocalEndpoint).Port; + + // Server: complete the pre-login exchange advertising encryption, read + // the client's TLS ClientHello, then withhold the ServerHello so the + // client's handshake read stays pending. + Task serverTask = Task.Run(() => + { + using TcpClient acceptedClient = listener.AcceptTcpClient(); + using NetworkStream stream = acceptedClient.GetStream(); + + byte[] buffer = new byte[4096]; + try + { + // Read the client's PRELOGIN packet. + int read = stream.Read(buffer, 0, buffer.Length); + if (read <= 0) + { + return; + } + + // Advertise ENCRYPT_ON so the client begins the TLS handshake. + stream.Write(s_preLoginEncryptOnResponse, 0, s_preLoginEncryptOnResponse.Length); + stream.Flush(); + + // Read the first byte of the client's TLS ClientHello (wrapped + // in a TDS pre-login packet). A byte here proves the client + // accepted the pre-login response and entered the TLS handshake; + // it is now awaiting the ServerHello with its SNI read pending. + if (stream.ReadByte() < 0) + { + // Client tore down without sending a ClientHello: the + // handshake was never actually in flight. + return; + } + + handshakeInFlight.Set(); + + // Withhold the ServerHello until the test releases us. + releaseServer.Wait(); + } + catch + { + // The client may tear down the socket mid-handshake; ignore. + } + }); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"127.0.0.1,{port}", + // Require encryption so the client performs the TLS handshake. + Encrypt = SqlConnectionEncryptOption.Mandatory, + TrustServerCertificate = true, + ConnectTimeout = 60, + ConnectRetryCount = 0, + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + + // Begin connection establishment. This will not complete because the + // server never finishes the TLS handshake. + Task openTask = connection.OpenAsync(); + + // Wait until the client is inside the TLS handshake with a pending read. + Assert.True( + handshakeInFlight.Wait(HandshakeBudget), + "The client never reached the TLS handshake, so no SNI read was in " + + "flight; the test cannot exercise the close-during-TLS-handshake path."); + + // Close/dispose the connection on a worker thread while the TLS + // handshake read is in flight. This is the operation that can deadlock. + Task closeTask = Task.Run(() => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }); + + bool closedInTime = closeTask.Wait(CloseBudget); + + // Release the server thread so it can unwind cleanly regardless of the + // outcome above. + releaseServer.Set(); + + // Observe the open task so its (expected) failure is not unobserved. + try + { + openTask.Wait(CloseBudget); + } + catch + { + // OpenAsync is expected to fault or cancel once the connection is + // torn down. That is not what this test asserts. + } + finally + { + if (!disposeInsteadOfClose) + { + connection.Dispose(); + } + + try + { + serverTask.Wait(CloseBudget); + } + catch + { + // Ignore server teardown faults. + } + + listener.Stop(); + } + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + + $"within {CloseBudget.TotalSeconds:N0}s while a TLS handshake read was " + + "in flight. This indicates the SNIClose deadlock " + + "(ADO.Net #43847 / ICM 775308542)."); + } } From ad5297eaa92c8860e809c8314fd73a99d21c4e34 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:42:21 -0300 Subject: [PATCH 03/10] Address PR #4420 review: bound cleanup on deadlock + correct regression-test docs Resolves copilot-pull-request-reviewer feedback on #4420: - Guard the finally-block connection.Dispose() cleanup with closedInTime in all four scenarios (post-login read, pre-login, TLS handshake, MARS). When a close deadlock is detected (closedInTime == false), skip the potentially-blocking Dispose() so the bounded-wait Assert fails fast instead of hanging. - Correct the SNICloseDeadlockTest class doc: these are regression guards that PASS on the current code base (MDS drains in-flight async I/O and closes promptly); they would fail only if a future change reintroduced the deadlock. Tests: SNICloseDeadlockTest 6/6 pass on net9.0 and net462; MarsCloseDeadlockTest 2/2 pass on net9.0 against a live server. --- .../SQL/AsyncTest/MarsCloseDeadlockTest.cs | 9 +++++- .../SNICloseDeadlockTest.cs | 31 ++++++++++++++----- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs index 5284d9a689..21381f564d 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs @@ -122,7 +122,14 @@ public void CloseOrDispose_WithPendingMarsAsyncRead_DoesNotDeadlock(bool dispose finally { command.Dispose(); - connection.Dispose(); + // Only perform potentially-blocking cleanup if the close + // completed. If it deadlocked (closedInTime == false), calling + // Dispose() here could also block indefinitely and defeat the + // bounded-wait regression signal asserted below. + if (closedInTime) + { + connection.Dispose(); + } } Assert.True( diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs index e882fd27b1..20522ba042 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs @@ -32,14 +32,17 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; /// stalls after receiving the client's SQL batch. This guarantees that the /// client's async read for the response is genuinely in flight at the moment /// the connection is closed. The close is performed on a worker thread and -/// wrapped in a bounded wait: a deadlock manifests as the wait timing out, -/// which fails the test. After the fix, the close completes promptly. +/// wrapped in a bounded wait: a healthy close completes promptly, whereas a +/// deadlock manifests as the wait timing out, which fails the test. /// /// /// -/// Expected state: these tests are expected to FAIL (time out) against -/// the current, unfixed close path and PASS once the drain-before-close fix is -/// implemented. +/// Expected state: these tests PASS against the current code base. +/// Microsoft.Data.SqlClient does not suffer from this deadlock: the close path +/// drains (or cancels) the in-flight async I/O and completes well within the +/// bounded wait. They therefore serve as regression guards - if a future change +/// reintroduced the SNIClose deadlock, the bounded wait would time out and the +/// tests would fail. /// /// public class SNICloseDeadlockTest @@ -187,7 +190,11 @@ private static void RunPendingAsyncReadCloseScenario(bool disposeInsteadOfClose) finally { command.Dispose(); - if (!disposeInsteadOfClose) + // Only perform potentially-blocking cleanup if the close completed. + // If it deadlocked (closedInTime == false), calling Dispose() here + // could also block indefinitely and defeat the bounded-wait + // regression signal asserted below. + if (closedInTime && !disposeInsteadOfClose) { connection.Dispose(); } @@ -324,7 +331,11 @@ private static void RunPendingHandshakeCloseScenario(bool disposeInsteadOfClose) } finally { - if (!disposeInsteadOfClose) + // Only perform potentially-blocking cleanup if the close completed. + // If it deadlocked (closedInTime == false), calling Dispose() here + // could also block indefinitely and defeat the bounded-wait + // regression signal asserted below. + if (closedInTime && !disposeInsteadOfClose) { connection.Dispose(); } @@ -523,7 +534,11 @@ private static void RunPendingTlsHandshakeCloseScenario(bool disposeInsteadOfClo } finally { - if (!disposeInsteadOfClose) + // Only perform potentially-blocking cleanup if the close completed. + // If it deadlocked (closedInTime == false), calling Dispose() here + // could also block indefinitely and defeat the bounded-wait + // regression signal asserted below. + if (closedInTime && !disposeInsteadOfClose) { connection.Dispose(); } From 29aa65af6da46e0f912e9df82961a3ca50c8774e Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Fri, 3 Jul 2026 06:28:52 -0300 Subject: [PATCH 04/10] Refactor SNIClose tests to xUnit Theory (Close/Dispose via InlineData) Replace each pair of Close/Dispose [Fact] methods plus their private helper with a single [Theory] parameterized by disposeInsteadOfClose ([InlineData(false)]/[InlineData(true)]), matching the MARS manual test's style. No behavior change; still 6 cases (3 theories x 2). --- .../SNICloseDeadlockTest.cs | 51 +++++-------------- 1 file changed, 12 insertions(+), 39 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs index 20522ba042..12483ac592 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs @@ -94,19 +94,10 @@ protected override TDSMessageCollection CreateQueryResponse( } } - [Fact] - public void CloseConnection_WithPendingAsyncRead_DoesNotDeadlock() - { - RunPendingAsyncReadCloseScenario(disposeInsteadOfClose: false); - } - - [Fact] - public void DisposeConnection_WithPendingAsyncRead_DoesNotDeadlock() - { - RunPendingAsyncReadCloseScenario(disposeInsteadOfClose: true); - } - - private static void RunPendingAsyncReadCloseScenario(bool disposeInsteadOfClose) + [Theory] + [InlineData(false)] + [InlineData(true)] + public void CloseOrDispose_WithPendingAsyncRead_DoesNotDeadlock(bool disposeInsteadOfClose) { using ManualResetEventSlim batchReceived = new(false); using ManualResetEventSlim releaseResponse = new(false); @@ -207,18 +198,6 @@ private static void RunPendingAsyncReadCloseScenario(bool disposeInsteadOfClose) "This indicates the SNIClose deadlock (ADO.Net #43847 / ICM 775308542)."); } - [Fact] - public void CloseConnection_DuringPreLoginHandshake_DoesNotDeadlock() - { - RunPendingHandshakeCloseScenario(disposeInsteadOfClose: false); - } - - [Fact] - public void DisposeConnection_DuringPreLoginHandshake_DoesNotDeadlock() - { - RunPendingHandshakeCloseScenario(disposeInsteadOfClose: true); - } - /// /// Reproduces the ICM scenario more faithfully: the connection is torn down /// while it is still in the middle of connection establishment (the @@ -232,7 +211,10 @@ public void DisposeConnection_DuringPreLoginHandshake_DoesNotDeadlock() /// handshake-phase close path manifests as the wait timing out. /// /// - private static void RunPendingHandshakeCloseScenario(bool disposeInsteadOfClose) + [Theory] + [InlineData(false)] + [InlineData(true)] + public void CloseOrDispose_DuringPreLoginHandshake_DoesNotDeadlock(bool disposeInsteadOfClose) { using ManualResetEventSlim clientConnected = new(false); using ManualResetEventSlim releaseServer = new(false); @@ -394,18 +376,6 @@ private static void RunPendingHandshakeCloseScenario(bool disposeInsteadOfClose) 0x01, // ENCRYPTION = ENCRYPT_ON }; - [Fact] - public void CloseConnection_DuringTlsHandshake_DoesNotDeadlock() - { - RunPendingTlsHandshakeCloseScenario(disposeInsteadOfClose: false); - } - - [Fact] - public void DisposeConnection_DuringTlsHandshake_DoesNotDeadlock() - { - RunPendingTlsHandshakeCloseScenario(disposeInsteadOfClose: true); - } - /// /// Reproduces the ICM scenario faithfully: the connection is torn down while /// the client is inside the TLS handshake over TDS, with an SNI read pending @@ -423,7 +393,10 @@ public void DisposeConnection_DuringTlsHandshake_DoesNotDeadlock() /// manifests as the wait timing out. /// /// - private static void RunPendingTlsHandshakeCloseScenario(bool disposeInsteadOfClose) + [Theory] + [InlineData(false)] + [InlineData(true)] + public void CloseOrDispose_DuringTlsHandshake_DoesNotDeadlock(bool disposeInsteadOfClose) { using ManualResetEventSlim handshakeInFlight = new(false); using ManualResetEventSlim releaseServer = new(false); From 988fac7475ffbaa4d83c21c3ae7e59e529a38963 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Fri, 3 Jul 2026 12:28:48 -0300 Subject: [PATCH 05/10] Address PR #4420 review: dedicated close thread + always-run test cleanup Second round of copilot-pull-request-reviewer feedback on #4420: - Run the Close/Dispose worker on a dedicated long-running thread (Task.Factory.StartNew(..., TaskCreationOptions.LongRunning)) instead of a thread-pool thread, so a hypothetical deadlock regression parks one dedicated thread rather than starving the shared pool. - Wrap each scenario body in try/finally so cleanup always runs even if a precondition assert throws: the release event is always signaled, the TCP listener is always stopped, and the command/connection are disposed when the close worker never started or completed. The blocking connection.Dispose() is still skipped when a deadlock is actually detected (closedInTime == false), so the failure stays bounded and no listener/connection is leaked. Applies to all four scenarios (post-login read, pre-login, TLS handshake, MARS). Tests: SNICloseDeadlockTest 6/6 pass on net9.0 and net462; MarsCloseDeadlockTest 2/2 pass on net9.0. --- .../SQL/AsyncTest/MarsCloseDeadlockTest.cs | 120 +++--- .../SNICloseDeadlockTest.cs | 389 ++++++++++-------- 2 files changed, 287 insertions(+), 222 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs index 21381f564d..41626fe778 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs @@ -74,70 +74,88 @@ public void CloseOrDispose_WithPendingMarsAsyncRead_DoesNotDeadlock(bool dispose }.ConnectionString; SqlConnection connection = new(connectionString); - connection.Open(); + SqlCommand command = null; + Task readTask = null; + bool closeAttempted = false; + bool closedInTime = false; + try + { + connection.Open(); - SqlCommand command = - new($"WAITFOR DELAY '{StallDelay}'; SELECT 1;", connection); + command = new($"WAITFOR DELAY '{StallDelay}'; SELECT 1;", connection); - // Start an async read that will pend: the server withholds the - // response for StallDelay, so this Task will not complete until we - // either wait out the delay or tear down the connection. - Task readTask = command.ExecuteReaderAsync(); + // Start an async read that will pend: the server withholds the + // response for StallDelay, so this Task will not complete until + // we either wait out the delay or tear down the connection. + readTask = command.ExecuteReaderAsync(); - // Confirm the read is genuinely in flight before we close: it must - // not have completed and the connection must still be open. - Assert.False( - readTask.Wait(PendingConfirmDelay), - "The async read completed before the server released the " + - "response; the read was not in flight at close time."); - Assert.Equal(ConnectionState.Open, connection.State); + // Confirm the read is genuinely in flight before we close: it + // must not have completed and the connection must still be open. + Assert.False( + readTask.Wait(PendingConfirmDelay), + "The async read completed before the server released the " + + "response; the read was not in flight at close time."); + Assert.Equal(ConnectionState.Open, connection.State); - // Close/dispose the connection on a worker thread while the MARS - // async read is in flight. This is the operation that can deadlock - // in SNIClose. - Task closeTask = Task.Run(() => - { - if (disposeInsteadOfClose) - { - connection.Dispose(); - } - else - { - connection.Close(); - } - }); + // Close/dispose the connection on a dedicated, long-running + // thread while the MARS async read is in flight. This is the + // operation that can deadlock in SNIClose. A dedicated thread + // (rather than a thread-pool thread via Task.Run) ensures a + // hypothetical regression parks this one thread instead of + // starving the shared thread pool. + closeAttempted = true; + Task closeTask = Task.Factory.StartNew( + () => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }, + TaskCreationOptions.LongRunning); - bool closedInTime = closeTask.Wait(CloseBudget); + closedInTime = closeTask.Wait(CloseBudget); - // Observe the read task so its (expected) failure is not unobserved. - try - { - readTask.Wait(CloseBudget); - } - catch - { - // The pending read is expected to fault or cancel once the - // connection is torn down. That is not what this test asserts. + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not " + + $"complete within {CloseBudget.TotalSeconds:N0}s while a MARS " + + "async read was in flight. This indicates the SNIClose deadlock " + + "(ADO.Net #43847 / ICM 775308542)."); } finally { - command.Dispose(); - // Only perform potentially-blocking cleanup if the close - // completed. If it deadlocked (closedInTime == false), calling - // Dispose() here could also block indefinitely and defeat the - // bounded-wait regression signal asserted below. - if (closedInTime) + // Observe the pending read so its (expected) fault is not + // unobserved, even if a precondition assert above threw. + if (readTask != null) + { + try + { + readTask.Wait(CloseBudget); + } + catch + { + // The pending read is expected to fault or cancel once + // the connection is torn down. That is not what this + // test asserts. + } + } + + command?.Dispose(); + + // Dispose the connection unless a close deadlock was actually + // detected (in which case Dispose() could also block). Disposing + // is safe when the close worker never started or when it + // completed. + if (!closeAttempted || closedInTime) { connection.Dispose(); } } - - Assert.True( - closedInTime, - $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not " + - $"complete within {CloseBudget.TotalSeconds:N0}s while a MARS " + - "async read was in flight. This indicates the SNIClose deadlock " + - "(ADO.Net #43847 / ICM 775308542)."); } } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs index 12483ac592..525c8c4215 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs @@ -122,80 +122,98 @@ public void CloseOrDispose_WithPendingAsyncRead_DoesNotDeadlock(bool disposeInst }; SqlConnection connection = new(builder.ConnectionString); - connection.Open(); - - SqlCommand command = new("SELECT 1", connection); - - // Start an async read that will pend: the server stalls in - // CreateQueryResponse, so this Task will not complete until we either - // release the server or tear down the connection. - Task readTask = command.ExecuteReaderAsync(); - - // Ensure the async read is genuinely in flight before we close. - Assert.True( - batchReceived.Wait(HandshakeBudget), - "The server never received the SQL batch, so the async read was " + - "not in flight; the test cannot exercise the close-with-pending-IO path."); - - // The server is stalled withholding the response, so the client's async - // read must still be pending: the Task cannot have completed and the - // connection must still be open. If either of these is false, the read - // is not actually in flight and the test is not exercising the - // close-with-pending-IO path. - Assert.False( - readTask.IsCompleted, - "The async read completed before the server released the response; " + - "the read was not in flight at close time."); - Assert.Equal(System.Data.ConnectionState.Open, connection.State); - - // Close/dispose the connection on a worker thread while the async read - // is in flight. This is the operation that can deadlock in SNIClose. - Task closeTask = Task.Run(() => - { - if (disposeInsteadOfClose) - { - connection.Dispose(); - } - else - { - connection.Close(); - } - }); - - bool closedInTime = closeTask.Wait(CloseBudget); - - // Always release the stalled server thread so it can unwind and the - // server can be disposed cleanly, regardless of the outcome above. - releaseResponse.Set(); - - // Observe the read task so its (expected) failure is not unobserved. + SqlCommand? command = null; + Task? readTask = null; + bool closeAttempted = false; + bool closedInTime = false; try { - readTask.Wait(CloseBudget); - } - catch - { - // The pending read is expected to fault or cancel once the - // connection is torn down. That is not what this test asserts. + connection.Open(); + + command = new("SELECT 1", connection); + + // Start an async read that will pend: the server stalls in + // CreateQueryResponse, so this Task will not complete until we + // either release the server or tear down the connection. + readTask = command.ExecuteReaderAsync(); + + // Ensure the async read is genuinely in flight before we close. + Assert.True( + batchReceived.Wait(HandshakeBudget), + "The server never received the SQL batch, so the async read was " + + "not in flight; the test cannot exercise the close-with-pending-IO path."); + + // The server is stalled withholding the response, so the client's + // async read must still be pending: the Task cannot have completed + // and the connection must still be open. If either of these is + // false, the read is not actually in flight and the test is not + // exercising the close-with-pending-IO path. + Assert.False( + readTask.IsCompleted, + "The async read completed before the server released the response; " + + "the read was not in flight at close time."); + Assert.Equal(System.Data.ConnectionState.Open, connection.State); + + // Close/dispose the connection on a dedicated, long-running thread + // while the async read is in flight. This is the operation that can + // deadlock in SNIClose. A dedicated thread (rather than a + // thread-pool thread via Task.Run) ensures a hypothetical regression + // parks this one thread instead of starving the shared thread pool. + closeAttempted = true; + Task closeTask = Task.Factory.StartNew( + () => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }, + TaskCreationOptions.LongRunning); + + closedInTime = closeTask.Wait(CloseBudget); + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + + $"within {CloseBudget.TotalSeconds:N0}s while an async read was in flight. " + + "This indicates the SNIClose deadlock (ADO.Net #43847 / ICM 775308542)."); } finally { - command.Dispose(); - // Only perform potentially-blocking cleanup if the close completed. - // If it deadlocked (closedInTime == false), calling Dispose() here - // could also block indefinitely and defeat the bounded-wait - // regression signal asserted below. - if (closedInTime && !disposeInsteadOfClose) + // Always release the stalled server thread, even if a precondition + // assert above threw, so the using-scoped server can be disposed + // without blocking on the query engine. + releaseResponse.Set(); + + // Observe the pending read so its (expected) fault is not unobserved. + if (readTask != null) + { + try + { + readTask.Wait(CloseBudget); + } + catch + { + // The pending read is expected to fault or cancel once the + // connection is torn down. That is not what this test asserts. + } + } + + command?.Dispose(); + + // Dispose the connection unless a close deadlock was actually + // detected (in which case Dispose() could also block indefinitely + // and defeat the bounded-wait regression signal). Disposing is safe + // when the close worker never started or when it completed. + if (!closeAttempted || closedInTime) { connection.Dispose(); } } - - Assert.True( - closedInTime, - $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + - $"within {CloseBudget.TotalSeconds:N0}s while an async read was in flight. " + - "This indicates the SNIClose deadlock (ADO.Net #43847 / ICM 775308542)."); } /// @@ -268,56 +286,79 @@ public void CloseOrDispose_DuringPreLoginHandshake_DoesNotDeadlock(bool disposeI }; SqlConnection connection = new(builder.ConnectionString); - - // Begin connection establishment. This will not complete because the - // server never answers the pre-login handshake. - Task openTask = connection.OpenAsync(); - - // Wait until the server has received the client's pre-login, i.e. the - // client's handshake read is in flight. - Assert.True( - clientConnected.Wait(HandshakeBudget), - "The server never received the client's pre-login packet, so the " + - "handshake read was not in flight; the test cannot exercise the " + - "close-during-handshake path."); - - // Close/dispose the connection on a worker thread while the handshake - // read is in flight. This is the operation that can deadlock. - Task closeTask = Task.Run(() => - { - if (disposeInsteadOfClose) - { - connection.Dispose(); - } - else - { - connection.Close(); - } - }); - - bool closedInTime = closeTask.Wait(CloseBudget); - - // Release the server thread so it can unwind cleanly regardless of the - // outcome above. - releaseServer.Set(); - - // Observe the open task so its (expected) failure is not unobserved. + Task? openTask = null; + bool closeAttempted = false; + bool closedInTime = false; try { - openTask.Wait(CloseBudget); - } - catch - { - // OpenAsync is expected to fault or cancel once the connection is - // torn down. That is not what this test asserts. + // Begin connection establishment. This will not complete because + // the server never answers the pre-login handshake. + openTask = connection.OpenAsync(); + + // Wait until the server has received the client's pre-login, i.e. + // the client's handshake read is in flight. + Assert.True( + clientConnected.Wait(HandshakeBudget), + "The server never received the client's pre-login packet, so the " + + "handshake read was not in flight; the test cannot exercise the " + + "close-during-handshake path."); + + // Close/dispose the connection on a dedicated, long-running thread + // while the handshake read is in flight. This is the operation that + // can deadlock. A dedicated thread (rather than a thread-pool thread + // via Task.Run) ensures a hypothetical regression parks this one + // thread instead of starving the shared thread pool. + closeAttempted = true; + Task closeTask = Task.Factory.StartNew( + () => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }, + TaskCreationOptions.LongRunning); + + closedInTime = closeTask.Wait(CloseBudget); + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + + $"within {CloseBudget.TotalSeconds:N0}s while a pre-login response " + + "read was in flight. This indicates the SNIClose deadlock " + + "(ADO.Net #43847 / ICM 775308542)."); } finally { - // Only perform potentially-blocking cleanup if the close completed. - // If it deadlocked (closedInTime == false), calling Dispose() here - // could also block indefinitely and defeat the bounded-wait - // regression signal asserted below. - if (closedInTime && !disposeInsteadOfClose) + // Always release the server loop and stop the listener, even if a + // precondition assert above threw, so the background server task + // cannot stay blocked and the listening socket is not leaked into + // subsequent tests. + releaseServer.Set(); + listener.Stop(); + + // Observe the open task so its (expected) failure is not unobserved. + if (openTask != null) + { + try + { + openTask.Wait(CloseBudget); + } + catch + { + // OpenAsync is expected to fault or cancel once the + // connection is torn down. That is not what this test asserts. + } + } + + // Dispose the connection unless a close deadlock was actually + // detected (in which case Dispose() could also block). Disposing is + // safe when the close worker never started or when it completed. + if (!closeAttempted || closedInTime) { connection.Dispose(); } @@ -330,16 +371,7 @@ public void CloseOrDispose_DuringPreLoginHandshake_DoesNotDeadlock(bool disposeI { // Ignore server teardown faults. } - - listener.Stop(); } - - Assert.True( - closedInTime, - $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + - $"within {CloseBudget.TotalSeconds:N0}s while a pre-login response " + - "read was in flight. This indicates the SNIClose deadlock " + - "(ADO.Net #43847 / ICM 775308542)."); } /// @@ -464,54 +496,78 @@ public void CloseOrDispose_DuringTlsHandshake_DoesNotDeadlock(bool disposeInstea }; SqlConnection connection = new(builder.ConnectionString); - - // Begin connection establishment. This will not complete because the - // server never finishes the TLS handshake. - Task openTask = connection.OpenAsync(); - - // Wait until the client is inside the TLS handshake with a pending read. - Assert.True( - handshakeInFlight.Wait(HandshakeBudget), - "The client never reached the TLS handshake, so no SNI read was in " + - "flight; the test cannot exercise the close-during-TLS-handshake path."); - - // Close/dispose the connection on a worker thread while the TLS - // handshake read is in flight. This is the operation that can deadlock. - Task closeTask = Task.Run(() => - { - if (disposeInsteadOfClose) - { - connection.Dispose(); - } - else - { - connection.Close(); - } - }); - - bool closedInTime = closeTask.Wait(CloseBudget); - - // Release the server thread so it can unwind cleanly regardless of the - // outcome above. - releaseServer.Set(); - - // Observe the open task so its (expected) failure is not unobserved. + Task? openTask = null; + bool closeAttempted = false; + bool closedInTime = false; try { - openTask.Wait(CloseBudget); - } - catch - { - // OpenAsync is expected to fault or cancel once the connection is - // torn down. That is not what this test asserts. + // Begin connection establishment. This will not complete because + // the server never finishes the TLS handshake. + openTask = connection.OpenAsync(); + + // Wait until the client is inside the TLS handshake with a pending + // read. + Assert.True( + handshakeInFlight.Wait(HandshakeBudget), + "The client never reached the TLS handshake, so no SNI read was in " + + "flight; the test cannot exercise the close-during-TLS-handshake path."); + + // Close/dispose the connection on a dedicated, long-running thread + // while the TLS handshake read is in flight. This is the operation + // that can deadlock. A dedicated thread (rather than a thread-pool + // thread via Task.Run) ensures a hypothetical regression parks this + // one thread instead of starving the shared thread pool. + closeAttempted = true; + Task closeTask = Task.Factory.StartNew( + () => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }, + TaskCreationOptions.LongRunning); + + closedInTime = closeTask.Wait(CloseBudget); + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + + $"within {CloseBudget.TotalSeconds:N0}s while a TLS handshake read was " + + "in flight. This indicates the SNIClose deadlock " + + "(ADO.Net #43847 / ICM 775308542)."); } finally { - // Only perform potentially-blocking cleanup if the close completed. - // If it deadlocked (closedInTime == false), calling Dispose() here - // could also block indefinitely and defeat the bounded-wait - // regression signal asserted below. - if (closedInTime && !disposeInsteadOfClose) + // Always release the server loop and stop the listener, even if a + // precondition assert above threw, so the background server task + // cannot stay blocked and the listening socket is not leaked into + // subsequent tests. + releaseServer.Set(); + listener.Stop(); + + // Observe the open task so its (expected) failure is not unobserved. + if (openTask != null) + { + try + { + openTask.Wait(CloseBudget); + } + catch + { + // OpenAsync is expected to fault or cancel once the + // connection is torn down. That is not what this test asserts. + } + } + + // Dispose the connection unless a close deadlock was actually + // detected (in which case Dispose() could also block). Disposing is + // safe when the close worker never started or when it completed. + if (!closeAttempted || closedInTime) { connection.Dispose(); } @@ -524,15 +580,6 @@ public void CloseOrDispose_DuringTlsHandshake_DoesNotDeadlock(bool disposeInstea { // Ignore server teardown faults. } - - listener.Stop(); } - - Assert.True( - closedInTime, - $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete " + - $"within {CloseBudget.TotalSeconds:N0}s while a TLS handshake read was " + - "in flight. This indicates the SNIClose deadlock " + - "(ADO.Net #43847 / ICM 775308542)."); } } From 88a17258ed9f1671b4caf7d6fa3f2c76c6ec090f Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Sat, 4 Jul 2026 10:44:44 -0300 Subject: [PATCH 06/10] Fix false-positive risk in pre-login SNIClose deadlock test Only signal clientConnected when a pre-login byte is actually read. Previously ReadByte() throwing or returning -1 still set the event, letting the test pass without a genuine in-flight handshake read. Mirrors the existing guard in the TLS-handshake scenario. Addresses PR #4420 review (discussion_r3521209803). --- .../SNICloseDeadlockTest.cs | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs index 525c8c4215..d375fcca2a 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs @@ -250,22 +250,31 @@ public void CloseOrDispose_DuringPreLoginHandshake_DoesNotDeadlock(bool disposeI try { - // Read one byte of the client's pre-login packet. Once this - // returns the client has sent its pre-login and posted its - // async read for the response. We only need to know data + // Read one byte of the client's pre-login packet. A byte here + // proves the client actually sent its pre-login and posted its + // async read for the response; we only need to know data // arrived, so a single byte is sufficient. - stream.ReadByte(); + if (stream.ReadByte() < 0) + { + // Client tore down without sending pre-login bytes: the + // handshake read was never actually in flight. Leave + // clientConnected unset so the test's precondition wait + // fails instead of producing a false positive. + return; + } + + clientConnected.Set(); + + // Hold the socket open (withholding the response) until the test + // releases us, so the client's read cannot complete naturally. + releaseServer.Wait(); } catch { - // The client may tear down the socket; ignore. + // The client may tear down the socket; ignore. clientConnected + // stays unset unless a pre-login byte was actually observed + // above, so a failed read cannot masquerade as a pending one. } - - clientConnected.Set(); - - // Hold the socket open (withholding the response) until the test - // releases us, so the client's read cannot complete naturally. - releaseServer.Wait(); }); SqlConnectionStringBuilder builder = new() From daa8f338976ffc0e963c4524ac6863ef2f1e1081 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:10:13 -0300 Subject: [PATCH 07/10] Add legacy System.Data.SqlClient SNIClose deadlock repro harness and race tests Add tools/SniCloseLegacyRepro: an xUnit harness that links the branch's SNIClose/MARS close-during-IO deadlock tests and runs them against the legacy System.Data.SqlClient driver (netfx in-box and the deprecated NuGet package). Includes a container runner (run-in-container.ps1) for exercising older in-box System.Data.dll versions, CPM props, and a README. Promote the race-provoking variants into the driver test suite: SNICloseRaceDeadlockTest (UnitTests) and MarsCloseStressTest (ManualTests). Enable nullable context in SNICloseDeadlockTest and MarsCloseDeadlockTest. Refs ICM 775308542 / ADO.Net #43847. --- .../SQL/AsyncTest/MarsCloseDeadlockTest.cs | 6 +- .../SQL/AsyncTest/MarsCloseStressTest.cs | 164 ++++++++++++ .../SNICloseDeadlockTest.cs | 2 + .../SNICloseRaceDeadlockTest.cs | 192 ++++++++++++++ tools/SniCloseLegacyRepro/Compat.cs | 117 ++++++++ .../SniCloseLegacyRepro/Directory.Build.props | 14 + .../Directory.Packages.props | 22 ++ tools/SniCloseLegacyRepro/GlobalUsings.cs | 26 ++ tools/SniCloseLegacyRepro/README.md | 251 ++++++++++++++++++ .../SniCloseLegacyRepro.csproj | 113 ++++++++ .../SniCloseLegacyRepro/run-in-container.ps1 | 104 ++++++++ 11 files changed, 1009 insertions(+), 2 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseRaceDeadlockTest.cs create mode 100644 tools/SniCloseLegacyRepro/Compat.cs create mode 100644 tools/SniCloseLegacyRepro/Directory.Build.props create mode 100644 tools/SniCloseLegacyRepro/Directory.Packages.props create mode 100644 tools/SniCloseLegacyRepro/GlobalUsings.cs create mode 100644 tools/SniCloseLegacyRepro/README.md create mode 100644 tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj create mode 100644 tools/SniCloseLegacyRepro/run-in-container.ps1 diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs index 41626fe778..dce3471791 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +#nullable enable + using System; using System.Data; using System.Threading.Tasks; @@ -74,8 +76,8 @@ public void CloseOrDispose_WithPendingMarsAsyncRead_DoesNotDeadlock(bool dispose }.ConnectionString; SqlConnection connection = new(connectionString); - SqlCommand command = null; - Task readTask = null; + SqlCommand? command = null; + Task? readTask = null; bool closeAttempted = false; bool closedInTime = false; try diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs new file mode 100644 index 0000000000..cb5072e764 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs @@ -0,0 +1,164 @@ +// 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. + +#nullable enable + +using System; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + /// + /// Concurrency-stress companion to for + /// ADO.Net #43847 / ICM 775308542. + /// + /// + /// Where MarsCloseDeadlockTest closes a single connection with one + /// in-flight MARS async read, this test opens many MARS connections - + /// each with an in-flight read - and closes them all simultaneously + /// (rendezvous on a ), repeated over several iterations. + /// Piling concurrent closes onto the SMUX logical-session teardown path + /// widens the window for the close race called out by the long-standing + /// "Comment CloseMARSSession / UNDONE" note in the native Dispose(). + /// + /// + /// + /// Each batch of closes is awaited under a bounded wait: a deadlock manifests + /// as the wait timing out, which fails the test. A healthy close aborts the + /// in-flight commands and returns promptly. + /// + /// + [Trait("Set", "1")] + public class MarsCloseStressTest + { + /// + /// How long each server-side batch withholds its response. Must comfortably + /// exceed so a prompt close is attributable to the + /// close path aborting the command, not the query completing on its own. + /// + private const string StallDelay = "00:00:30"; + + /// + /// Upper bound for how long a batch of concurrent, non-deadlocked closes + /// should take. Generous only to avoid false positives on slow agents. + /// + private static readonly TimeSpan CloseBudget = TimeSpan.FromSeconds(30); + + /// Number of close-storm iterations. + private const int Iterations = 8; + + /// MARS connections opened and closed together per iteration. + private const int ConnectionsPerIteration = 16; + + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + [InlineData(false)] + [InlineData(true)] + public void MarsConcurrentCloseStress_DoesNotDeadlock(bool disposeInsteadOfClose) + { + // Enable MARS so each async read runs over a SMUX logical session, and + // disable pooling so Close()/Dispose() tears down the physical + // connection (reaching SNIClose) instead of returning it to the pool. + string connectionString = + new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + MultipleActiveResultSets = true, + Pooling = false, + }.ConnectionString; + + for (int iter = 0; iter < Iterations; iter++) + { + SqlConnection[] connections = new SqlConnection[ConnectionsPerIteration]; + SqlCommand?[] commands = new SqlCommand?[ConnectionsPerIteration]; + Task?[] readTasks = new Task?[ConnectionsPerIteration]; + bool allClosed = false; + try + { + for (int k = 0; k < ConnectionsPerIteration; k++) + { + connections[k] = new SqlConnection(connectionString); + connections[k].Open(); + commands[k] = new SqlCommand($"WAITFOR DELAY '{StallDelay}'; SELECT 1;", connections[k]); + readTasks[k] = commands[k]!.ExecuteReaderAsync(); + } + + // Confirm every read is genuinely in flight before closing. + for (int k = 0; k < ConnectionsPerIteration; k++) + { + Assert.False( + readTasks[k]!.Wait(TimeSpan.FromMilliseconds(50)), + $"iteration {iter}, connection {k}: the async read completed before " + + "close; it was not in flight."); + } + + // Fire all closes at once through a barrier so they pile up on + // the close path concurrently. + using Barrier gate = new(ConnectionsPerIteration); + Task[] closeTasks = new Task[ConnectionsPerIteration]; + for (int k = 0; k < ConnectionsPerIteration; k++) + { + int idx = k; + closeTasks[k] = Task.Factory.StartNew( + () => + { + gate.SignalAndWait(); + if (disposeInsteadOfClose) + { + connections[idx].Dispose(); + } + else + { + connections[idx].Close(); + } + }, + TaskCreationOptions.LongRunning); + } + + allClosed = Task.WaitAll(closeTasks, CloseBudget); + + Assert.True( + allClosed, + $"iteration {iter}: {ConnectionsPerIteration} concurrent MARS " + + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} calls did not all " + + $"complete within {CloseBudget.TotalSeconds:N0}s. This indicates the " + + "SNIClose deadlock (ADO.Net #43847 / ICM 775308542)."); + } + finally + { + for (int k = 0; k < ConnectionsPerIteration; k++) + { + if (readTasks[k] != null) + { + try + { + readTasks[k]!.Wait(TimeSpan.FromSeconds(5)); + } + catch + { + // Reads are expected to fault or cancel on teardown. + } + } + + commands[k]?.Dispose(); + + // Only dispose connections if the closes completed; a + // deadlocked connection's Dispose() could also block. + if (allClosed) + { + try + { + connections[k]?.Dispose(); + } + catch + { + // Ignore teardown faults. + } + } + } + } + } + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs index d375fcca2a..465e284b6d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +#nullable enable + using System; using System.Net; using System.Net.Sockets; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseRaceDeadlockTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseRaceDeadlockTest.cs new file mode 100644 index 0000000000..27ab3a7516 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseRaceDeadlockTest.cs @@ -0,0 +1,192 @@ +// 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. + +#nullable enable + +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.SqlServer.TDS; +using Microsoft.SqlServer.TDS.EndPoint; +using Microsoft.SqlServer.TDS.Servers; +using Microsoft.SqlServer.TDS.SQLBatch; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; + +/// +/// Race variant of the SNIClose close-during-I/O regression tests +/// (ADO.Net #43847 / ICM 775308542). +/// +/// +/// The sibling SNICloseDeadlockTest closes over a quiescent +/// pending read: the server stays silent, so the close path simply cancels the +/// in-flight read and no completion callback is executing at close time. This +/// test instead releases the server's response at the same instant the +/// connection is closed (rendezvous on a ), so the +/// read's completion callback fires concurrently with SNIClose - the +/// exact ordering hazard the deadlock is about. It repeats many iterations to +/// widen the race window. +/// +/// +/// +/// The close runs on a dedicated long-running thread under a bounded wait: a +/// deadlock manifests as the wait timing out, which fails the test. A healthy +/// close completes promptly. +/// +/// +public class SNICloseRaceDeadlockTest +{ + /// + /// Upper bound for how long a non-deadlocked close should take. Generous so + /// it only distinguishes "completed" from "deadlocked" on slow CI agents. + /// + private static readonly TimeSpan CloseBudget = TimeSpan.FromSeconds(30); + + /// + /// Upper bound for waiting on the server to receive the client's batch, i.e. + /// for the client's async read to become pending. + /// + private static readonly TimeSpan HandshakeBudget = TimeSpan.FromSeconds(30); + + /// + /// A query engine that signals when a SQL batch arrives and then withholds + /// the response until released, so the test controls exactly when the + /// client's read completion fires. + /// + private sealed class ReleasableQueryEngine : QueryEngine + { + private readonly ManualResetEventSlim _batchReceived; + private readonly ManualResetEventSlim _releaseResponse; + + public ReleasableQueryEngine( + TdsServerArguments arguments, + ManualResetEventSlim batchReceived, + ManualResetEventSlim releaseResponse) + : base(arguments) + { + _batchReceived = batchReceived; + _releaseResponse = releaseResponse; + } + + protected override TDSMessageCollection CreateQueryResponse( + ITDSServerSession session, + TDSSQLBatchToken batchRequest) + { + _batchReceived.Set(); + _releaseResponse.Wait(); + return base.CreateQueryResponse(session, batchRequest); + } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void ResponseCompletionRacesClose_DoesNotDeadlock(bool disposeInsteadOfClose) + { + const int iterations = 100; + + for (int i = 0; i < iterations; i++) + { + using ManualResetEventSlim batchReceived = new(false); + using ManualResetEventSlim releaseResponse = new(false); + + TdsServerArguments arguments = new(); + using TdsServer server = new( + new ReleasableQueryEngine(arguments, batchReceived, releaseResponse), + arguments); + server.Start(); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"localhost,{server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + // Disable pooling so Close()/Dispose() tears down the physical + // connection (reaching SNIClose) instead of returning it to the pool. + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + SqlCommand? command = null; + Task? readTask = null; + bool closeAttempted = false; + bool closedInTime = false; + try + { + connection.Open(); + + command = new("SELECT 1", connection); + readTask = command.ExecuteReaderAsync(); + + Assert.True( + batchReceived.Wait(HandshakeBudget), + $"iteration {i}: the server never received the SQL batch, so the " + + "async read was not in flight."); + + // Close on a dedicated, long-running thread, gated on a barrier; + // release the server response at the same instant so the read's + // completion callback fires concurrently with SNIClose. + using Barrier gate = new(2); + + closeAttempted = true; + Task closeTask = Task.Factory.StartNew( + () => + { + gate.SignalAndWait(); + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }, + TaskCreationOptions.LongRunning); + + gate.SignalAndWait(); + releaseResponse.Set(); + + closedInTime = closeTask.Wait(CloseBudget); + + Assert.True( + closedInTime, + $"iteration {i}: {(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not " + + $"complete within {CloseBudget.TotalSeconds:N0}s while a response completion " + + "raced close. This indicates the SNIClose deadlock (ADO.Net #43847 / ICM 775308542)."); + } + finally + { + // Always release the (possibly still-stalled) server thread so + // the using-scoped server can be disposed without blocking. + releaseResponse.Set(); + + if (readTask != null) + { + try + { + readTask.Wait(CloseBudget); + } + catch + { + // The pending read is expected to fault or cancel once the + // connection is torn down. That is not what this test asserts. + } + } + + command?.Dispose(); + + // Dispose unless a close deadlock was actually detected (in which + // case Dispose() could also block and defeat the regression signal). + if (!closeAttempted || closedInTime) + { + connection.Dispose(); + } + } + } + } +} diff --git a/tools/SniCloseLegacyRepro/Compat.cs b/tools/SniCloseLegacyRepro/Compat.cs new file mode 100644 index 0000000000..82e6d5f94c --- /dev/null +++ b/tools/SniCloseLegacyRepro/Compat.cs @@ -0,0 +1,117 @@ +// 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 LegacyBuilder = System.Data.SqlClient.SqlConnectionStringBuilder; + +namespace SniCloseLegacyRepro.Compat +{ + /// + /// Minimal stand-in for Microsoft.Data.SqlClient's SqlConnectionEncryptOption + /// enum, which does not exist in the legacy System.Data.SqlClient driver. + /// + public enum SqlConnectionEncryptOption + { + Optional, + Mandatory, + Strict, + } + + /// + /// A connection-string builder facade over the LEGACY driver's (sealed) + /// SqlConnectionStringBuilder. It exposes only the keywords the linked test + /// files use, and accepts the Microsoft.Data-style + /// enum for Encrypt so those + /// files compile unchanged. The legacy builder is sealed, so this uses + /// composition rather than inheritance. + /// + public sealed class ShimSqlConnectionStringBuilder + { + private readonly LegacyBuilder _inner; + + public ShimSqlConnectionStringBuilder() => _inner = new LegacyBuilder(); + + public ShimSqlConnectionStringBuilder(string connectionString) => + _inner = new LegacyBuilder(connectionString); + + public string ConnectionString => _inner.ConnectionString; + + public string DataSource + { + get => _inner.DataSource; + set => _inner.DataSource = value; + } + + public bool Pooling + { + get => _inner.Pooling; + set => _inner.Pooling = value; + } + + public bool TrustServerCertificate + { + get => _inner.TrustServerCertificate; + set => _inner.TrustServerCertificate = value; + } + + public int ConnectTimeout + { + get => _inner.ConnectTimeout; + set => _inner.ConnectTimeout = value; + } + + public int ConnectRetryCount + { + get => _inner.ConnectRetryCount; + set => _inner.ConnectRetryCount = value; + } + + public bool MultipleActiveResultSets + { + get => _inner.MultipleActiveResultSets; + set => _inner.MultipleActiveResultSets = value; + } + +#if NETFRAMEWORK + // Only present on the in-box (netfx) legacy builder; the tests guard its + // use with #if NETFRAMEWORK, so this facade matches that guard. + public bool TransparentNetworkIPResolution + { + get => _inner.TransparentNetworkIPResolution; + set => _inner.TransparentNetworkIPResolution = value; + } +#endif + + /// + /// Maps the Microsoft.Data encryption enum onto the legacy boolean + /// Encrypt keyword: Optional => false; Mandatory/Strict => true. + /// + public SqlConnectionEncryptOption Encrypt + { + get => _inner.Encrypt + ? SqlConnectionEncryptOption.Mandatory + : SqlConnectionEncryptOption.Optional; + set => _inner.Encrypt = value != SqlConnectionEncryptOption.Optional; + } + } +} + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + /// + /// Stand-in for the real test-infrastructure DataTestUtility, providing just + /// the members the linked MARS test and the stress tests reference. The live + /// connection string is taken from the SNICLOSE_CONNSTR environment variable. + /// + public static class DataTestUtility + { + public static string TCPConnectionString => + Environment.GetEnvironmentVariable("SNICLOSE_CONNSTR") ?? string.Empty; + + public static bool AreConnStringsSetup() => + !string.IsNullOrWhiteSpace(TCPConnectionString); + + public static bool IsNotAzureSynapse() => true; + } +} diff --git a/tools/SniCloseLegacyRepro/Directory.Build.props b/tools/SniCloseLegacyRepro/Directory.Build.props new file mode 100644 index 0000000000..cd6b5c8b61 --- /dev/null +++ b/tools/SniCloseLegacyRepro/Directory.Build.props @@ -0,0 +1,14 @@ + + + + + + true + + + diff --git a/tools/SniCloseLegacyRepro/Directory.Packages.props b/tools/SniCloseLegacyRepro/Directory.Packages.props new file mode 100644 index 0000000000..23d7f8278f --- /dev/null +++ b/tools/SniCloseLegacyRepro/Directory.Packages.props @@ -0,0 +1,22 @@ + + + + + + + + + + diff --git a/tools/SniCloseLegacyRepro/GlobalUsings.cs b/tools/SniCloseLegacyRepro/GlobalUsings.cs new file mode 100644 index 0000000000..806fa4fb37 --- /dev/null +++ b/tools/SniCloseLegacyRepro/GlobalUsings.cs @@ -0,0 +1,26 @@ +// 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. + +// ----------------------------------------------------------------------------- +// Global using aliases that let the linked test files (which use bare +// Microsoft.Data.SqlClient type names) bind to the LEGACY System.Data.SqlClient +// driver instead. +// +// How this works: the linked files live in namespaces under +// Microsoft.Data.SqlClient.*, so an unqualified name like "SqlConnection" is +// resolved by walking the enclosing namespaces. Because this assembly does NOT +// reference Microsoft.Data.SqlClient, none of those namespaces define these +// types, so name resolution falls through to these global aliases. +// +// SqlConnectionStringBuilder and SqlConnectionEncryptOption are aliased to shim +// types (see Compat.cs) because the legacy builder's Encrypt property is a bool, +// whereas the tests assign the Microsoft.Data-only SqlConnectionEncryptOption +// enum. The shim reconciles that difference. +// ----------------------------------------------------------------------------- + +global using SqlConnection = System.Data.SqlClient.SqlConnection; +global using SqlCommand = System.Data.SqlClient.SqlCommand; +global using SqlDataReader = System.Data.SqlClient.SqlDataReader; +global using SqlConnectionStringBuilder = SniCloseLegacyRepro.Compat.ShimSqlConnectionStringBuilder; +global using SqlConnectionEncryptOption = SniCloseLegacyRepro.Compat.SqlConnectionEncryptOption; diff --git a/tools/SniCloseLegacyRepro/README.md b/tools/SniCloseLegacyRepro/README.md new file mode 100644 index 0000000000..4b5ae54683 --- /dev/null +++ b/tools/SniCloseLegacyRepro/README.md @@ -0,0 +1,251 @@ +# SniCloseLegacyRepro + +Diagnostic harness that runs **SNIClose / MARS close-during-I/O deadlock tests** against the +**legacy `System.Data.SqlClient`** driver, to see whether the deadlock from **ICM 775308542 / +ADO.Net #43847** ("SNIClose can deadlock with in-flight async I/O during connection close") +reproduces there. + +--- + +## Background + +ICM 775308542 / ADO.Net #43847 concern a potential SNIClose deadlock with +in-flight async I/O during connection close in the **retired, in-box +`System.Data.SqlClient`**. The environment reported in the ICM: + +| | Reported environment (ICM) | +|---|---| +| OS | Windows Server 2016 (10.0.14393) | +| Runtime | .NET Framework 4.7 (CLR 4.0.30319) | +| Driver | in-box `System.Data.dll` **4.7.4081.0** | +| CPUs | 4 | + +`System.Data.SqlClient` is retired, so the fix (if any) is being investigated in +`Microsoft.Data.SqlClient` via #43847. The branch adds regression tests on the +MDS side; this harness points those same tests at the legacy driver. + +--- + +## How it works + +Rather than re-implementing the tests, the project **links the real test source +files** and swaps only the driver reference: + +- **Linked tests** (compiled verbatim, not copied): + - `SNICloseDeadlockTest.cs` (from `tests/UnitTests/SimulatedServerTests`) — + 3 in-process tests: pending async read (via the in-repo TDS test server), + pre-login handshake, and TLS handshake (both via raw `TcpListener`). + - `SNICloseRaceDeadlockTest.cs` (same folder) — releases the server response + at the same instant as close so the read completion races `SNIClose` + (×100 iterations). + - `MarsCloseDeadlockTest.cs` (from `tests/ManualTests/SQL/AsyncTest`) — + live-server MARS pending-read test. + - `MarsCloseStressTest.cs` (same folder) — many MARS connections with + in-flight reads, all closed simultaneously over several iterations + (live server). +- **Compat shim** makes the legacy driver satisfy the linked files' bare + `Microsoft.Data.SqlClient` type names: + - `GlobalUsings.cs` — global `using` aliases mapping `SqlConnection`, + `SqlCommand`, `SqlDataReader` to `System.Data.SqlClient`, and + `SqlConnectionStringBuilder` / `SqlConnectionEncryptOption` to the shims. + - `Compat.cs` — a `SqlConnectionEncryptOption` enum stand-in, a + `SqlConnectionStringBuilder` facade over the (sealed) legacy builder that + accepts the enum-typed `Encrypt`, and a `DataTestUtility` stand-in that + reads the connection string from `SNICLOSE_CONNSTR`. + +All four test files are the **real driver tests**; this project only links them +and swaps the driver reference. (The race/stress tests were originally prototyped +here and have since been promoted into the driver test projects.) + +The bare-name binding trick: the linked files sit in `Microsoft.Data.SqlClient.*` +namespaces, and because this project does **not** reference MDS, unqualified +names like `SqlConnection` fall through to the global aliases in `GlobalUsings.cs`. + +### Why `xunit.runner.json` (shadowCopy: false) matters + +`Microsoft.DotNet.XUnitExtensions` (which provides `[ConditionalTheory]`) ships a +**delay-signed** beta assembly. On .NET Framework, xUnit's default shadow copy +re-verifies the strong name from the shadow-copy directory and fails with +`FileLoadException: strong name could not be verified`. The real test projects +ship `xunit.runner.json` with `shadowCopy: false`; this harness **links that same +file** so the delay-signed assembly loads from its build-output location (where +.NET Framework's strong-name bypass applies). + +--- + +## Target frameworks + +`net462;net47;net472;net481;net10.0` + +- **.NET Framework targets** use the **in-box** `System.Data.dll`. +- **.NET (Core) target** (`net10.0`) uses the **`System.Data.SqlClient` NuGet + package** (version parametrized — see below). + +> **Important:** the in-box `System.Data.dll` is *machine-global* for the whole +> 4.x line — only one physical DLL exists per machine. On this dev box that is +> 4.8.1, so every netfx target binds 4.8.1 at runtime; the older TFMs only change +> compile-time references and framework "quirk" behaviors, **not** the DLL bits. +> To run the exact reported 4.7.4081.0 bits you need an environment isolated to +> .NET 4.7.2 (see [Testing older in-box DLLs](#testing-older-in-box-dlls-containers)). + +--- + +## Running + +From the repo root. + +### In-process tests only (no SQL Server) + +```powershell +dotnet test .\tools\SniCloseLegacyRepro +# or a single framework: +dotnet test .\tools\SniCloseLegacyRepro -f net481 +``` + +The live MARS test and the MARS stress test are skipped when no connection +string is set. + +### With a live SQL Server (enables MARS + stress tests) + +Set `SNICLOSE_CONNSTR` yourself so the password is not routed through tooling: + +```powershell +$env:SNICLOSE_CONNSTR = "Server=127.0.0.1,1434;Database=master;User ID=;Password=;TrustServerCertificate=true;Connect Timeout=15" +dotnet test .\tools\SniCloseLegacyRepro -f net481 +Remove-Item Env:\SNICLOSE_CONNSTR # clear when done +``` + +### Sweeping legacy NuGet versions (net10.0 only) + +The `.NET (Core)` target's package version is parametrized via +`$(SqlClientLegacyVersion)` (default `4.9.1`). The versions must exist on a +restore feed. + +```powershell +foreach ($v in '4.5.3','4.6.1','4.7.0','4.8.6','4.9.1') { + dotnet test .\tools\SniCloseLegacyRepro -f net10.0 -p:SqlClientLegacyVersion=$v +} +``` + +### Emulating the reported 4-CPU box + +Pin the test process (and its child `testhost`) to 4 cores: + +```powershell +$p = Start-Process dotnet -ArgumentList 'test','.\tools\SniCloseLegacyRepro','-f','net47','--no-build' -PassThru -NoNewWindow +$p.ProcessorAffinity = [IntPtr]0xF # cores 0-3 +$p.WaitForExit() +``` + +--- + +## Testing older in-box DLLs (containers) + +Because the in-box `System.Data.dll` is machine-global, the faithful way to test +older bits is an isolated environment. Use the official .NET Framework images — +the **framework version tag controls `System.Data.dll`**; the **Windows base tag +controls the OS + native `sni.dll`**. + +``` +mcr.microsoft.com/dotnet/framework/runtime:-windowsservercore- +``` + +| Image tag | `System.Data.dll` ≈ | Role | +|---|---|---| +| `4.6.2-windowsservercore-ltsc2016` | 4.7.4081.0 (see note) | — | +| `4.7-windowsservercore-ltsc2016` | **4.7.4081.0** | **ICM match** | +| `4.7.2-windowsservercore-ltsc2016` | 4.7.4081.0 | ICM match | +| `4.8-windowsservercore-ltsc2019` | 4.8.4690.0 | newer | +| `4.8.1-windowsservercore-ltsc2022` | 4.8.9xxx | this dev box | + +> **.NET Framework 4.x is a single, in-place, monotonic runtime.** The `ltsc2016` +> base OS is already serviced to 4.7.2, so its in-box `System.Data.dll` is +> **4.7.4081.0** — and a lower framework tag (e.g. `4.6.2`) cannot downgrade it. +> On this base, every tag at or below 4.7.2 resolves to the same 4.7.4081.0 DLL, +> which happens to be the reported version (the practical floor). Getting +> a genuinely older DLL (e.g. 4.6.x) would require an *unserviced* older base OS +> image, which MCR does not publish. + +Verify the DLL version inside a container: + +```powershell +(Get-Item C:\Windows\Microsoft.NET\Framework64\v4.0.30319\System.Data.dll).VersionInfo.FileVersion +``` + +### `run-in-container.ps1` + +The framework `runtime` images have **no `dotnet` CLI**, so `dotnet test` will not +work there. The `run-in-container.ps1` helper handles this: it builds `-f net47` +on the host, stages `xunit.console.exe` (from `xunit.runner.console`) next to the +build output, and runs the tests in the container so they bind the container's +old in-box `System.Data.dll`. + +```powershell +# in-process tests only (no SQL Server): +.\run-in-container.ps1 + +# faithful ICM match: 4.7.4081.0 + 4 processors, live MARS + stress: +$env:SNICLOSE_CONNSTR = "Server=127.0.0.1,1434;Database=master;User ID=sa;Password=***;TrustServerCertificate=true" +.\run-in-container.ps1 -Cpus 4 + +# a different framework image: +.\run-in-container.ps1 -Image mcr.microsoft.com/dotnet/framework/runtime:4.8-windowsservercore-ltsc2019 +``` + +**Networking:** a Windows container cannot reach the host's loopback, and +`host.docker.internal` is not injected for hyperv-isolated Windows containers. +The script targets the Docker NAT gateway (the host's `vEthernet (nat)` IP, e.g. +`172.17.224.1`) and rewrites the connection string to it. Because the SQL Server +(a Linux container published to host loopback) does not listen on that IP, add a +one-time **elevated** host bridge: + +```powershell +netsh interface portproxy add v4tov4 listenaddress=172.17.224.1 listenport=1434 connectaddress=127.0.0.1 connectport=1434 +New-NetFirewallRule -DisplayName "SQL 1434 from Windows containers" -Direction Inbound -LocalAddress 172.17.224.1 -LocalPort 1434 -Protocol TCP -Action Allow +# teardown: +# netsh interface portproxy delete v4tov4 listenaddress=172.17.224.1 listenport=1434 +# Remove-NetFirewallRule -DisplayName "SQL 1434 from Windows containers" +``` + +--- + +## Findings + +Across every configuration tested, the deadlock **does not reproduce** (all +tests pass), including: + +- in-box `System.Data.dll` 4.8.1 on `net462` / `net47` / `net472` / `net481` + (host dev box) +- 4-CPU-pinned runs +- the completion-races-close and MARS concurrency stress variants +- the full `System.Data.SqlClient` NuGet lineage on `net10.0` + (4.5.3, 4.6.1, 4.7.0, 4.8.6, 4.9.1) +- **the reported (ICM) environment**, reproduced faithfully in a container: + Windows Server 2016 base (`ltsc2016`) · .NET Framework 4.7 · in-box + `System.Data.dll` **4.7.4081.0** · **4 processors** (`--cpu-count 4`) · live + MARS + concurrency stress against SQL Server 2025 → **12/12 pass**. +- container `ltsc2019` · .NET Framework 4.8 · `System.Data.dll` 4.8.4690.0 · + 4 processors · live MARS + stress → **12/12 pass**. + +So the SNIClose close-during-I/O deadlock does not reproduce even in a +bit-for-bit reconstruction of the reported ICM environment. Because that +*exact* `System.Data.dll` (4.7.4081.0) also passes, there is no +version boundary / "fixed in a later servicing build" explanation: the synthetic +tests simply do not trigger whatever the original incident hit. That points away +from a plain client-side ordering bug reachable from managed timing alone, and +toward something more environment-specific in the original incident (e.g. exact +network/timing conditions, or a native-layer interaction not exercised here). + +--- + +## Isolation notes + +- Uses Central Package Management via its own `Directory.Build.props` + (`ManagePackageVersionsCentrally=true`) and `Directory.Packages.props`, which + imports the shared SqlClient tests package versions. The legacy + `System.Data.SqlClient` package is pinned there; the net10.0 target overrides + it per-run via `VersionOverride` / `-p:SqlClientLegacyVersion=…`. +- Legacy `System.Data.SqlClient` versions are served from the repo's governed + feed (no extra `NuGet.config`). +- References the driver-agnostic in-repo TDS test-server projects + (`tests/tools/TDS/*`). diff --git a/tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj b/tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj new file mode 100644 index 0000000000..544af613c7 --- /dev/null +++ b/tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj @@ -0,0 +1,113 @@ + + + + + + + net462;net47;net472;net481;net10.0 + + 4.9.1 + enable + latest + disable + true + false + + false + false + + $(NoWarn);CS0618 + + + + + + + + + + + + + + + + xunit.runner.json + PreserveNewest + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tools/SniCloseLegacyRepro/run-in-container.ps1 b/tools/SniCloseLegacyRepro/run-in-container.ps1 new file mode 100644 index 0000000000..bccbeb1a02 --- /dev/null +++ b/tools/SniCloseLegacyRepro/run-in-container.ps1 @@ -0,0 +1,104 @@ +# Runs the net47 build of this harness inside a .NET Framework Windows container, +# so the tests bind the container's OLD in-box System.Data.dll (e.g. 4.7.4081.0) +# instead of the host's. The framework container has no dotnet CLI, so we run the +# tests with xunit.console.exe. +# +# Examples: +# # in-process tests only (no SQL Server needed): +# .\run-in-container.ps1 +# +# # include the live MARS + stress tests against a host SQL Server: +# $env:SNICLOSE_CONNSTR = "Server=127.0.0.1,1434;Database=master;User ID=sa;Password=***;TrustServerCertificate=true" +# .\run-in-container.ps1 +# +# # a different framework image / version: +# .\run-in-container.ps1 -Image mcr.microsoft.com/dotnet/framework/runtime:4.8-windowsservercore-ltsc2019 + +[CmdletBinding()] +param( + [string]$Framework = 'net47', + [string]$Image = 'mcr.microsoft.com/dotnet/framework/runtime:4.7-windowsservercore-ltsc2016', + [string]$ConnectionString = $env:SNICLOSE_CONNSTR, + # Container-reachable host IP that the loopback SQL Server is bridged to. + # Defaults to the Docker NAT gateway (the host's 'vEthernet (nat)' adapter). + [string]$SqlHost, + [string]$RunnerVersion = '2.9.3', + # Constrain the hyperv container VM to this many processors (0 = unconstrained). + # The ICM environment had 4 procs; races can be CPU-count sensitive. + [int]$Cpus = 0, + [switch]$SkipBuild +) + +$ErrorActionPreference = 'Stop' +$projDir = $PSScriptRoot +$proj = Join-Path $projDir 'SniCloseLegacyRepro.csproj' + +# --- Build the net47 output (host-side; independent of the docker engine) ------ +if (-not $SkipBuild) { + Write-Host "Building $Framework ..." -ForegroundColor Cyan + dotnet build $proj -c Debug -f $Framework | Out-Host + if ($LASTEXITCODE -ne 0) { throw "Build failed." } +} + +$outDir = Join-Path $projDir "bin\Debug\$Framework" +if (-not (Test-Path (Join-Path $outDir 'SniCloseLegacyRepro.dll'))) { + throw "Build output not found: $outDir (build first, or omit -SkipBuild)." +} + +# --- Stage xunit.console.exe next to the test assembly ------------------------- +# The framework container has no dotnet CLI, so run tests with the console runner. +$gp = (dotnet nuget locals global-packages --list) -replace '^.*?:\s*', '' +$runnerTools = Join-Path $gp "xunit.runner.console\$RunnerVersion\tools\$Framework" +if (-not (Test-Path $runnerTools)) { + $runnerTools = Join-Path $gp "xunit.runner.console\$RunnerVersion\tools\net472" +} +if (-not (Test-Path (Join-Path $runnerTools 'xunit.console.exe'))) { + throw "xunit.console.exe not found under $runnerTools. Is xunit.runner.console $RunnerVersion restored?" +} +Copy-Item (Join-Path $runnerTools '*') $outDir -Force + +# --- Ensure Docker is on the Windows engine ----------------------------------- +$null = docker context use desktop-windows 2>$null +$serverOs = docker version --format '{{.Server.Os}}' 2>$null +if ($serverOs -ne 'windows') { + throw "Docker is not on the Windows engine (Server OS='$serverOs'). Switch with: & 'C:\Program Files\Docker\Docker\DockerCli.exe' -SwitchWindowsEngine" +} + +# --- Build the container args -------------------------------------------------- +# A Windows container cannot reach the host's loopback SQL Server directly, and +# host.docker.internal is not injected for hyperv-isolated Windows containers. +# Instead we target the Docker NAT gateway IP (the host's 'vEthernet (nat)' +# adapter); a one-time elevated host portproxy must bridge that IP to loopback: +# netsh interface portproxy add v4tov4 listenaddress= listenport=1434 ` +# connectaddress=127.0.0.1 connectport=1434 +$envArgs = @() +if ($ConnectionString) { + if (-not $SqlHost) { + $SqlHost = Get-NetIPAddress -InterfaceAlias 'vEthernet (nat)' -AddressFamily IPv4 -ErrorAction SilentlyContinue | + Select-Object -First 1 -ExpandProperty IPAddress + } + if (-not $SqlHost) { + throw "Could not determine a container-reachable host IP; pass -SqlHost explicitly." + } + $cs = $ConnectionString ` + -replace '127\.0\.0\.1', $SqlHost ` + -replace '(?i)(?<=[=;\s])localhost(?=[,;]|$)', $SqlHost + $envArgs = @('-e', "SNICLOSE_CONNSTR=$cs") + Write-Host "Live tests enabled (SQL Server via $SqlHost; requires a host portproxy to 127.0.0.1)." -ForegroundColor Cyan +} +else { + Write-Host "No connection string: live MARS + stress tests will be skipped." -ForegroundColor Yellow +} + +Write-Host "Running tests in $Image (isolation=hyperv) ..." -ForegroundColor Cyan +$cpuArgs = @() +if ($Cpus -gt 0) { + $cpuArgs = @('--cpu-count', "$Cpus") + Write-Host "Container VM constrained to $Cpus processor(s)." -ForegroundColor Cyan +} +docker run --rm --isolation=hyperv @cpuArgs @envArgs -v "${outDir}:C:\app" -w 'C:\app' $Image ` + C:\app\xunit.console.exe C:\app\SniCloseLegacyRepro.dll -parallel none + +$code = $LASTEXITCODE +Write-Host "xunit.console.exe exit code: $code" -ForegroundColor $(if ($code -eq 0) { 'Green' } else { 'Red' }) +exit $code From 87d5885477a998b3afda74895dc65393eadf18e9 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:49:21 -0300 Subject: [PATCH 08/10] Add exploratory Tier-2 reproduction attempts to the SNIClose harness Add tools/SniCloseLegacyRepro/ReproAttempts.cs: harness-only attempts targeting conditions the shared SNIClose/MARS tests do not create - close under thread-pool starvation, sync-over-async on a single-threaded SynchronizationContext, cancelling OpenAsync during the TLS handshake, and Open() under client thread-pool starvation. None reproduces the issue on the exact reported bits (System.Data.dll 4.7.4081.0, 4 CPUs). Note: an in-process TDS test server shares the client thread pool, so saturating it starves the server (a test artifact); against a real out-of-process server Open() is robust under starvation. README Findings updated accordingly. Refs ICM 775308542 / ADO.Net #43847. --- tools/SniCloseLegacyRepro/README.md | 19 + tools/SniCloseLegacyRepro/ReproAttempts.cs | 595 +++++++++++++++++++++ 2 files changed, 614 insertions(+) create mode 100644 tools/SniCloseLegacyRepro/ReproAttempts.cs diff --git a/tools/SniCloseLegacyRepro/README.md b/tools/SniCloseLegacyRepro/README.md index 4b5ae54683..441e5b836b 100644 --- a/tools/SniCloseLegacyRepro/README.md +++ b/tools/SniCloseLegacyRepro/README.md @@ -236,6 +236,25 @@ from a plain client-side ordering bug reachable from managed timing alone, and toward something more environment-specific in the original incident (e.g. exact network/timing conditions, or a native-layer interaction not exercised here). +### Additional reproduction attempts (`ReproAttempts.cs`) + +Harness-only, exploratory variants targeting conditions the shared tests do not +create. On the exact 4.7.4081.0 bits at 4 CPUs, **none reproduces the issue**: + +- **Close under thread-pool starvation** → no deadlock. +- **Sync-over-async on a single-threaded `SynchronizationContext`** (classic + ASP.NET shape) → no captured-context deadlock. +- **Cancel `OpenAsync` during the TLS handshake** (internal abort/close path) + → settles promptly, no deadlock. +- **`Open()` under client thread-pool starvation** (real, out-of-process server; + plain and TLS) → **`Open()` succeeds**. Starvation is *not* the reproduction. + +> A note on a false lead: an *in-process* TDS test server shares the client's +> thread pool, so saturating the pool starves the **server** and makes `Open()` +> time out - a test artifact. Against a real out-of-process server the driver's +> `Open()` is robust under the same starvation, so the timeout does not indicate +> a driver defect. + --- ## Isolation notes diff --git a/tools/SniCloseLegacyRepro/ReproAttempts.cs b/tools/SniCloseLegacyRepro/ReproAttempts.cs new file mode 100644 index 0000000000..73bd2e4a41 --- /dev/null +++ b/tools/SniCloseLegacyRepro/ReproAttempts.cs @@ -0,0 +1,595 @@ +// 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. + +#nullable enable + +// ----------------------------------------------------------------------------- +// Tier-2 reproduction ATTEMPTS for ICM 775308542 / ADO.Net #43847 (harness-only, +// exploratory - not promoted to the driver test suite unless one reproduces). +// +// The shared SNIClose/MARS tests conclusively show that a single connection +// closed while one async I/O is in flight does NOT deadlock, even on the +// customer's exact in-box System.Data.dll (4.7.4081.0) at 4 CPUs. These variants +// target conditions those tests do not create, which better match the reported +// symptom ("initial TLS handshake failure" under a 4-proc production load): +// +// 1. ThreadPoolStarvation - the worker pool is saturated (as on a small, +// busy box) when the connection is closed, so an async completion callback +// cannot obtain a thread while the close path runs. +// 2. SyncOverAsyncContext - a single-threaded SynchronizationContext (as in +// classic ASP.NET) is blocked on an async SqlClient operation via .Result; +// if the driver captures the context for a continuation, it deadlocks. +// 3. CancelOpenDuringTls - OpenAsync is cancelled while the TLS handshake +// read is in flight, driving the driver's INTERNAL abort/close path +// (rather than an external Close on another thread). +// 4. OpenUnderStarvation - does client thread-pool starvation cause a +// connection/handshake failure? Against a REAL server it does NOT: the +// driver's Open() is robust. (An in-process test server shares the pool +// and would be co-starved - a test artifact, not driver behavior.) +// +// All waits are bounded, so a genuine deadlock is reported as a failed +// bounded-wait rather than hanging the run. +// ----------------------------------------------------------------------------- + +using System; +using System.Collections.Concurrent; +using System.Net; +using System.Net.Sockets; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.SqlServer.TDS; +using Microsoft.SqlServer.TDS.EndPoint; +using Microsoft.SqlServer.TDS.Servers; +using Microsoft.SqlServer.TDS.SQLBatch; +using Microsoft.Data.SqlClient.ManualTesting.Tests; // DataTestUtility shim (live connection string) +using Xunit; + +namespace SniCloseLegacyRepro +{ + /// Shared helpers for the reproduction attempts. + internal static class ReproSupport + { + public static readonly TimeSpan CloseBudget = TimeSpan.FromSeconds(30); + public static readonly TimeSpan HandshakeBudget = TimeSpan.FromSeconds(30); + + /// + /// A query engine that signals when a SQL batch arrives and then withholds + /// the response until released. + /// + public sealed class StallingQueryEngine : QueryEngine + { + private readonly ManualResetEventSlim _batchReceived; + private readonly ManualResetEventSlim _release; + + public StallingQueryEngine( + TdsServerArguments arguments, + ManualResetEventSlim batchReceived, + ManualResetEventSlim release) + : base(arguments) + { + _batchReceived = batchReceived; + _release = release; + } + + protected override TDSMessageCollection CreateQueryResponse( + ITDSServerSession session, + TDSSQLBatchToken batchRequest) + { + _batchReceived.Set(); + _release.Wait(); + return base.CreateQueryResponse(session, batchRequest); + } + } + + /// + /// A minimal, protocol-valid TDS PRELOGIN response advertising ENCRYPT_ON + /// so a client that requested encryption proceeds into the TLS handshake. + /// + public static readonly byte[] PreLoginEncryptOnResponse = + { + 0x12, 0x01, 0x00, 0x1A, 0x00, 0x00, 0x01, 0x00, + 0x00, 0x00, 0x0B, 0x00, 0x06, + 0x01, 0x00, 0x11, 0x00, 0x01, + 0xFF, + 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x01, + }; + } + + /// + /// A single-threaded synchronization context that pumps posted callbacks on + /// one dedicated (background) thread - the shape that makes classic ASP.NET + /// sync-over-async deadlock. + /// + internal sealed class SingleThreadedSynchronizationContext : SynchronizationContext, IDisposable + { + private readonly BlockingCollection<(SendOrPostCallback callback, object? state)> _queue = new(); + private readonly Thread _thread; + + public SingleThreadedSynchronizationContext() + { + _thread = new Thread(Pump) { IsBackground = true, Name = "SingleThreadedSyncCtx" }; + _thread.Start(); + } + + private void Pump() + { + SetSynchronizationContext(this); + try + { + foreach ((SendOrPostCallback callback, object? state) in _queue.GetConsumingEnumerable()) + { + callback(state); + } + } + catch (InvalidOperationException) + { + // Queue completed while blocked; normal shutdown. + } + } + + public override void Post(SendOrPostCallback d, object? state) => _queue.Add((d, state)); + + public override void Send(SendOrPostCallback d, object? state) => + throw new NotSupportedException(); + + /// Queue an action to run on the context thread. + public void Run(Action action) => Post(_ => action(), null); + + public void Dispose() + { + try + { + _queue.CompleteAdding(); + } + catch + { + // Ignore. + } + } + } + + /// + /// Attempt 1: close a connection with an in-flight async read while the + /// worker thread pool is fully saturated, so an async completion callback + /// cannot obtain a thread while the close path runs. + /// + public class ThreadPoolStarvationReproTests + { + [Theory] + [InlineData(false)] + [InlineData(true)] + public void CloseWithPendingReadUnderThreadPoolStarvation_DoesNotDeadlock(bool disposeInsteadOfClose) + { + ThreadPool.GetMinThreads(out int minW, out int minIo); + ThreadPool.GetMaxThreads(out int maxW, out int maxIo); + + using ManualResetEventSlim releaseWorkers = new(false); + using ManualResetEventSlim batchReceived = new(false); + using ManualResetEventSlim releaseResponse = new(false); + int busy = 0; + bool poolConstrained = false; + try + { + TdsServerArguments arguments = new(); + using TdsServer server = new( + new ReproSupport.StallingQueryEngine(arguments, batchReceived, releaseResponse), + arguments); + server.Start(); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"localhost,{server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + SqlCommand? command = null; + Task? readTask = null; + bool closeAttempted = false; + bool closedInTime = false; + try + { + // Open and start the read while the pool is healthy, so the + // starvation below isolates the CLOSE path (not connection + // establishment, which is starvation-sensitive on its own). + connection.Open(); + command = new("SELECT 1", connection); + readTask = command.ExecuteReaderAsync(); + + Assert.True( + batchReceived.Wait(ReproSupport.HandshakeBudget), + "The server never received the batch; the async read was not in flight."); + + // Now saturate a small, busy-box worker pool so any async + // completion the close path depends on cannot obtain a thread. + int poolSize = Math.Max(2, Math.Min(4, Environment.ProcessorCount)); + ThreadPool.SetMinThreads(poolSize, poolSize); + ThreadPool.SetMaxThreads(poolSize, poolSize); + poolConstrained = true; + for (int i = 0; i < poolSize; i++) + { + ThreadPool.QueueUserWorkItem(_ => + { + Interlocked.Increment(ref busy); + releaseWorkers.Wait(); + }); + } + SpinWait.SpinUntil(() => Volatile.Read(ref busy) >= poolSize, TimeSpan.FromSeconds(10)); + + closeAttempted = true; + Task closeTask = Task.Factory.StartNew( + () => + { + if (disposeInsteadOfClose) + { + connection.Dispose(); + } + else + { + connection.Close(); + } + }, + TaskCreationOptions.LongRunning); + + closedInTime = closeTask.Wait(ReproSupport.CloseBudget); + + Assert.True( + closedInTime, + $"{(disposeInsteadOfClose ? "Dispose()" : "Close()")} did not complete within " + + $"{ReproSupport.CloseBudget.TotalSeconds:N0}s while the thread pool was saturated " + + "(possible SNIClose deadlock / thread-starvation)."); + } + finally + { + // Free the pool first so the pending read and any close-path + // continuations can drain, then release the server. + releaseWorkers.Set(); + if (poolConstrained) + { + ThreadPool.SetMinThreads(minW, minIo); + ThreadPool.SetMaxThreads(maxW, maxIo); + } + releaseResponse.Set(); + + if (readTask != null) + { + try { readTask.Wait(ReproSupport.CloseBudget); } catch { /* expected */ } + } + command?.Dispose(); + if (!closeAttempted || closedInTime) + { + connection.Dispose(); + } + } + } + finally + { + releaseWorkers.Set(); + ThreadPool.SetMinThreads(minW, minIo); + ThreadPool.SetMaxThreads(maxW, maxIo); + } + } + } + + /// + /// Attempt 2: block a single-threaded synchronization context on an async + /// SqlClient read via .Result. If any driver continuation captures the + /// context, the post-back cannot run and the operation deadlocks. + /// + public class SyncOverAsyncContextReproTests + { + [Fact] + public void SyncOverAsyncOnSingleThreadedContext_DoesNotDeadlock() + { + using ManualResetEventSlim batchReceived = new(false); + using ManualResetEventSlim releaseResponse = new(false); + using ManualResetEventSlim scenarioDone = new(false); + Exception? scenarioError = null; + + TdsServerArguments arguments = new(); + using TdsServer server = new( + new ReproSupport.StallingQueryEngine(arguments, batchReceived, releaseResponse), + arguments); + server.Start(); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"localhost,{server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + // Once the read is in flight, release the server so its completion + // fires and the continuation must post back to the (blocked) context. + Task releaser = Task.Run(() => + { + if (batchReceived.Wait(ReproSupport.HandshakeBudget)) + { + releaseResponse.Set(); + } + }); + + using SingleThreadedSynchronizationContext context = new(); + SqlConnection? connection = null; + context.Run(() => + { + try + { + connection = new SqlConnection(builder.ConnectionString); + connection.Open(); + using SqlCommand command = new("SELECT 1", connection); + + // Sync-over-async on the single-threaded context: blocks the + // pump thread. Deadlocks iff a continuation captured the context. + using SqlDataReader reader = command.ExecuteReaderAsync().GetAwaiter().GetResult(); + while (reader.Read()) + { + // Drain. + } + } + catch (Exception ex) + { + scenarioError = ex; + } + finally + { + scenarioDone.Set(); + } + }); + + bool completed = scenarioDone.Wait(ReproSupport.CloseBudget); + + // Best-effort cleanup; if we deadlocked, the context thread is parked + // (background) and the connection is stuck - do not block on it. + try { releaser.Wait(TimeSpan.FromSeconds(5)); } catch { /* ignore */ } + if (completed) + { + try { connection?.Dispose(); } catch { /* ignore */ } + } + + Assert.True( + completed, + $"Sync-over-async on a single-threaded SynchronizationContext did not complete within " + + $"{ReproSupport.CloseBudget.TotalSeconds:N0}s. This indicates a captured-context deadlock in " + + "the driver's async read path (ADO.Net #43847 / ICM 775308542)."); + + // If it completed, it should have completed cleanly (or with an + // expected SqlException), not via some other failure mode. + Assert.True( + scenarioError is null or System.Data.SqlClient.SqlException, + $"Unexpected error on the context thread: {scenarioError}"); + } + } + + /// + /// Attempt 3: cancel OpenAsync while the TLS handshake read is in + /// flight, exercising the driver's INTERNAL abort/close path concurrently + /// with the handshake I/O (rather than an external Close on another thread). + /// + public class HandshakeCancellationReproTests + { + [Fact] + public void CancelOpenAsyncDuringTlsHandshake_DoesNotDeadlock() + { + using ManualResetEventSlim handshakeInFlight = new(false); + using ManualResetEventSlim releaseServer = new(false); + + TcpListener listener = new(IPAddress.Loopback, 0); + listener.Start(); + int port = ((IPEndPoint)listener.LocalEndpoint).Port; + + Task serverTask = Task.Run(() => + { + using TcpClient acceptedClient = listener.AcceptTcpClient(); + using NetworkStream stream = acceptedClient.GetStream(); + byte[] buffer = new byte[4096]; + try + { + int read = stream.Read(buffer, 0, buffer.Length); + if (read <= 0) + { + return; + } + + stream.Write(ReproSupport.PreLoginEncryptOnResponse, 0, ReproSupport.PreLoginEncryptOnResponse.Length); + stream.Flush(); + + // Read the first byte of the client's TLS ClientHello, then + // withhold the ServerHello so the handshake read stays pending. + if (stream.ReadByte() < 0) + { + return; + } + + handshakeInFlight.Set(); + releaseServer.Wait(); + } + catch + { + // Client may tear down mid-handshake; ignore. + } + }); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"127.0.0.1,{port}", + Encrypt = SqlConnectionEncryptOption.Mandatory, + TrustServerCertificate = true, + ConnectTimeout = 60, + ConnectRetryCount = 0, + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + using CancellationTokenSource cts = new(); + Task? openTask = null; + bool cancelAttempted = false; + bool settledInTime = false; + try + { + openTask = connection.OpenAsync(cts.Token); + + Assert.True( + handshakeInFlight.Wait(ReproSupport.HandshakeBudget), + "The client never reached the TLS handshake; no read was in flight to cancel."); + + // Cancel while the handshake read is pending: this drives the + // driver's internal abort/close path against the in-flight I/O. + cancelAttempted = true; + cts.Cancel(); + + try + { + settledInTime = openTask.Wait(ReproSupport.CloseBudget); + } + catch (AggregateException) + { + // OpenAsync faulting/cancelling is the expected outcome; what + // matters is that it *settled* rather than hanging. + settledInTime = true; + } + + Assert.True( + settledInTime, + $"OpenAsync did not settle within {ReproSupport.CloseBudget.TotalSeconds:N0}s after " + + "cancellation while a TLS handshake read was in flight (possible SNIClose deadlock)."); + } + finally + { + releaseServer.Set(); + listener.Stop(); + + if (openTask != null) + { + try { openTask.Wait(ReproSupport.CloseBudget); } catch { /* expected fault/cancel */ } + } + + if (!cancelAttempted || settledInTime) + { + connection.Dispose(); + } + + try { serverTask.Wait(ReproSupport.CloseBudget); } catch { /* ignore */ } + } + } + } + + /// + /// Attempt 4: does client-side thread-pool starvation cause a CONNECTION + /// failure? This checks the hypothesis that the reported "connection/ + /// handshake failure under load" is thread starvation rather than an + /// SNIClose deadlock. + /// + /// + /// IMPORTANT: an in-process TDS test server shares the client's thread pool, + /// so saturating the pool starves the SERVER and makes Open() time out - a + /// test artifact, not driver behavior. These tests therefore require a real, + /// out-of-process SQL Server (SNICLOSE_CONNSTR). The observed result is that + /// Open() SUCCEEDS under client thread-pool starvation, i.e. starvation is + /// NOT the reproduction. + /// + /// + public class OpenUnderThreadPoolStarvationReproTests + { + private static readonly TimeSpan HardBudget = TimeSpan.FromSeconds(60); + + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + [InlineData(false)] // Encrypt = Optional + [InlineData(true)] // Encrypt = Mandatory (drives the TLS handshake) + public void SyncOpenAgainstRealServerUnderThreadPoolStarvation_Succeeds(bool encrypt) + { + string cs = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + Encrypt = encrypt ? SqlConnectionEncryptOption.Mandatory : SqlConnectionEncryptOption.Optional, + TrustServerCertificate = true, + Pooling = false, + ConnectTimeout = 30, + }.ConnectionString; + + RunStarvedOpen(cs, out bool settled, out bool timedOut, out Exception? error); + + Assert.True( + settled, + $"Open() did not settle within {HardBudget.TotalSeconds:N0}s under thread-pool starvation " + + "(it hung)."); + Assert.True( + error is null, + $"Open() against a real server failed under client thread-pool starvation: {error}"); + Assert.False( + timedOut, + "Open() timed out under client thread-pool starvation against a real server."); + } + + private static void RunStarvedOpen(string connectionString, out bool settled, out bool timedOut, out Exception? error) + { + ThreadPool.GetMinThreads(out int minW, out int minIo); + ThreadPool.GetMaxThreads(out int maxW, out int maxIo); + // Not disposed: saturating workers may still be parked on it when the + // method returns; disposing would race them into ObjectDisposedException. + ManualResetEventSlim releaseWorkers = new(false); + int busy = 0; + settled = false; + timedOut = false; + error = null; + try + { + // Constrain the pool to a small, busy-box size and occupy every + // worker thread so any continuation the driver needs is starved. + int poolSize = Math.Max(2, Math.Min(4, Environment.ProcessorCount)); + ThreadPool.SetMinThreads(poolSize, poolSize); + ThreadPool.SetMaxThreads(poolSize, poolSize); + for (int i = 0; i < poolSize; i++) + { + ThreadPool.QueueUserWorkItem(_ => + { + Interlocked.Increment(ref busy); + releaseWorkers.Wait(); + }); + } + SpinWait.SpinUntil(() => Volatile.Read(ref busy) >= poolSize, TimeSpan.FromSeconds(10)); + + Exception? captured = null; + Thread openThread = new(() => + { + try + { + using SqlConnection conn = new(connectionString); + conn.Open(); + conn.Close(); + } + catch (Exception ex) + { + captured = ex; + } + }) + { IsBackground = true, Name = "StarvedOpen" }; + + openThread.Start(); + settled = openThread.Join(HardBudget); + error = captured; + timedOut = captured is System.Data.SqlClient.SqlException sqlEx + && (sqlEx.Number == -2 + || sqlEx.Message.IndexOf("timeout", StringComparison.OrdinalIgnoreCase) >= 0 + || sqlEx.Message.IndexOf("timed out", StringComparison.OrdinalIgnoreCase) >= 0); + } + finally + { + releaseWorkers.Set(); + ThreadPool.SetMinThreads(minW, minIo); + ThreadPool.SetMaxThreads(maxW, maxIo); + } + } + } +} From 3f2b3f4e0ed349412d222852f8fec978f7be3cab Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:11:39 -0300 Subject: [PATCH 09/10] Promote handshake-cancellation test; harden SNIClose harness for CI - Promote CancelOpenAsyncDuringTlsHandshake_DoesNotDeadlock from the legacy repro harness into the real driver UnitTests suite (SimulatedServerTests/SNICloseHandshakeCancellationTest.cs). The harness now links and runs the driver copy instead of a private one. - Disable xUnit test parallelization in the harness (AssemblyInfo.cs). The ThreadPool-starvation repro tests mutate the global ThreadPool and were starving other tests connections under parallel execution; this matches the container runner (-parallel none) and makes runs deterministic. - Gate the re-entrant SNIClose soak behind SNICLOSE_SOAK_ENABLE (opt-in). It fired once on the legacy in-box netfx driver (not on the Core lineage) but is non-deterministic, so it is skipped by default to keep CI clean. - Forward soak env vars (workers/seconds) through run-in-container.ps1. --- .../SNICloseHandshakeCancellationTest.cs | 180 +++++ tools/SniCloseLegacyRepro/AssemblyInfo.cs | 12 + tools/SniCloseLegacyRepro/ReproAttempts.cs | 632 ++++++++++++++---- .../SniCloseLegacyRepro.csproj | 2 + .../SniCloseLegacyRepro/run-in-container.ps1 | 6 + 5 files changed, 701 insertions(+), 131 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseHandshakeCancellationTest.cs create mode 100644 tools/SniCloseLegacyRepro/AssemblyInfo.cs diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseHandshakeCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseHandshakeCancellationTest.cs new file mode 100644 index 0000000000..9a41c2f3c7 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseHandshakeCancellationTest.cs @@ -0,0 +1,180 @@ +// 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. + +#nullable enable + +using System; +using System.Net; +using System.Net.Sockets; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; + +/// +/// Regression test for the connection-teardown paths related to ADO.Net #43847 / +/// ICM 775308542. Where tears the connection +/// down with an external Close()/Dispose(), this drives the +/// driver's internal abort/close path by cancelling OpenAsync while +/// the TLS handshake read is in flight. +/// +/// +/// A bare TCP listener completes just enough of the pre-login exchange to push +/// the client into the TLS handshake (it advertises ENCRYPT_ON, reads the +/// client's ClientHello, then withholds the ServerHello). The client's +/// OpenAsync is then cancelled while that handshake read is pending. The +/// cancellation must abort the in-flight I/O and complete the operation promptly; +/// a deadlock in the internal close path manifests as the bounded wait timing +/// out, which fails the test. +/// +/// +public class SNICloseHandshakeCancellationTest +{ + /// + /// Upper bound for how long the cancelled OpenAsync should take to settle. + /// Generous so it only distinguishes "completed" from "deadlocked". + /// + private static readonly TimeSpan CloseBudget = TimeSpan.FromSeconds(30); + + /// + /// Upper bound for waiting until the client's TLS handshake read is pending. + /// + private static readonly TimeSpan HandshakeBudget = TimeSpan.FromSeconds(30); + + /// + /// A minimal, protocol-valid TDS PRELOGIN response advertising ENCRYPT_ON so + /// a client that requested encryption proceeds into the TLS handshake. + /// + private static readonly byte[] s_preLoginEncryptOnResponse = + { + // ---- TDS packet header (8 bytes) ---- + 0x12, // Type: PRELOGIN + 0x01, // Status: EOM + 0x00, 0x1A, // Length: 26 (8 header + 18 payload), big-endian + 0x00, 0x00, // SPID + 0x01, // PacketID + 0x00, // Window + // ---- PRELOGIN option table ---- + 0x00, 0x00, 0x0B, 0x00, 0x06, // VERSION: offset 11, length 6 + 0x01, 0x00, 0x11, 0x00, 0x01, // ENCRYPTION: offset 17, length 1 + 0xFF, // TERMINATOR + // ---- Option data ---- + 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, // VERSION 17.0.0.0 + 0x01, // ENCRYPTION = ENCRYPT_ON + }; + + [Fact] + public void CancelOpenAsyncDuringTlsHandshake_DoesNotDeadlock() + { + using ManualResetEventSlim handshakeInFlight = new(false); + using ManualResetEventSlim releaseServer = new(false); + + TcpListener listener = new(IPAddress.Loopback, 0); + listener.Start(); + int port = ((IPEndPoint)listener.LocalEndpoint).Port; + + // Server: complete the pre-login exchange advertising encryption, read the + // client's TLS ClientHello, then withhold the ServerHello so the client's + // handshake read stays pending. + Task serverTask = Task.Run(() => + { + using TcpClient acceptedClient = listener.AcceptTcpClient(); + using NetworkStream stream = acceptedClient.GetStream(); + byte[] buffer = new byte[4096]; + try + { + int read = stream.Read(buffer, 0, buffer.Length); + if (read <= 0) + { + return; + } + + stream.Write(s_preLoginEncryptOnResponse, 0, s_preLoginEncryptOnResponse.Length); + stream.Flush(); + + // A byte here proves the client entered the TLS handshake and is + // awaiting the ServerHello with its read pending. + if (stream.ReadByte() < 0) + { + return; + } + + handshakeInFlight.Set(); + releaseServer.Wait(); + } + catch + { + // The client may tear down the socket mid-handshake; ignore. + } + }); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"127.0.0.1,{port}", + Encrypt = SqlConnectionEncryptOption.Mandatory, + TrustServerCertificate = true, + ConnectTimeout = 60, + ConnectRetryCount = 0, + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + using CancellationTokenSource cts = new(); + Task? openTask = null; + bool cancelAttempted = false; + bool settledInTime = false; + try + { + openTask = connection.OpenAsync(cts.Token); + + Assert.True( + handshakeInFlight.Wait(HandshakeBudget), + "The client never reached the TLS handshake; no read was in flight to cancel."); + + // Cancel while the handshake read is pending: this drives the driver's + // internal abort/close path against the in-flight I/O. + cancelAttempted = true; + cts.Cancel(); + + try + { + settledInTime = openTask.Wait(CloseBudget); + } + catch (AggregateException) + { + // OpenAsync faulting/cancelling is the expected outcome; what + // matters is that it *settled* rather than hanging. + settledInTime = true; + } + + Assert.True( + settledInTime, + $"OpenAsync did not settle within {CloseBudget.TotalSeconds:N0}s after cancellation while a " + + "TLS handshake read was in flight. This indicates a deadlock in the internal close path " + + "(ADO.Net #43847 / ICM 775308542)."); + } + finally + { + releaseServer.Set(); + listener.Stop(); + + if (openTask != null) + { + try { openTask.Wait(CloseBudget); } catch { /* expected fault/cancel */ } + } + + // Dispose unless a deadlock was detected (Dispose could also block). + if (!cancelAttempted || settledInTime) + { + connection.Dispose(); + } + + try { serverTask.Wait(CloseBudget); } catch { /* ignore */ } + } + } +} diff --git a/tools/SniCloseLegacyRepro/AssemblyInfo.cs b/tools/SniCloseLegacyRepro/AssemblyInfo.cs new file mode 100644 index 0000000000..a243838e3d --- /dev/null +++ b/tools/SniCloseLegacyRepro/AssemblyInfo.cs @@ -0,0 +1,12 @@ +// 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 Xunit; + +// This diagnostic harness includes tests that mutate PROCESS-GLOBAL state (e.g. +// ThreadPool.SetMin/MaxThreads in the starvation experiments) and heavy soak +// tests. Running collections in parallel lets those perturb unrelated tests +// (starving their connections), so we run serially - matching the container +// runner, which uses `xunit.console.exe -parallel none`. +[assembly: CollectionBehavior(DisableTestParallelization = true)] diff --git a/tools/SniCloseLegacyRepro/ReproAttempts.cs b/tools/SniCloseLegacyRepro/ReproAttempts.cs index 73bd2e4a41..e2107616d5 100644 --- a/tools/SniCloseLegacyRepro/ReproAttempts.cs +++ b/tools/SniCloseLegacyRepro/ReproAttempts.cs @@ -22,18 +22,40 @@ // if the driver captures the context for a continuation, it deadlocks. // 3. CancelOpenDuringTls - OpenAsync is cancelled while the TLS handshake // read is in flight, driving the driver's INTERNAL abort/close path -// (rather than an external Close on another thread). +// (rather than an external Close on another thread). PROMOTED to the driver +// test suite as SNICloseHandshakeCancellationTest (linked in via the +// harness csproj), so it also runs against the legacy driver here. // 4. OpenUnderStarvation - does client thread-pool starvation cause a // connection/handshake failure? Against a REAL server it does NOT: the // driver's Open() is robust. (An in-process test server shares the pool // and would be co-starved - a test artifact, not driver behavior.) // +// After the ICM dump was obtained, the real mechanism was identified: a +// RE-ENTRANT SNIClose (Close() called synchronously from within +// ReadAsyncCallback via the command-timeout -> failed-attention-write path, so +// SNIClose spins in WaitForActiveCallbacks waiting for the callback it runs in). +// Two further attempts target that mechanism: +// +// 5. CallbackReentrancy - async read + short command timeout + a proxy that +// breaks the client's write. A regression guard for the MDS asyncClose fix; +// does not deterministically force the legacy re-entrancy (see its notes). +// 6. ReentrantSNICloseSoak - many concurrent workers RST connections at ~the +// command-timeout instant, hunting the race probabilistically. OPT-IN +// (set SNICLOSE_SOAK_ENABLE) because it is heavy and, when it fires, poisons +// the process. It fired ONCE in a full-suite run on the legacy in-box driver +// (net462/47x/48x) but NOT on the Core NuGet lineage (net10.0) - a clean +// driver-lineage split consistent with the real bug - yet it did not recur +// in subsequent runs. So it confirms the mechanism is reachable but is not a +// reliable reproduction (matching the SQL EE's "very difficult to capture"). +// The definitive evidence remains the dump + the asyncClose code review. +// // All waits are bounded, so a genuine deadlock is reported as a failed // bounded-wait rather than hanging the run. // ----------------------------------------------------------------------------- using System; using System.Collections.Concurrent; +using System.Linq; using System.Net; using System.Net.Sockets; using System.Threading; @@ -81,20 +103,6 @@ protected override TDSMessageCollection CreateQueryResponse( return base.CreateQueryResponse(session, batchRequest); } } - - /// - /// A minimal, protocol-valid TDS PRELOGIN response advertising ENCRYPT_ON - /// so a client that requested encryption proceeds into the TLS handshake. - /// - public static readonly byte[] PreLoginEncryptOnResponse = - { - 0x12, 0x01, 0x00, 0x1A, 0x00, 0x00, 0x01, 0x00, - 0x00, 0x00, 0x0B, 0x00, 0x06, - 0x01, 0x00, 0x11, 0x00, 0x01, - 0xFF, - 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x01, - }; } /// @@ -370,122 +378,6 @@ public void SyncOverAsyncOnSingleThreadedContext_DoesNotDeadlock() } } - /// - /// Attempt 3: cancel OpenAsync while the TLS handshake read is in - /// flight, exercising the driver's INTERNAL abort/close path concurrently - /// with the handshake I/O (rather than an external Close on another thread). - /// - public class HandshakeCancellationReproTests - { - [Fact] - public void CancelOpenAsyncDuringTlsHandshake_DoesNotDeadlock() - { - using ManualResetEventSlim handshakeInFlight = new(false); - using ManualResetEventSlim releaseServer = new(false); - - TcpListener listener = new(IPAddress.Loopback, 0); - listener.Start(); - int port = ((IPEndPoint)listener.LocalEndpoint).Port; - - Task serverTask = Task.Run(() => - { - using TcpClient acceptedClient = listener.AcceptTcpClient(); - using NetworkStream stream = acceptedClient.GetStream(); - byte[] buffer = new byte[4096]; - try - { - int read = stream.Read(buffer, 0, buffer.Length); - if (read <= 0) - { - return; - } - - stream.Write(ReproSupport.PreLoginEncryptOnResponse, 0, ReproSupport.PreLoginEncryptOnResponse.Length); - stream.Flush(); - - // Read the first byte of the client's TLS ClientHello, then - // withhold the ServerHello so the handshake read stays pending. - if (stream.ReadByte() < 0) - { - return; - } - - handshakeInFlight.Set(); - releaseServer.Wait(); - } - catch - { - // Client may tear down mid-handshake; ignore. - } - }); - - SqlConnectionStringBuilder builder = new() - { - DataSource = $"127.0.0.1,{port}", - Encrypt = SqlConnectionEncryptOption.Mandatory, - TrustServerCertificate = true, - ConnectTimeout = 60, - ConnectRetryCount = 0, - Pooling = false, -#if NETFRAMEWORK - TransparentNetworkIPResolution = false, -#endif - }; - - SqlConnection connection = new(builder.ConnectionString); - using CancellationTokenSource cts = new(); - Task? openTask = null; - bool cancelAttempted = false; - bool settledInTime = false; - try - { - openTask = connection.OpenAsync(cts.Token); - - Assert.True( - handshakeInFlight.Wait(ReproSupport.HandshakeBudget), - "The client never reached the TLS handshake; no read was in flight to cancel."); - - // Cancel while the handshake read is pending: this drives the - // driver's internal abort/close path against the in-flight I/O. - cancelAttempted = true; - cts.Cancel(); - - try - { - settledInTime = openTask.Wait(ReproSupport.CloseBudget); - } - catch (AggregateException) - { - // OpenAsync faulting/cancelling is the expected outcome; what - // matters is that it *settled* rather than hanging. - settledInTime = true; - } - - Assert.True( - settledInTime, - $"OpenAsync did not settle within {ReproSupport.CloseBudget.TotalSeconds:N0}s after " + - "cancellation while a TLS handshake read was in flight (possible SNIClose deadlock)."); - } - finally - { - releaseServer.Set(); - listener.Stop(); - - if (openTask != null) - { - try { openTask.Wait(ReproSupport.CloseBudget); } catch { /* expected fault/cancel */ } - } - - if (!cancelAttempted || settledInTime) - { - connection.Dispose(); - } - - try { serverTask.Wait(ReproSupport.CloseBudget); } catch { /* ignore */ } - } - } - } - /// /// Attempt 4: does client-side thread-pool starvation cause a CONNECTION /// failure? This checks the hypothesis that the reported "connection/ @@ -592,4 +484,482 @@ private static void RunStarvedOpen(string connectionString, out bool settled, ou } } } + + /// + /// A transparent TCP proxy that forwards bytes between a client and a backend + /// TDS server, and can - on demand - break the CLIENT's write path while + /// leaving its pending read outstanding. It does this via + /// Shutdown(SocketShutdown.Receive) on the client-facing socket: the + /// client's next write (the timeout attention) then arrives at a + /// receive-shutdown socket and is answered with a TCP RST, so the client's + /// SNIWritePacket fails - the exact condition from the ICM dump. + /// + internal sealed class AttentionBreakingProxy : IDisposable + { + private readonly TcpListener _listener; + private readonly IPEndPoint _backend; + private Socket? _clientSocket; + private Socket? _backendSocket; + private volatile bool _broken; + + public int Port { get; } + + public AttentionBreakingProxy(IPEndPoint backend) + { + _backend = backend; + _listener = new TcpListener(IPAddress.Loopback, 0); + _listener.Start(); + Port = ((IPEndPoint)_listener.LocalEndpoint).Port; + Task.Run(AcceptAndPump); + } + + /// + /// Break the client's write direction: subsequent client writes RST while + /// its pending read stays outstanding (the backend sends nothing). + /// + public void BreakClientWrites() + { + _broken = true; + try + { + // SD_RECEIVE: data the client sends afterwards (the attention) is + // answered with a TCP RST, failing the client's write, while the + // client's pending read remains outstanding. + _clientSocket?.Shutdown(SocketShutdown.Receive); + } + catch + { + // best effort + } + } + + private void AcceptAndPump() + { + try + { + _clientSocket = _listener.AcceptSocket(); + _backendSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + _backendSocket.Connect(_backend); + + // client -> backend on a worker; backend -> client on this thread. + Task.Run(() => Pump(_clientSocket, _backendSocket, clientToBackend: true)); + Pump(_backendSocket, _clientSocket, clientToBackend: false); + } + catch + { + // Teardown races are expected; ignore. + } + } + + private void Pump(Socket from, Socket to, bool clientToBackend) + { + byte[] buffer = new byte[8192]; + try + { + while (true) + { + int n = from.Receive(buffer); + if (n <= 0) + { + break; + } + + // Once broken, stop forwarding client bytes to the backend; the + // client's write will RST on its receive-shutdown socket. + if (clientToBackend && _broken) + { + break; + } + + to.Send(buffer, 0, n, SocketFlags.None); + } + } + catch + { + // Expected on RST / teardown. + } + } + + public void Dispose() + { + try { _listener.Stop(); } catch { /* ignore */ } + try { _clientSocket?.Dispose(); } catch { /* ignore */ } + try { _backendSocket?.Dispose(); } catch { /* ignore */ } + } + } + + /// + /// Attempt 5 (matches the ICM dump's mechanism): the reported deadlock is a + /// RE-ENTRANT SNIClose. An async read times out; the driver sends an + /// attention from within ReadAsyncCallback; the attention + /// SNIWritePacket fails; the legacy driver then calls + /// SqlConnection.Close() SYNCHRONOUSLY on the callback thread, so + /// SNIClose spins in SNI_Conn::WaitForActiveCallbacks() waiting + /// for the very callback it is running within - a self-deadlock. + /// + /// + /// Microsoft.Data.SqlClient defers the close off the callback thread via its + /// asyncClose path (ThrowExceptionAndWarning wraps the close in + /// Task.Factory.StartNew), so the read settles promptly and this test + /// passes - a regression guard for that fix. + /// + /// + /// + /// NOTE: this exercises the async-read-timeout + broken-write path and asserts + /// no deadlock, but it does NOT deterministically force the re-entrant close + /// on the legacy driver: that requires the exact TCP state from the dump + /// (the attention write fails while the read stays pending - a half-broken + /// connection), which is very hard to synthesize (a RST kills both + /// directions; the SQL EE analysis likewise noted it is "very difficult to + /// capture"). The definitive evidence is the dump plus the code review of the + /// asyncClose deferral in Microsoft.Data.SqlClient. + /// + /// + public class SNICloseCallbackReentrancyReproTests + { + private static readonly TimeSpan SettleBudget = TimeSpan.FromSeconds(45); + private static readonly TimeSpan BatchBudget = TimeSpan.FromSeconds(30); + + [Fact] + public void CloseFromReadCallbackOnAttentionFailure_DoesNotDeadlock() + { + using ManualResetEventSlim batchReceived = new(false); + using ManualResetEventSlim releaseResponse = new(false); + + TdsServerArguments arguments = new(); + using TdsServer server = new( + new ReproSupport.StallingQueryEngine(arguments, batchReceived, releaseResponse), + arguments); + server.Start(); + + using AttentionBreakingProxy proxy = new(new IPEndPoint(IPAddress.Loopback, server.EndPoint.Port)); + + SqlConnectionStringBuilder builder = new() + { + DataSource = $"127.0.0.1,{proxy.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + // MARS off and pooling off, matching the reported configuration and + // ensuring Close() tears down the physical connection (reaching SNIClose). + Pooling = false, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }; + + SqlConnection connection = new(builder.ConnectionString); + SqlCommand? command = null; + Task? readTask = null; + bool settledInTime = false; + try + { + connection.Open(); + + // Short command timeout so the pending async read times out, which + // drives OnTimeoutCore -> SendAttention from within ReadAsyncCallback. + command = new("SELECT 1", connection) { CommandTimeout = 2 }; + readTask = command.ExecuteReaderAsync(); + + Assert.True( + batchReceived.Wait(BatchBudget), + "The server never received the batch; the async read was not in flight."); + + // Break the client's write path so the timeout attention's + // SNIWritePacket fails, forcing Close() from within ReadAsyncCallback. + proxy.BreakClientWrites(); + + try + { + settledInTime = readTask.Wait(SettleBudget); + } + catch (AggregateException) + { + // Faulting/cancelling is the healthy outcome; what matters is it settled. + settledInTime = true; + } + + Assert.True( + settledInTime, + $"ExecuteReaderAsync did not settle within {SettleBudget.TotalSeconds:N0}s after the " + + "command-timeout attention write failed. This indicates the re-entrant SNIClose " + + "deadlock: Close() called from within ReadAsyncCallback -> SNIClose -> " + + "WaitForActiveCallbacks (ADO.Net #43847 / ICM 775308542)."); + } + finally + { + releaseResponse.Set(); + command?.Dispose(); + // Only dispose the connection if we did NOT deadlock; a deadlocked + // connection's Dispose() would also hang in SNIClose. + if (settledInTime) + { + try { connection.Dispose(); } catch { /* ignore */ } + } + } + } + } + + /// + /// A transparent TCP proxy that relays bytes between a client and a backend + /// TDS server and can hard-RST the client connection on demand + /// (LingerState 0 + Close), used by the soak to break the connection at a + /// timed offset relative to the async read's command timeout. + /// + internal sealed class RstProxy : IDisposable + { + private readonly TcpListener _listener; + private readonly IPEndPoint _backend; + private Socket? _clientSocket; + private Socket? _backendSocket; + + public int Port { get; } + + public RstProxy(IPEndPoint backend) + { + _backend = backend; + _listener = new TcpListener(IPAddress.Loopback, 0); + _listener.Start(); + Port = ((IPEndPoint)_listener.LocalEndpoint).Port; + Task.Run(AcceptAndPump); + } + + /// Hard-reset the client connection (sends a TCP RST). + public void Rst() + { + try + { + Socket? s = _clientSocket; + if (s != null) + { + s.LingerState = new LingerOption(true, 0); + s.Close(); + } + } + catch + { + // best effort + } + } + + private void AcceptAndPump() + { + try + { + _clientSocket = _listener.AcceptSocket(); + _backendSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + _backendSocket.Connect(_backend); + Task.Run(() => Pump(_clientSocket, _backendSocket)); + Pump(_backendSocket, _clientSocket); + } + catch + { + // Teardown races are expected; ignore. + } + } + + private static void Pump(Socket from, Socket to) + { + byte[] buffer = new byte[8192]; + try + { + while (true) + { + int n = from.Receive(buffer); + if (n <= 0) + { + break; + } + to.Send(buffer, 0, n, SocketFlags.None); + } + } + catch + { + // Expected on RST / teardown. + } + } + + public void Dispose() + { + try { _listener.Stop(); } catch { /* ignore */ } + try { _clientSocket?.Dispose(); } catch { /* ignore */ } + try { _backendSocket?.Dispose(); } catch { /* ignore */ } + } + } + + /// + /// Soak that hunts for the re-entrant SNIClose deadlock probabilistically. + /// Many concurrent workers repeatedly open a connection, start an async read + /// that the server stalls, and RST the connection at ~the command-timeout + /// instant (with jitter), trying to land in the tiny window where the SNI + /// read timeout has fired but the timeout attention has not yet been written - + /// so the attention SNIWritePacket fails and the legacy driver closes + /// synchronously from within ReadAsyncCallback. A hang (a read that + /// never settles) means the re-entrant SNIClose was hit. + /// + /// + /// Tunable via environment variables: + /// SNICLOSE_SOAK_WORKERS (default 16; the ICM saw a "~36-thread" condition), + /// SNICLOSE_SOAK_SECONDS (default 60). + /// The test fails if it reproduces (green = did not reproduce this run). + /// + /// + public class ReentrantSNICloseSoakTests + { + private sealed class BoundedStallQueryEngine : QueryEngine + { + private readonly int _stallMs; + + public BoundedStallQueryEngine(TdsServerArguments arguments, int stallMs) + : base(arguments) + { + _stallMs = stallMs; + } + + protected override TDSMessageCollection CreateQueryResponse( + ITDSServerSession session, + TDSSQLBatchToken batchRequest) + { + // Stall every query long enough that the client's short command + // timeout fires first; the (eventual) response goes to an already + // RST-torn-down connection and the handler thread unwinds. + Thread.Sleep(_stallMs); + return base.CreateQueryResponse(session, batchRequest); + } + } + + private static int EnvInt(string name, int fallback) => + int.TryParse(Environment.GetEnvironmentVariable(name), out int v) && v > 0 ? v : fallback; + + /// + /// Gate: the soak is opt-in (set SNICLOSE_SOAK_ENABLE) because it is + /// heavy, rarely fires, and - when it DOES reproduce - poisons the + /// process (hung SNIClose threads cascade into later tests). Skipped by + /// default so normal harness runs stay clean and deterministic. + /// + public static bool SoakEnabled() => + !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("SNICLOSE_SOAK_ENABLE")); + + [ConditionalFact(typeof(ReentrantSNICloseSoakTests), nameof(SoakEnabled))] + public void ConcurrentTimeoutWithRstNearTimeout_DoesNotReentrantlyDeadlock() + { + int workers = EnvInt("SNICLOSE_SOAK_WORKERS", 16); + TimeSpan soak = TimeSpan.FromSeconds(EnvInt("SNICLOSE_SOAK_SECONDS", 60)); + const int commandTimeoutSec = 1; + const int stallMs = 6000; + TimeSpan perOpBudget = TimeSpan.FromSeconds(20); + + TdsServerArguments arguments = new(); + using TdsServer server = new(new BoundedStallQueryEngine(arguments, stallMs), arguments); + server.Start(); + IPEndPoint backend = new(IPAddress.Loopback, server.EndPoint.Port); + + using CancellationTokenSource cts = new(soak); + long attempts = 0; + string? hangInfo = null; + object gate = new(); + + void Worker(int id) + { + Random rng = new(unchecked(id * 397 ^ Environment.TickCount)); + while (!cts.IsCancellationRequested && Volatile.Read(ref hangInfo) is null) + { + RstProxy? proxy = null; + SqlConnection? conn = null; + SqlCommand? cmd = null; + Task? readTask = null; + bool settled = false; + try + { + proxy = new RstProxy(backend); + string cs = new SqlConnectionStringBuilder + { + DataSource = $"127.0.0.1,{proxy.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = false, + ConnectTimeout = 15, +#if NETFRAMEWORK + TransparentNetworkIPResolution = false, +#endif + }.ConnectionString; + + conn = new SqlConnection(cs); + conn.Open(); + cmd = new SqlCommand("SELECT 1", conn) { CommandTimeout = commandTimeoutSec }; + readTask = cmd.ExecuteReaderAsync(); + Interlocked.Increment(ref attempts); + + // Fire the RST at ~the command-timeout instant, with jitter, + // trying to land just after the SNI read timeout but before + // (or during) the attention write. + int rstDelayMs = commandTimeoutSec * 1000 + rng.Next(-40, 100); + RstProxy captured = proxy; + _ = Task.Delay(rstDelayMs).ContinueWith(_ => + { + try { captured.Rst(); } catch { /* ignore */ } + }); + + try + { + settled = readTask.Wait(perOpBudget); + } + catch (AggregateException) + { + settled = true; // faulted/cancelled = healthy + } + + if (!settled) + { + lock (gate) + { + hangInfo ??= $"worker {id}: ExecuteReaderAsync did not settle within " + + $"{perOpBudget.TotalSeconds:N0}s (attempt #{Interlocked.Read(ref attempts)})"; + } + } + } + catch + { + // Per-iteration connect/read failures under the RST storm are expected. + } + finally + { + cmd?.Dispose(); + // Only tear down if this iteration settled; a hung connection's + // Dispose() would also block in SNIClose. + if (settled) + { + try { conn?.Dispose(); } catch { /* ignore */ } + try { proxy?.Dispose(); } catch { /* ignore */ } + } + } + } + } + + Thread[] threads = new Thread[workers]; + for (int i = 0; i < workers; i++) + { + int id = i; + threads[i] = new Thread(() => Worker(id)) { IsBackground = true, Name = $"SoakWorker{id}" }; + threads[i].Start(); + } + + // Wait until the soak window elapses or a hang is detected, then give + // workers a moment to unwind. + DateTime deadline = DateTime.UtcNow + soak + perOpBudget + TimeSpan.FromSeconds(10); + while (DateTime.UtcNow < deadline + && Volatile.Read(ref hangInfo) is null + && threads.Any(t => t.IsAlive)) + { + Thread.Sleep(500); + } + foreach (Thread t in threads) + { + t.Join(TimeSpan.FromSeconds(2)); + } + + Assert.True( + Volatile.Read(ref hangInfo) is null, + $"Reproduced the re-entrant SNIClose deadlock: {hangInfo}. Total attempts: " + + $"{Interlocked.Read(ref attempts)}. (close from within ReadAsyncCallback -> SNIClose -> " + + "WaitForActiveCallbacks; ADO.Net #43847 / ICM 775308542)."); + } + } } diff --git a/tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj b/tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj index 544af613c7..7c7f5507de 100644 --- a/tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj +++ b/tools/SniCloseLegacyRepro/SniCloseLegacyRepro.csproj @@ -104,6 +104,8 @@ Link="Shared/SNICloseDeadlockTest.cs" /> + Date: Wed, 15 Jul 2026 18:49:32 -0300 Subject: [PATCH 10/10] Address PR #4420 review: widen MARS stress StallDelay; add ps1 license header - MarsCloseStressTest: set StallDelay to 00:02:00 (4x the 30s CloseBudget) so a prompt close is unambiguously attributable to the close path aborting the in-flight command, not the WAITFOR completing naturally (fixes the equal 30s==30s boundary flagged in review thread r3588309486). - run-in-container.ps1: add the standard .NET Foundation MIT license header. --- .../tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs | 3 ++- tools/SniCloseLegacyRepro/run-in-container.ps1 | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs index cb5072e764..763ee12690 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseStressTest.cs @@ -38,8 +38,9 @@ public class MarsCloseStressTest /// How long each server-side batch withholds its response. Must comfortably /// exceed so a prompt close is attributable to the /// close path aborting the command, not the query completing on its own. + /// Kept at 4x so the boundary is unambiguous. /// - private const string StallDelay = "00:00:30"; + private const string StallDelay = "00:02:00"; /// /// Upper bound for how long a batch of concurrent, non-deadlocked closes diff --git a/tools/SniCloseLegacyRepro/run-in-container.ps1 b/tools/SniCloseLegacyRepro/run-in-container.ps1 index 1fa7a107a1..02d1901b45 100644 --- a/tools/SniCloseLegacyRepro/run-in-container.ps1 +++ b/tools/SniCloseLegacyRepro/run-in-container.ps1 @@ -1,3 +1,7 @@ +# 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. + # Runs the net47 build of this harness inside a .NET Framework Windows container, # so the tests bind the container's OLD in-box System.Data.dll (e.g. 4.7.4081.0) # instead of the host's. The framework container has no dotnet CLI, so we run the