Merge | TaskToApm #3263
Merge | TaskToApm #3263
Conversation
benrr101
added
the
Common Project 🚮
There was a problem hiding this comment.
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj: Language not supported
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/netcore/src/Common/System/Threading/Tasks/TaskToApm.cs:1
- [nitpick] Ensure that all dependent code paths have adequate tests to verify that removing TaskToApm did not inadvertently break any APM-related behaviors.
File removal
| Task<int> readTask = ReadAsync(array, offset, count, CancellationToken.None); | ||
| if (asyncCallback is not null) | ||
| { | ||
| readTask.ContinueWith(result => asyncCallback(result)); |
There was a problem hiding this comment.
Consider replacing ContinueWith with GetAwaiter().OnCompleted to better align with asynchronous execution patterns and avoid potential issues with task continuations.
| readTask.ContinueWith(result => asyncCallback(result)); | |
| readTask.GetAwaiter().OnCompleted(() => asyncCallback(readTask)); |
Copilot uses AI. Check for mistakes.
|
Looking forward to the day when all this effort can be spent on user facing fixes and features.. but nice to see you are getting closer. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3263 +/- ##
==========================================
+ Coverage 72.81% 73.02% +0.20%
==========================================
Files 297 299 +2
Lines 59661 57216 -2445
==========================================
- Hits 43444 41782 -1662
+ Misses 16217 15434 -783
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looks like TaskToAPM was pulled from .NET Framework: https://referencesource.microsoft.com/#mscorlib/system/threading/Tasks/TaskToApm.cs,419004d6a72826c7,references |
| Task<int> readTask = ReadAsync(array, offset, count, CancellationToken.None); | ||
| if (asyncCallback is not null) | ||
| { | ||
| readTask.ContinueWith(result => asyncCallback(result)); |
There was a problem hiding this comment.
The difference I can see here is that you're returning a reference to the read task, not the callback. So the caller has no way to await the callback completion. See the TAP to APM section here for an example: https://learn.microsoft.com/en-us/dotnet/standard/asynchronous-programming-patterns/interop-with-other-asynchronous-patterns-and-types
In that example, they create a new task completion source that the callback signals to and return the Task associated with that completion source.
There was a problem hiding this comment.
Hmm. The example from the link I linked in the description didn't wait for the callback to finish... Not sure why they wouldn't do that.
Either way, I've rewritten it to follow the pattern in the link you provided.
There was a problem hiding this comment.
@mdaigle is that good enough for you? 😅
There was a problem hiding this comment.
After reading more about the Begin* and End* APIs for Stream, I think my comment here was off base. The expectation from callers is that the returned result does not include the execution of the callback. The callback runs as a fire and forget task. I would say let's stick with the built in implementation for .NET Framework that we were previously using.
There was a problem hiding this comment.
@mdaigle Ok, I've reverted that change of the netfx implementation. There isn't really a "built-in" implementation like there is for netcore, so I hope this is what you meant.
It was also made public in .NET Core, as TaskToAsyncResult. There are some differences in the state management, (code) but I've not checked whether those code paths are ever hit. |
|
@edwardneal Ok, I've rewritten the netcore implementation to use TaskToAsyncResult. |
Description: This PR removes the TaskToApm class. This class was used to take task-based asynchronous operations (TPL) and convert them to the Asynchronous Programming Model (APM) pattern. It was only used in one situation in the SqlSequentialStream class.
MSDN has its own simple guide on how to convert from tasks to callbacks, which seems like it would eliminate the need for this bulky class. Please see https://learn.microsoft.com/en-us/dotnet/standard/parallel-programming/tpl-and-traditional-async-programming#implement-the-apm-pattern-by-using-tasks
Testing: Since this is a fairly significant change, it will definitely need to be tested.