- Notifications
You must be signed in to change notification settings - Fork311
Merge | TaskToApm#3263
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merge | TaskToApm#3263
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
codecovbot commentedApr 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mdaigle is that good enough for you? 😅
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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, asTaskToAsyncResult. 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. |
4b6cefc intomainUh oh!
There was an error while loading.Please reload this page.
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, whichseems like it would eliminate the need for this bulky class. Please seehttps://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.