Skip to content
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

Merged
merged 5 commits into from
Sep 15, 2020
Merged

Mark Thread.ResetAbort as obsolete #42228

merged 5 commits into from
Sep 15, 2020

Conversation

Hona
Copy link
Contributor

@Hona Hona commented Sep 14, 2020

Closes #41925

Uses the same diagnostic ID as per the issue discussion.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@ghost
Copy link

ghost commented Sep 15, 2020

Hello @danmosemsft!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a 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.

[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.

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
}

@danmoseley
Copy link
Member

Ah, thanks Levi. I didn't find the existing one in the src, because I was looking for SYSLIB not Obsoletions.ThreadAbortDiagId.

@dnfadmin
Copy link

dnfadmin commented Sep 15, 2020

CLA assistant check
All CLA requirements met.

@Hona
Copy link
Contributor Author

Hona commented Sep 15, 2020

Should the entire try/catch block be removed?

@GrabYourPitchforks
Copy link
Member

@TheRealHona Remove lines 182 - 183 and 192 - 196. That keeps the while loop intact while removing the try / catch parts.

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, much appreciated! :)

@Hona
Copy link
Contributor Author

Hona commented Sep 15, 2020

I will squash these commits when I get home, unless you will squash it all with the merge.

@GrabYourPitchforks
Copy link
Member

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!

@GrabYourPitchforks
Copy link
Member

Failure is known issue #41652.

@GrabYourPitchforks GrabYourPitchforks merged commit 60aa3be into dotnet:master Sep 15, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark System.Threading.Thread ResetAbort as Obsolete
5 participants