Async Generic Helpers#4334
Conversation
# 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
benrr101
added
the
DO NOT MERGE
There was a problem hiding this comment.
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/AsyncHelperimplementation with generic-state continuation helpers and timeout helpers, and removed the legacyAsyncHelperpreviously embedded inSqlUtil.cs. - Updated multiple async call sites (e.g.,
SqlCommand,SqlBulkCopy,TdsParser*) to use the new stateful continuation APIs. - Added Moq-based UnitTests for
AsyncHelperbehaviors 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4334 +/- ##
==========================================
- Coverage 66.69% 64.30% -2.39%
==========================================
Files 284 281 -3
Lines 43238 66390 +23152
==========================================
+ Hits 28836 42695 +13859
- Misses 14402 23695 +9293
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:
|
benrr101
added
Code Health 💊
| onCancellation: static state => ((StrongBox<CancellationTokenRegistration>)state).Value.Dispose(), | ||
| exceptionConverter: ex => SQL.BulkLoadInvalidDestinationTable(_destinationTableName, ex) | ||
| ); | ||
| onFailure: (regReconnectCancel2, exception) => |
There was a problem hiding this comment.
Can this be static or is it now capturing instance state? if it's capturing that's a slight memory performance regression.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I suspect we're OK here, since I doubt these failure/error conditions are hot paths.
We could make all of these lambdas static if we moved the callback bodies onto the classes as instance methods (or class methods if they really didn't need any object state). That might be an interesting investigation to see how messy it gets with a bunch of small callback methods defined on the classes themselves rather than inline.
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
This is definitely a déjà vu PR 😄 . I will look at the tests once my current feedback has been addressed.
| onCancellation: static state => ((StrongBox<CancellationTokenRegistration>)state).Value.Dispose(), | ||
| exceptionConverter: ex => SQL.BulkLoadInvalidDestinationTable(_destinationTableName, ex) | ||
| ); | ||
| onFailure: (regReconnectCancel2, exception) => |
There was a problem hiding this comment.
I suspect we're OK here, since I doubt these failure/error conditions are hot paths.
We could make all of these lambdas static if we moved the callback bodies onto the classes as instance methods (or class methods if they really didn't need any object state). That might be an interesting investigation to see how messy it gets with a bunch of small callback methods defined on the classes themselves rather than inline.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
It gets very messy. If we change from a capturing lambda to an instance callback we gain nothing we only change the type of the allocation from a compiler generated async state machine to a delegate per call. In order to gain you have to have a static lambda and a static target so the delegate is one-time instantiated by the compiler and you have to have your instance as a state parameter which burns the (Task, object) state parameter of the default continuation shape forcing you to pack any additional state into a compound object, which has to be a ref type. This is why the weird use of Tuple<,> and not valuetuple exists in the current code. In practice I found that moving the callbacks into static trampolines that call a specific instance method on the state param makes the code more difficult to follow so it is only used when it is on hot paths where the allocation reduction is significant enough to outweigh the readability concerns. |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Looking better - just 2 outstanding comments.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
Remove `!` that wasn't necessary because of Debug.Assert above Add documentation as per @paulmedynski's comments
mdaigle
left a comment
mdaigle
left a comment
There was a problem hiding this comment.
Just one testing request
| _ = task.Exception; | ||
| }, | ||
| CancellationToken.None, | ||
| TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, |
There was a problem hiding this comment.
We use TaskContinuationOptions.ExecuteSynchronously because the work is so light here?
There was a problem hiding this comment.
So I wasn't super sure on this either, since these were 🤖 suggestions. Here's the explanation I got:
ExecuteSynchronously
Despite its name, ExecuteSynchronously does not guarantee synchronous execution. It asks the selected scheduler to run the continuation inline on the thread that
completes the antecedent task, when the scheduler considers that safe.
Without it:
- Task.Delay completes.
- The continuation is queued to the ThreadPool.
- Another ThreadPool work item eventually runs the continuation.
With it:
- Task.Delay completes.
- The timeout continuation can immediately run on the completing thread.
Benefits:
- Avoids an additional ThreadPool queue operation.
- Reduces allocations/scheduling overhead.
- Reduces latency between the timeout expiring and the exception being set.
Risks:
- The completing thread must execute the continuation.
- Blocking or expensive continuation work delays that thread.
- Inline execution introduces reentrancy and ordering differences.
- Completing the TaskCompletionSource may itself synchronously execute downstream continuations because these sources do not use RunContinuationsAsynchronously.
- Long chains of synchronous continuations can increase stack depth.
For the timeout helpers, the continuation appears small:
if (!task.IsCanceled && !taskCompletionSource.Task.IsCompleted)
{
taskCompletionSource.TrySetException(onTimeout());
}
However, onTimeout() and TrySetException() can trigger more work than is immediately visible. Consequently, ExecuteSynchronously is a reasonable optimization, but it is
not required for correctness and is less obviously beneficial than TaskScheduler.Default.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
|
👨🦱 comments were also addressed :) (@paulmedynski @mdaigle ) |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Just looking for some doc updates. Code looks good.
| /// * Will try to set exception on <paramref name="taskCompletionSource"/> | ||
| /// * Cancelled | ||
| /// * <paramref name="onCancellation"/> is called (if provided) | ||
| /// * Any exception thrown by <paramref name="onCancellation"/> will be logged and |
There was a problem hiding this comment.
I think the same goes for onFailure too.
| mockOnCancellation.VerifyNeverCalled(); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
Can we get some docs on each test method that explains what it's testing? Sorry I didn't notice this sooner.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
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.