Async Generic Helpers by benrr101 · Pull Request #4334 · dotnet/SqlClient

benrr101 · 2026-06-03T03:52:38Z

Description

This is a rebuild of #3705. Very small changes this time. The goal is to make the state object(s) used by the async helpers be generic types, so that the callbacks don't need to unwrap the state object(s) to their type. This provides better type safety for these methods. Additionally, the new versions of the helpers are more consistently written, which will be easier to maintain going forward.

In the previous PR, many of the async helper calls were updated to better utilize the generic state object(s). This made the PR very large and prone to failures. In an effort to get this reviewed more quickly, I've just taken out the most important changes from that PR (the helpers themselves) and made the associated changes smaller (fix calls that were not compiling).

This PR maintains the unobserved exception changes, and should resolve #2104 and #3720

Testing

An entire suite of unit tests for the helpers were added, and the old reflection-based tests were removed.
The PR validation tests have been passing before moving from draft to ready state.

# Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj # src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.netcore.cs # src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs # src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj # src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.netfx.cs # src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

Copilot

Pull request overview

This PR refactors the internal async continuation/timeout helpers by introducing a new Microsoft.Data.SqlClient.Utilities.AsyncHelper with generic state overloads, updates several product call sites to use the new APIs, and adds new UnitTests coverage (plus a Moq dependency) to validate the helper behaviors.

Changes:

Added a new internal Utilities/AsyncHelper implementation with generic-state continuation helpers and timeout helpers, and removed the legacy AsyncHelper previously embedded in SqlUtil.cs.
Updated multiple async call sites (e.g., SqlCommand, SqlBulkCopy, TdsParser*) to use the new stateful continuation APIs.
Added Moq-based UnitTests for AsyncHelper behaviors plus small Moq helper extensions.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/Utilities/MockExtensions.cs	Adds Moq extension helpers to reduce boilerplate in new unit tests.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Utilities/AsyncHelperTest.cs	Adds extensive unit coverage for continuation helpers and timeout/unobserved-exception behavior.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj	Introduces Moq PackageReference for unit test projects.
src/Microsoft.Data.SqlClient/tests/Directory.Packages.props	Adds central package version for Moq in tests.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs	New generic-state async continuation/timeout helper implementation.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs	Converts a continuation callback to use typed state via new helper.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs	Updates continuation usage to new `CreateContinuationTaskWithState` overloads.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs	Removes legacy internal AsyncHelper previously defined here.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs	Imports new helper namespace for existing WaitForCompletion usage.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs	Updates async continuations to use new helper parameter naming and typed state patterns.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs	Updates continuations/timeouts to new helper overloads and typed state.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs	Updates continuations to new helper overloads and typed state patterns.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Encryption.cs	Updates continuation usage to new typed-state helper overloads.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	Updates multiple continuations/timeouts to new helper APIs and typed-state patterns.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs	Imports new helper namespace and adjusts usings/conditionals.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

codecov · 2026-06-04T19:33:56Z

Codecov Report

❌ Patch coverage is 77.69080% with 114 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.80%. Comparing base (bfbdd30) to head (e85c55c).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
.../Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs	89.47%	44 Missing ⚠️
...Client/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	12.12%	29 Missing ⚠️
.../src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs	22.22%	14 Missing ⚠️
.../Microsoft/Data/SqlClient/SqlCommand.Encryption.cs	0.00%	10 Missing ⚠️
...ent/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs	12.50%	7 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	44.44%	5 Missing ⚠️
...qlClient/src/Microsoft/Data/SqlClient/TdsParser.cs	50.00%	4 Missing ⚠️
...rc/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4334      +/-   ##
==========================================
- Coverage   66.69%   64.80%   -1.89%     
==========================================
  Files         284      281       -3     
  Lines       43238    66391   +23153     
==========================================
+ Hits        28836    43023   +14187     
- Misses      14402    23368    +8966

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`64.80% <77.69%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Wraith2 · 2026-06-04T23:54:34Z

-                            onCancellation: static state => ((StrongBox<CancellationTokenRegistration>)state).Value.Dispose(),
-                            exceptionConverter: ex => SQL.BulkLoadInvalidDestinationTable(_destinationTableName, ex)
-                        );
+                            onFailure: (regReconnectCancel2, exception) =>


Can this be static or is it now capturing instance state? if it's capturing that's a slight memory performance regression.

No this one is not static. There are still improvements that can be made to make more callbacks static. At this point, having a separate overload for a one-off situation where the exception type needs to be changed isn't worth it. It's also worth noting that the success callback is not static, nor is the exception converter itself. As such, I'm not sure it will make much of a difference.

It depends how often it's called. If it's once per field it could be significant. It's unlikely to cause time problems but it could add to memory thrashing. I only called this one out because it's changed from static to non-static, existing non-statics were already being pinned by some sort of capture.

I did a pass through the whole codebase years ago and made everything that was capable static, that's why there's weird use of Tuple and some TaskCompletionSource state smuggling

Wraith2 · 2026-06-05T00:03:05Z

-                        completion: localCompletion,
+                        taskToContinue: writeTask,
+                        taskCompletionSource: localCompletion,
                        state: Tuple.Create(this, localCompletion),


Can this one use the two state parameter form used earlier to avoid the tuple creation again?

Sure this one can utilize the two state parameter version

Wraith2 · 2026-06-05T00:07:55Z

@@ -1272,13 +1269,12 @@ internal Task ExecuteFlush()
                    else
                    {
                        return AsyncHelper.CreateContinuationTaskWithState(


Unless state is constained to class this pattern could be used with a struct and lead to a hard to find bug. If we don't want the constraint it might be worth an assertion of reference type.

Fair enough - I'll try to document why the constraint exists

Wraith2 · 2026-06-05T00:12:55Z

    -->
    <PackageReference Include="Microsoft.Extensions.TimeProvider.Testing" ExcludeAssets="build;buildTransitive" />
    <PackageReference Include="Microsoft.NET.Test.Sdk" />
+    <PackageReference Include="Moq" />


I remember that there was some controversy around Moq that caused a lot of people to not want to use it anymore. Is it worth considering other newer alternatives like https://github.com/guinhx/NimbleMock (MIT)

It appears the controversy has been resolved, though the goodwill of the project has been damaged. Improved perf is definitely nice, though I'm not familiar enough with NimbleMock to endorse it yet. The other big contender, NSubstitute is an option, but I've presented the pros/cons in the past and I personally prefer the syntax of Moq. There doesn't appear to be any legal reasons to use something else, so for now, let's focus on the functional changes rather than the testing minutia.

…class types.

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.

+                // Act
+                // - Run task that will always time out
+                TaskCompletionSource<object?> tcs = new();
+                AsyncHelper.WaitForCompletion(
+                    tcs.Task,
+                    timeoutInSeconds: 1,
+                    onTimeout: null,
+                    rethrowExceptions: true);
+
+                // - Task has timed out, simulate faulting task completion source
+                tcs.SetException(new Exception("late failure"));
+
+                // - Force collection of unobserved task
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+                GC.Collect();


+            await RunWithTimeout(taskCompletionSource.Task, RunTimeout);
+
+            // Assert
+            // - taskCompletionSource should have been cancelled, regardless of mockOnSuccess throwing


+            // Arrange
+            // - Task to continue that is cancelled
+            Task taskToContinue = Task.FromException(new Exception());
+            TaskCompletionSource<object?> taskCompletionSource = GetTaskCompletionSource();
+            Mock<Action> mockOnSuccess = new();
+            Mock<Action> mockOnCancellation = new();
+
+            // Act
+            AsyncHelper.ContinueTask(
+                taskToContinue,
+                taskCompletionSource,
+                mockOnSuccess.Object,
+                onFailure: null,
+                mockOnCancellation.Object);
+            await RunWithTimeout(taskCompletionSource.Task, RunTimeout);
+
+            // Assert
+            // - taskCompletionSource should have been cancelled
+            Assert.Equal(TaskStatus.Faulted, taskCompletionSource.Task.Status);


+            await RunWithTimeout(taskCompletionSource.Task, RunTimeout);
+
+            // Assert
+            // - taskCompletionSource should have been cancelled, regardless of mockOnSuccess throwing


+            await RunWithTimeout(taskCompletionSource.Task, RunTimeout);
+
+            // Assert
+            // - taskCompletionSource should have been cancelled


+            await RunWithTimeout(taskCompletionSource.Task, RunTimeout);
+
+            // Assert
+            // - taskCompletionSource should have been cancelled, regardless of mockOnSuccess throwing


+            await RunWithTimeout(taskCompletionSource.Task, RunTimeout);
+
+            // Assert
+            // - taskCompletionSource should have been cancelled


+        public async Task CreateContinuationTask_TaskCancelsNoHandler()
+        {
+            // Arrange
+            // - Task to continue completed successfully


benrr101 added 6 commits June 2, 2026 13:10

Implement AsyncHelpers with generic state, add unit tests

e20fc9b

Change existing async helper calls in SqlBulkCopy.cs

ab424fa

Change existing async helper calls in TdsParser.cs

1608dfd

Change existing async helper calls in TdsParser.cs

80beeff

Change existing async helper calls in SqlCommand

734fa77

Copilot AI review requested due to automatic review settings June 3, 2026 03:52

benrr101 added the DO NOT MERGE PRs that are created for test reasons, should not be merged. label Jun 3, 2026

github-project-automation Bot added this to SqlClient Board Jun 3, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Jun 3, 2026

Copilot started reviewing on behalf of benrr101 June 3, 2026 03:52 View session

Copilot AI reviewed Jun 3, 2026

View reviewed changes

benrr101 added 2 commits June 3, 2026 12:19

Remove old tests

c0127ec

Comments from copilot round 1

e516e3b

Copilot AI review requested due to automatic review settings June 4, 2026 17:57

Copilot started reviewing on behalf of benrr101 June 4, 2026 17:57 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Copilot stopped reviewing on behalf of benrr101 due to an error June 4, 2026 18:56
Copilot had to stop work due to a timeout.

benrr101 added Code Health 💊 Issues/PRs that are targeted to source code quality improvements. and removed DO NOT MERGE PRs that are created for test reasons, should not be merged. labels Jun 4, 2026

benrr101 marked this pull request as ready for review June 4, 2026 22:50

benrr101 requested a review from a team as a code owner June 4, 2026 22:50

benrr101 added this to the 7.1.0-preview2 milestone Jun 4, 2026

benrr101 mentioned this pull request Jun 4, 2026

Cleanup | AsyncHelpers Generic State #3705

Closed

benrr101 changed the title ~~[DRAFT] Async Generic Helpers~~ Async Generic Helpers Jun 4, 2026

Wraith2 reviewed Jun 4, 2026

View reviewed changes

Wraith2 reviewed Jun 5, 2026

View reviewed changes

benrr101 added 2 commits June 5, 2026 14:04

Address feedback from wraith - including constraining state types to …

8a3ff94

…class types.

Fix tests for last change

e85c55c

Copilot AI review requested due to automatic review settings June 5, 2026 21:22

Copilot started reviewing on behalf of benrr101 June 5, 2026 21:23 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

mdaigle assigned mdaigle and paulmedynski Jun 5, 2026

mdaigle moved this from To triage to In review in SqlClient Board Jun 5, 2026

Conversation

benrr101 commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benrr101 commented Jun 3, 2026 •

edited

Loading

codecov Bot commented Jun 4, 2026 •

edited

Loading