Fix async cancellation failing to send TDS attention signal#4435
Open
cheenamalhotra wants to merge 11 commits into
Open
Fix async cancellation failing to send TDS attention signal#4435cheenamalhotra wants to merge 11 commits into
cheenamalhotra wants to merge 11 commits into
Conversation
Remove lock(_stateObj) from EndExecuteReaderAsync, EndExecuteNonQueryAsync, and EndExecuteXmlReaderAsync. The lock prevented Cancel() from acquiring the stateObj monitor to send a TDS attention signal when the async completion path was blocked on a synchronous network read (e.g., waiting for metadata during WAITFOR). This caused cancellation via CancellationToken to hang until the query completed naturally. Concurrent close/cancel safety is maintained by: - Parser state checks within TryRun (detects Broken/Closed state) - Cancel()'s polling loop via Monitor.TryEnter with parser state guards - The stateObj's internal synchronization mechanisms Fixes #4424 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validates that CancellationToken triggers TDS attention when the server has sent partial results (RAISERROR WITH NOWAIT) but is blocked on WAITFOR. This test would previously hang for 60+ seconds without the fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a deadlock/hang in async cancellation by removing lock(_stateObj) from the async EndExecute* paths so that TdsParserStateObject.Cancel() can acquire the monitor and send a TDS attention signal even when the async end-path is blocked on synchronous network reads (e.g., partial results + WAITFOR).
Changes:
- Removed
lock(_stateObj)fromEndExecuteReaderAsync,EndExecuteNonQueryAsync, andEndExecuteXmlReaderAsyncto avoid blocking cancellation’s attention-send path. - Added a new manual regression test intended to reproduce the “partial results + server blocked” cancellation hang scenario from #4424.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs | Removes lock(_stateObj) from EndExecuteReaderAsync and documents rationale tied to #4424. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs | Removes lock(_stateObj) from EndExecuteNonQueryAsync to keep cancellation from being blocked by the monitor. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs | Removes lock(_stateObj) from EndExecuteXmlReaderAsync consistent with reader/nonquery. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs | Adds a manual regression test for cancellation sending attention after partial results are received. |
…pt SqlException Move CancelAfter() before ExecuteReaderAsync so cancellation fires during the async completion path (not just ReadAsync). Also accept SqlException in addition to OperationCanceledException since attention acknowledgment from the server surfaces as SqlException. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand that exercises the internal-end path in CreateLocalCompletionTask by warming the query metadata cache first, then running a long-running AE query with CancellationToken. Also removes the lock from CreateLocalCompletionTask to fix the same contention issue in the AE retry path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Use severity 10 in RAISERROR (matches real-world repro from #4424) - Assert.Fail if reader is returned in WAITFOR-first test (should never happen) - Fix AE test cache key: use same CommandText with parameterized @delay so warmup and cancel executions share the metadata cache entry Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Assert cts.IsCancellationRequested in all tests to guard against false positives from unrelated SqlExceptions - Reorder AE test query to WAITFOR DELAY @delay; SELECT ... so that EndExecuteReaderInternal blocks on metadata (exercises the CreateLocalCompletionTask internal-end path as intended) - Add Assert.Fail in AE test if reader is unexpectedly returned Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The lock in CreateLocalCompletionTask (AE internal-end/retry path) is intentionally needed — that path handles cancellation via the task's IsCanceled state, not via stateObj.Cancel(). Removing it caused TestSqlCommandCancellationToken to fail with unexpected exception types. The fix for #4424 only needs lock removal in the user-facing EndExecuteReaderAsync/NonQuery/XmlReader paths where Cancel() contends with synchronous network reads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… comment - Add CancellationOfInfiniteWhileLoop_DoesNotHang test (repro from #44) - Fix CTS timing: start CancelAfter after OpenAsync to avoid false positives when connection open is slow - Clarify CreateLocalCompletionTask lock comment to accurately describe why the lock is retained (timing differs from user-facing path) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Reword CreateLocalCompletionTask lock comment to accurately state that this lock CAN block Cancel(), and explain why it's retained (data is already buffered when this continuation fires, unlike the user-facing path where blocking reads caused #4424) - Add 45s watchdog timeout to CancellationOfInfiniteWhileLoop test with best-effort cleanup (Cancel/Close) to prevent hanging the test suite if the regression reappears Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs:2465
- The retained lock(_stateObj) in CreateLocalCompletionTask can still recreate the #4424 cancellation hang in the Always Encrypted internal-end path when the server sends any early packet (e.g., RAISERROR ... WITH NOWAIT) before blocking on metadata. In that case localCompletion can complete after the first packet (BeginExecuteInternalReadStage uses _stateObj.ReadSni), this continuation can then enter lock(_stateObj) and block inside InternalEndExecute while waiting synchronously for metadata—preventing TdsParserStateObject.Cancel() from acquiring the same monitor to send ATTENTION.
// Lock on _stateObj serializes this internal-end path with
// close/cancel. Note: stateObj.Cancel() also acquires this monitor,
// so this lock CAN block Cancel() while endFunc is executing.
// This is retained here (unlike the user-facing EndExecute* methods)
// because this continuation runs after the initial async I/O has
// already completed — the blocking metadata read that caused #4424
// in the user-facing path does not apply here since the data is
// already buffered by the time this continuation fires.
lock (_stateObj)
{
endFunc(this, task, /*isInternal:*/ true, endMethod);
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4435 +/- ##
==========================================
- Coverage 65.83% 65.18% -0.65%
==========================================
Files 287 284 -3
Lines 43763 66778 +23015
==========================================
+ Hits 28812 43532 +14720
- Misses 14951 23246 +8295
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:
|
paulmedynski
left a comment
paulmedynski
left a comment
Contributor
There was a problem hiding this comment.
I'm not a fan of the new tests, but I'm not sure what other realistic options may be right now. Waiting for comments from others before approving.
| Assert.NotNull(caughtException); | ||
| Assert.True(cts.IsCancellationRequested, | ||
| "CancellationTokenSource was not cancelled; exception may be unrelated to cancellation."); | ||
| Assert.True(stopwatch.ElapsedMilliseconds < 30000, |
Contributor
There was a problem hiding this comment.
Will this become another flaky test? Is there a way to programmatically cancel the token rather than giving it a delayed cancellation (3 seconds)? Could you cancel it from another thread after starting the async read?
Same for the next test as well.
Ideally, these would be unit tests and we would feed bytes to our TDS parser and no timing would be involved. Next best would be to expand the TDS Test server to allow programmatic delays and whatnot.
Member
Author
There was a problem hiding this comment.
Good thought. Let me see what can be done.
Member
Author
There was a problem hiding this comment.
Good call — I've refactored this test (and the sibling tests in DataReaderCancellationTest.cs) to remove the preemptive timer-based cancellation. The new pattern:
- Start
ExecuteReaderAsync/ExecuteNonQueryAsyncwithout awaiting so the token registration is in place and the query is dispatched to the server. - Kick off a separate
Task.Runthat briefly yields (500 ms) and then callscts.Cancel()from that thread. The 500 ms is not a timeout budget — the server-side WAITFOR/infinite loop runs for 60 s (or forever), so we have a huge window; the short delay just ensures the RPC has actually reached the server and started executing before we cancel, so we're genuinely exercising the TDS attention path rather than short-circuiting inside the client. awaitthe exec task and expectOperationCanceledExceptionorSqlException(attention ack).awaitthe cancel task to make sure it observed the cancel and didn't leak.
This replaces the previous new CancellationTokenSource(TimeSpan.FromSeconds(3)) / cts.CancelAfter(TimeSpan.FromSeconds(2)) patterns, which fired regardless of whether the async operation had actually begun and were the flakiness source you called out.
Agree that the ideal end state is unit tests driving bytes into the TDS parser or a programmable TDS test server. I'll file a follow-up to expand our test infra there — out of scope for this bugfix PR.
mdaigle
previously approved these changes
Jul 13, 2026
mdaigle
left a comment
mdaigle
left a comment
Contributor
There was a problem hiding this comment.
Agreed on tests. But code looks fine.
Comment on lines
+120
to
+128
| // Cancel from another thread after briefly yielding to ensure the | ||
| // async operation has been dispatched and reached the server-side | ||
| // WAITFOR. This avoids the flakiness of a preemptive timer that | ||
| // could fire before the query is actually in flight. | ||
| Task cancelTask = Task.Run(async () => | ||
| { | ||
| await Task.Delay(System.TimeSpan.FromMilliseconds(500)); | ||
| cts.Cancel(); | ||
| }); |
Comment on lines
+2346
to
+2354
| // Cancel from another thread after briefly yielding to ensure the | ||
| // async operation has been dispatched and reached the server-side | ||
| // WAITFOR. This avoids the flakiness of a preemptive timer that | ||
| // could fire before the query is actually in flight. | ||
| Task cancelTask = Task.Run(async () => | ||
| { | ||
| await Task.Delay(TimeSpan.FromMilliseconds(500)); | ||
| cts.Cancel(); | ||
| }); |
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.
Description
Fixes #4424
Fixes #44
When using
CancellationTokenwith async operations likeExecuteReaderAsync/ExecuteNonQueryAsync, cancellation fails to send a TDS attention signal to SQL Server if the server is blocked (e.g., infiniteWHILEloop,WAITFOR DELAY, or partial results fromRAISERROR WITH NOWAITfollowed by a blocking operation). The cancellation hangs until the query completes naturally — which may be forever for infinite loops.Root Cause
EndExecuteReaderAsync(and the equivalent NonQuery/XmlReader methods) heldlock(_stateObj)while calling intoEndExecute*Internal. When the server hadn't yet sent result metadata (blocked on a long-running or infinite query),TryConsumeMetaDataperformed a synchronous network read (_syncOverAsync = true), blocking the thread while holding the monitor lock.Meanwhile,
stateObj.Cancel()usesMonitor.TryEnter(this, 100ms)in a polling loop on the same stateObj instance. Since the lock was held byEndExecute*Asyncfor the entire duration of the server-side wait,Cancel()could never acquire the monitor and never sent the attention signal.Fix
Remove
lock(_stateObj)from the user-facing async end methods:EndExecuteReaderAsyncEndExecuteNonQueryAsyncEndExecuteXmlReaderAsyncThe lock is unnecessary in these paths because concurrent access is already handled by:
Monitor.TryEnterwith parser state guards in its loopTryRunchecking parserBroken/ClosedstateNote: The
lock(_stateObj)inCreateLocalCompletionTask(AE internal-end/retry path) is intentionally preserved — that path handles cancellation via the task'sIsCanceledstate rather than viastateObj.Cancel().Testing
TestSqlCommandCancellationToken) passChanges
SqlCommand.Reader.cslock(_stateObj)inEndExecuteReaderAsyncSqlCommand.NonQuery.cslock(_stateObj)inEndExecuteNonQueryAsyncSqlCommand.Xml.cslock(_stateObj)inEndExecuteXmlReaderAsyncDataReaderCancellationTest.csCancellationSendsAttention_WhenPartialResultsReceivedandCancellationDuringExecuteReaderAsync_SendsAttentiontestsApiShould.csTestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommandtest