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

ImageAnimator.StopAnimate (on Unix) could throw PNSE #39405

Closed
jeffhandley opened this issue Jul 16, 2020 · 4 comments · Fixed by #52236
Closed

ImageAnimator.StopAnimate (on Unix) could throw PNSE #39405

jeffhandley opened this issue Jul 16, 2020 · 4 comments · Fixed by #52236
Assignees
Milestone

Comments

@jeffhandley
Copy link
Member

With dotnet/designs#139 and #39269, we are marking Thread.Abort() as Obsolete because it unconditionally throws PlatformNotSupportedException. The tests for the PR turned up that ImageAnimator.Unix.cs uses Thread.Abort() inside StopAnimate.

        public static void StopAnimate(Image image, EventHandler onFrameChangedHandler)
        {
            if (image == null)
                return;

            if (ht.ContainsKey(image))
            {
                AnimateEventArgs evtArgs = (AnimateEventArgs)ht[image]!;
                evtArgs.RunThread!.Abort();
                ht.Remove(image);
            }
        }

I am going to put a #pragma warning disable around the Thread.Abort() call, but this code would be unexpectedly throwing PNSE right now, which means it's either dead code or there's a lurking bug that no one has reported.

@ghost
Copy link

ghost commented Jul 16, 2020

Tagging subscribers to this area: @safern, @tannergooding
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 16, 2020
@safern safern removed the untriaged New issue has not been triaged by the area owner label Jul 16, 2020
@safern safern added this to the Future milestone Jul 16, 2020
@jeffhandley jeffhandley self-assigned this Jan 8, 2021
@jeffhandley jeffhandley modified the milestones: Future, 6.0.0 Jan 8, 2021
@jeffhandley
Copy link
Member Author

Area owners (@maryamariyan @michaelgsharp @safern @tarekgh) -- Do you think it would be appropriate to mark ImageAnimator.StopAnimate as [UnsupportedOSPlatform("unix")] since it's going to throw a PNSE if the image is animating? Or do you think we'd want to find a new way to implement animation stops such that it's not based on aborting threads?

@tarekgh
Copy link
Member

tarekgh commented Apr 25, 2021

I prefer if there is a way to avoid using Windows specific Thread.Abort and find equivalent on Linux. I am not even sure if Thread.Abort is the best option for Windows either.

CC @jkotas

@jkotas
Copy link
Member

jkotas commented Apr 25, 2021

Add a bool flag that gets set by StopAnimate and the animation loop checks every iteration. It will match what the implementation does on Windows.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Apr 28, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 4, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants