-
Notifications
You must be signed in to change notification settings - Fork 1k
Refactor/Improve InvokeAsync to address potential disposing issues #13564
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
Refactor/Improve InvokeAsync to address potential disposing issues #13564
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 refactors the InvokeAsync methods in Control_InvokeAsync.cs to address potential disposing issues and adds improved exception documentation.
- Added a null-check exception comment for the callback parameter.
- Inserted a handle creation check before invoking the asynchronous callback.
- Updated the cancellation token registration handling and its disposal pattern.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/System.Windows.Forms/System/Windows/Forms/Control_InvokeAsync.cs | Adds handle creation checks and adjusts cancellation token registration disposal in the async InvokeAsync implementations. |
src/System.Windows.Forms/GlobalSuppressions.cs | Introduces new suppressions for CA2007 warnings on the InvokeAsync methods. |
Comments suppressed due to low confidence (1)
src/System.Windows.Forms/System/Windows/Forms/Control_InvokeAsync.cs:186
- [nitpick] It may be beneficial to add a comment explaining that the cancellation token registration is disposed inside WrappedCallbackAsync to ensure proper unregistration and avoid potential disposal issues in asynchronous contexts.
using (registration)
src/System.Windows.Forms/System/Windows/Forms/Control_InvokeAsync.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/System/Windows/Forms/Control_InvokeAsync.cs
Outdated
Show resolved
Hide resolved
a231d58
to
3d907e2
Compare
bc2c642
to
7de829c
Compare
9d06bec
to
43d4551
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13564 +/- ##
====================================================
- Coverage 77.08926% 51.91707% -25.17219%
====================================================
Files 3272 2064 -1208
Lines 644545 287861 -356684
Branches 47673 42105 -5568
====================================================
- Hits 496875 149449 -347426
+ Misses 144002 135523 -8479
+ Partials 3668 2889 -779
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
edce38a
to
805baf5
Compare
805baf5
to
4266df2
Compare
Improve invoke behavior in InvokeAsync. Update folder for Copilot instructions for generating the InvokeAsync unit tests. (Note: They had been manually refactored quite a lot, but still saved tons of time.)
5b22c35
to
550fc44
Compare
src/System.Windows.Forms/System/Windows/Forms/Control_InvokeAsync.cs
Outdated
Show resolved
Hide resolved
else | ||
{ | ||
completion.TrySetException(ex); | ||
} |
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.
We can create a function to avoid repeating this pattern.
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.
Just those two suggestions. Rest looks good to me.
Addresses #12696, #12697
(Mentioning the latter just for same topic. I do not think, this is an issue really.)
Microsoft Reviewers: Open in CodeFlow