-
Notifications
You must be signed in to change notification settings - Fork 317
Cleanup | AsyncHelpers Generic State #3705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes the AsyncHelper utility class by introducing generic type parameters for state management in task continuation methods. The changes eliminate runtime type casting in continuation callbacks, improve code readability, and reduce the risk of type-related bugs.
Key changes:
- Rewrote
AsyncHelpermethods to support up to two generic type parameters for stateful continuations - Introduced internal record types to encapsulate state objects for static lambda continuations
- Added comprehensive unit tests using Moq for all AsyncHelper scenarios
- Updated 100+ call sites across the codebase to use the new type-safe APIs
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs | New implementation with generic state parameters and continuation records |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs | Removed old AsyncHelper implementation |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Utilities/AsyncHelperTest.cs | Comprehensive unit tests for all AsyncHelper methods and scenarios |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Utilities/MockExtensions.cs | Helper extension methods for Moq setup in tests |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.*.cs | Updated to use generic AsyncHelper methods with decomposed tuples |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs | Updated to use generic AsyncHelper methods, removed closure-based state |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser*.cs | Updated AsyncHelper call sites to use named parameters and generic state |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj | Added Moq dependency and reorganized package references |
| src/Directory.Packages.props | Added Moq package version specification |
| typedState2.OnFailure?.Invoke(typedState2.State, task.Exception); | ||
| } | ||
| finally | ||
| { | ||
| typedState2.TaskCompletionSource.TrySetException(task.Exception); |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing task.Exception directly (an AggregateException) instead of task.Exception.InnerException. This is inconsistent with the non-generic ContinueTask method at line 27, which passes InnerException. The failure handler signature expects a single Exception, not an AggregateException.
| typedState2.OnFailure?.Invoke(typedState2.State, task.Exception); | |
| } | |
| finally | |
| { | |
| typedState2.TaskCompletionSource.TrySetException(task.Exception); | |
| typedState2.OnFailure?.Invoke(typedState2.State, task.Exception.InnerException); | |
| } | |
| finally | |
| { | |
| typedState2.TaskCompletionSource.TrySetException(task.Exception.InnerException); |
| try | ||
| { | ||
| typedState2.OnFailure?.Invoke(typedState2.State, task.Exception); | ||
| } | ||
| finally | ||
| { | ||
| typedState2.TaskCompletionSource.TrySetException(task.Exception); |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing task.Exception (an AggregateException) instead of task.Exception.InnerException. This is inconsistent with the non-generic ContinueTask method at line 34, which passes InnerException.
| try | |
| { | |
| typedState2.OnFailure?.Invoke(typedState2.State, task.Exception); | |
| } | |
| finally | |
| { | |
| typedState2.TaskCompletionSource.TrySetException(task.Exception); | |
| Exception exc = task.Exception.InnerException; | |
| try | |
| { | |
| typedState2.OnFailure?.Invoke(typedState2.State, exc); | |
| } | |
| finally | |
| { | |
| typedState2.TaskCompletionSource.TrySetException(exc); |
| { | ||
| try | ||
| { | ||
| typedState2.OnFailure?.Invoke(typedState2.State1, typedState2.State2, task.Exception); |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing task.Exception directly (an AggregateException) instead of task.Exception.InnerException. Should be consistent with the non-generic version and unwrap the inner exception.
| typedState2.OnFailure?.Invoke(typedState2.State1, typedState2.State2, task.Exception); | |
| typedState2.OnFailure?.Invoke(typedState2.State1, typedState2.State2, task.Exception.InnerException); |
| } | ||
| finally | ||
| { | ||
| typedState2.TaskCompletionSource.TrySetException(task.Exception); |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing task.Exception (an AggregateException) instead of task.Exception.InnerException. Should unwrap the inner exception for consistency.
| typedState2.TaskCompletionSource.TrySetException(task.Exception); | |
| if (task.Exception.InnerExceptions.Count == 1) | |
| { | |
| typedState2.TaskCompletionSource.TrySetException(task.Exception.InnerException); | |
| } | |
| else | |
| { | |
| typedState2.TaskCompletionSource.TrySetException(task.Exception); | |
| } |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs
Show resolved
Hide resolved
| // @TODO: This doesn't do anything, afaik. | ||
| throw exception; |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment indicates that throwing the exception in the onFailure handler may not have the intended effect. The exception is rethrown but might not propagate correctly since this is in a continuation callback. This should either be removed if truly ineffective, or the exception handling should be revised.
db04282 to
0e3250f
Compare
paulmedynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at everything except AsyncHelper.cs for now. Maybe a quick call to explain this a bit more for me (and the team?) would help.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.netcore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/UnitTests/Utilities/MockExtensions.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Outdated
Show resolved
Hide resolved
…ease timeout substantially
| onCancellation.VerifyNeverCalled(); | ||
| mockOnSuccess.Verify(action => action(), Times.Once); | ||
| mockOnFailure.VerifyNeverCalled(); | ||
| mockOnFailure.VerifyNeverCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mockOnFailure.VerifyNeverCalled(); | |
| mockOnCancellation.VerifyNeverCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be worth requesting changes over!
# Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.netcore.cs # src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj # src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.netfx.cs
… checks b/c I felt like it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:2888
- Variable source may be null at this access as suggested by this null check.
Variable source may be null at this access as suggested by this null check.
source.SetCanceled();
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:2893
source.SetException(task.Exception.InnerException);
...crosoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Utilities/AsyncHelperTest.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs
Show resolved
Hide resolved
...crosoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Utilities/AsyncHelperTest.cs
Show resolved
Hide resolved
...crosoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Utilities/AsyncHelperTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/AsyncHelper.cs
Show resolved
Hide resolved
Fix event source listener test so it is ok with the trace from the unobserved exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:2888
- Variable source may be null at this access as suggested by this null check.
Variable source may be null at this access as suggested by this null check.
source.SetCanceled();
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:2893
source.SetException(task.Exception.InnerException);
| Task taskToContinue = Task.CompletedTask; | ||
| TaskCompletionSource<object?> taskCompletionSource = GetTaskCompletionSource(); | ||
| const int state1 = 123; | ||
| const int state2 = 234 |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the constant declaration. This will cause a compilation error.
| const int state2 = 234 | |
| const int state2 = 234; |
| completion, | ||
| timeout, | ||
| onFailure: static () => SQL.CR_ReconnectTimeout(), | ||
| onTimeout: static () => SQL.CR_ReconnectTimeout(), |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter name should be onTimeout to match the method signature of SetTimeoutException. Using onFailure will cause a compilation error.
| { | ||
| ((SqlConnection)state).GetOpenTdsConnection().DecrementAsyncCount(); | ||
| }); | ||
| onFailure: static (this2, _, _) => |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect parameter usage in the onFailure callback. The signature expects Action<TState1, TState2, Exception> but the lambda is using (this2, _, _) which discards both the second state parameter and the exception. The second parameter should be the Tuple state (parameters), not a discard. This should be (this2, parameters, exception) or similar.
| onFailure: static (this2, _, _) => | |
| onFailure: static (this2, parameters, exception) => |
| AsyncHelper.ContinueTaskWithState( | ||
| task: cancellableReconnectTS.Task, | ||
| completion: source, | ||
| taskToContinue:cancellableReconnectTS.Task, |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after the colon in the named argument. This should be taskToContinue: cancellableReconnectTS.Task for consistency with the rest of the codebase.
| taskToContinue:cancellableReconnectTS.Task, | |
| taskToContinue: cancellableReconnectTS.Task, |
| /// * <paramref name="onSuccess"/> is called | ||
| /// * IF an exception is thrown during execution of <paramref name="onSuccess"/>, the | ||
| /// helper will try to set an exception on the <paramref name="taskCompletionSource"/>. | ||
| /// * <paramref name="taskCompletionSource"/> is *not* with result on success. This |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent comment style. The comment states "is not with result on success" which appears to be missing a word. It should read "is not set with result on success" to be grammatically correct.
| /// * <paramref name="taskCompletionSource"/> is *not* with result on success. This | |
| /// * <paramref name="taskCompletionSource"/> is *not* set with result on success. This |
| // Assert | ||
| // - Collected trace event IDs are in the range of official trace event IDs | ||
| // @TODO: This is brittle, refactor the SqlClientEventSource code so the event IDs it can throw are accessible here | ||
| HashSet<int> acceptableEventIds = new HashSet<int>(Enumerable.Range(0, 21)); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "Collected trace event IDs are in the range of official trace event IDs" but then creates a range from 0-20 (Enumerable.Range(0, 21) creates 0 through 20), while the original code checked for range 1-21 (Enumerable.Range(1, 21)). This changes the test behavior - event ID 0 was not previously accepted, and event ID 21 is no longer accepted. This may cause test failures if event ID 21 is valid or if event ID 0 is invalid.
| HashSet<int> acceptableEventIds = new HashSet<int>(Enumerable.Range(0, 21)); | |
| HashSet<int> acceptableEventIds = new HashSet<int>(Enumerable.Range(1, 21)); |
| // - Force collection of unobserved task | ||
| GC.Collect(); | ||
| GC.WaitForPendingFinalizers(); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call to 'GC.Collect()'.
| // - Force collection of unobserved task | |
| GC.Collect(); | |
| GC.WaitForPendingFinalizers(); | |
| // - No need to force collection of unobserved task; rely on explicit observation |
| } | ||
|
|
||
| // Force observation of any exception | ||
| _ = taskToRun.Exception; |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| catch (Exception e) | ||
| { | ||
| typedState.TaskCompletionSource.TrySetException(e); | ||
| } |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic catch clause.
| catch (Exception e) | ||
| { | ||
| typedState2.TaskCompletionSource.TrySetException(e); | ||
| } |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic catch clause.
Description
Hello all, in this latest installment, I'm rewriting majority of the AsyncHelper ContinueTask and CreateContinuationTask methods to have generic state. One thing that has bothered me a lot throughout the codebase is that many (if not all) of the ContinueTask or CreateContinuationTask callbacks have lines that decode the state object(s) passed into them. Something like this:
Not only is this challenging to read, it poses the potential for bugs where the type the state object is cast to could be anything, valid or invalid.
Instead, I've taken the liberty to:
You can now safely rewrite the above example as (generic isn't necessary since it's obvious via object types):
Issues
N/A
Testing
Introduced many unit tests for the new code, existing code can be validated via CI.