Tests: SNIClose close-during-async-IO regression tests AB#43847 #4420
Open
paulmedynski wants to merge 10 commits into
Open
Tests: SNIClose close-during-async-IO regression tests AB#43847 #4420paulmedynski wants to merge 10 commits into
paulmedynski wants to merge 10 commits into
Conversation
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.
…ke, 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).
Contributor
There was a problem hiding this comment.
Pull request overview
Adds regression coverage to ensure SqlConnection.Close() / Dispose() do not deadlock when native SNI has an in-flight async read during connection teardown, spanning post-login, pre-login, TLS-handshake, and MARS logical-session phases.
Changes:
- Added in-process simulated-server unit tests covering close/dispose with a pending async read after login, during pre-login, and during TLS-over-TDS handshake.
- Added a live SQL Server manual test covering the MARS SMUX logical-session path with a pending async read during close/dispose.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/SNICloseDeadlockTest.cs | New simulated-server unit tests that force pending reads and verify close/dispose completes within a bounded timeout across multiple connection phases. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/MarsCloseDeadlockTest.cs | New manual live-server MARS test that keeps an async read pending on a logical session and verifies close/dispose completes promptly. |
…on-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.
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).
…anup 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.
paulmedynski
added
the
Area\Tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4420 +/- ##
==========================================
- Coverage 65.78% 64.17% -1.61%
==========================================
Files 286 284 -2
Lines 43696 66782 +23086
==========================================
+ Hits 28745 42857 +14112
- Misses 14951 23925 +8974
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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).
cheenamalhotra
changed the title
Member
|
Can you confirm these tests reproduce issue in SDS? If yes, we can safely resolve the ICM as resolved/non-existent in MDS. |
…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.
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.
- 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.
Contributor
Author
|
@cheenamalhotra - I put together a tool that runs the tests against a Windows 2016 container that has the exact SDS version noted in the ICM. It failed once, and has never failed on the SDS NuGet package, nor with modern SqlClient. |
…e 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Investigation and regression tests for AB#43847 — "SNIClose can deadlock with in-flight async I/O during connection close" (ICM 775308542).
The original incident was reported against the retired System.Data.SqlClient. This work (a) determines the root cause from the incident dump, (b) verifies whether the same deadlock exists in current Microsoft.Data.SqlClient (MDS), and (c) adds regression tests plus a dedicated legacy-driver repro harness.
Conclusion: the deadlock is not present in Microsoft.Data.SqlClient. MDS defers the connection close off the I/O callback thread (
asyncClose), soSNIClosenever runs re-entrantly and the self-deadlock cannot occur. The bug is specific to the in-boxSystem.Data.SqlClientcode path.Root cause (from the ICM dump)
The hang is a re-entrant
SNICloseself-deadlock in legacySystem.Data.SqlClient:ReadAsyncCallback→OnTimeoutCore→SendAttention→SNIWritePacketfails (the socket is half-broken: writes fail while the read stays pending).ThrowExceptionAndWarningcallsSqlConnection.Close()synchronously on the callback thread.Close→SNIClose→SNI_Conn::WaitForActiveCallbacks()spins (Sleep(100)) waiting forREF_ActiveCallbacksto drain — but the currently-executingReadAsyncCallbackis that outstanding reference. It can never drain from within itself → permanent self-deadlock.EndExecuteReaderAsync→CloseSession→ResetCancelAndProcessAttention) blocks on it, cascading into hung IOCP threads. New connections' pre-login TLS reads then time out ("SSL Provider - wait operation timed out").Why MDS is not affected
In MDS,
OnTimeoutCoreuses anasyncCloseflag: a failedSendAttentionis caught (the task is cancelled and the break deferred), andThrowExceptionAndWarning(TdsParser.cs) withasyncClose: truewraps the close inTask.Factory.StartNew.SNIClosetherefore runs off the callback thread, the active-callback reference drains normally, and there is no re-entrancy.Tests added
UnitTests — in-process TDS server
SNICloseDeadlockTest.cs— post-login query-read, pre-login stall, and TLS-over-TDS handshake close scenarios (Close/Dispose), each asserting the async read is genuinely pending at close.SNICloseRaceDeadlockTest.cs— race-timing variant of the close-during-async-read path.SNICloseHandshakeCancellationTest.cs(new) —CancelOpenAsyncDuringTlsHandshake_DoesNotDeadlock: a rawTcpListeneradvertisesENCRYPT_ONand withholds the ServerHello; the client'sOpenAsync(token)is cancelled mid-handshake and must settle promptly. Promoted here from the legacy harness so it runs against real MDS.ManualTests — live SQL Server
MarsCloseDeadlockTest.cs/MarsCloseStressTest.cs— MARS smux logical-session close/dispose with a pending async read (WAITFOR DELAY), which requires a real server because the in-process test server does not support MARS.Each scenario closes on a worker thread under a bounded wait; a deadlock would manifest as the wait timing out.
Legacy repro harness —
tools/SniCloseLegacyRepro/A dedicated xUnit harness that runs the same driver test files against the retired System.Data.SqlClient to confirm the incident lineage and probe for the re-entrant close:
net462;net47;net472;net481;net10.0; selectable legacy package version viaSqlClientLegacyVersion(default 4.9.1), plus the in-boxSystem.Data.dllon netfx.Compat.cs,GlobalUsings.cs) adapt the driver tests (which target the MDS API) to the legacySystem.Data.SqlClientsurface.ReproAttempts.csholds six exploratory Tier-2 strategies (thread-pool starvation, sync-over-async contexts, attention-breaking / RST proxies, callback re-entrancy) hunting the "write fails while read stays pending" window.ReentrantSNICloseSoak— a heavy probabilistic soak, gated opt-in behindSNICLOSE_SOAK_ENABLE. It fired once on the legacy in-box netfx driver (net462/47x/48x, with a net462 cascade) but not on the Core NuGet lineage (net10.0) — a clean driver-lineage split consistent with the real bug. It did not recur, matching the SQL EE assessment that the race is "very difficult to capture." The definitive evidence remains the dump plus theasyncClosecode review.run-in-container.ps1) againstltscimages with a live SQL Server bridged over the container NAT gateway.The harness lives under
tools/and is not part of the shipping product or default CI.Validation
Notes:
DisableTestParallelization(the thread-pool-starvation repros mutate the globalThreadPool); this matches the container runner's-parallel noneand makes runs deterministic.TdsParserStateObjectNative.Dispose()still carries anUNDONEcomment about blocking for pending callbacks on logical (MARS) connections, andDisposeCounters()waits on_readingCountbut not_pendingCallbacks. These are latent hardening opportunities, but no live deadlock was found in current MDS.Checklist
SNIClosein legacySystem.Data.SqlClient; MDS mitigated viaasyncClosedeferral)