-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Mark Thread.ResetAbort as obsolete #42228
Mark Thread.ResetAbort as obsolete #42228
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.
thank you!
Hello @danmosemsft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
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.
Hi @TheRealHona, thanks for the PR! Please make two changes, and then we can merge it. First, please mark the src code obsolete, same as the ref code. Feel free to copy + paste the original obsoletion lines that are on the Abort
class - I don't think we need to change the .resx text for this.
runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
Lines 180 to 195 in b147bbe
[Obsolete(Obsoletions.ThreadAbortMessage, DiagnosticId = Obsoletions.ThreadAbortDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] | |
public void Abort() | |
{ | |
throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadAbort); | |
} | |
[Obsolete(Obsoletions.ThreadAbortMessage, DiagnosticId = Obsoletions.ThreadAbortDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] | |
public void Abort(object? stateInfo) | |
{ | |
throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadAbort); | |
} | |
public static void ResetAbort() | |
{ | |
throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadAbort); | |
} |
Second, please fix the compilation break in ImageAnimator. It should involve removing the 'catch' clause, leaving only the code within the original 'try' block remaining.
runtime/src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.Unix.cs
Lines 180 to 196 in b147bbe
public void LoopHandler() | |
{ | |
try | |
{ | |
int n = 0; | |
while (true) | |
{ | |
Thread.Sleep(delay[n++]); | |
frameChangeHandler(null, animateEventArgs); | |
if (n == delay.Length) | |
n = 0; | |
} | |
} | |
catch (ThreadAbortException) | |
{ | |
Thread.ResetAbort(); // we're going to finish anyway | |
} |
Ah, thanks Levi. I didn't find the existing one in the src, because I was looking for SYSLIB not Obsoletions.ThreadAbortDiagId. |
Should the entire try/catch block be removed? |
@TheRealHona Remove lines 182 - 183 and 192 - 196. That keeps the while loop intact while removing the try / catch parts. |
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.
Awesome, much appreciated! :)
I will squash these commits when I get home, unless you will squash it all with the merge. |
No need. It's easier for us to do it as part of merge. Force-pushing PRs makes reviewing them more difficult. Thanks for offering, though! |
Failure is known issue #41652. |
Closes #41925
Uses the same diagnostic ID as per the issue discussion.