-1

I've inherited some code from a previous developer and the app occasionally gives a thread abort exception when performing a certain task.

    private void popup()
    {

        Thread th = new Thread(() =>
        {
            try
            {
                OpenForm();
            }
            catch (ThreadAbortException ex)
            {
                Console.WriteLine("Thread was forcibly aborted");
            }
        });

        th.Start();
        while (fromflag)
        {
        }

        th.Abort();

    }

The thread opens a popup with an animated loading gif, downloads files from a server and then completes. I set fromflag to false when done. I can't set a timer as it can take any time to download the files.

How can I write this differently so then I don't have to use th.Abort and the thread closes itself when complete?

d219
  • 2,707
  • 5
  • 31
  • 36
Adam McMahon
  • 765
  • 3
  • 24
  • 1
    Why do you feel like you need the `Abort` call now? – mjwills Feb 06 '18 at 12:19
  • I wonder how this happens "occasionally" since you are forcefully aborting the thread every single time. – Camilo Terevinto Feb 06 '18 at 12:20
  • @CamiloTerevinto: `Thread.Abort` is a no-op if the thread has ended "naturally", so that might be the case most of the time. – Jeroen Mostert Feb 06 '18 at 12:22
  • Well, I should say - I always get the exception in debug output. But only occasionally does it pop up to the user and close the app when running live. – Adam McMahon Feb 06 '18 at 12:23
  • If I remove the th.Abort then the popup never closes. – Adam McMahon Feb 06 '18 at 12:24
  • 2
    There's a lot of WTF going on with that code, ranging from the alarming busy wait loop, to the nasty creation of a form in a background thread, to the downright offensive call to `Thread.Abort()`. I'm going to bet that `fromflag` isn't declared as `volatile` either (not that it should be - the code should be rewritten to avoid a busy loop altogether). Anyway: The form itself should be given a way to know when to close itself rather than using `Thread.Abort()`. – Matthew Watson Feb 06 '18 at 12:27
  • My eyes bleed at this code.. of all the hacks this is just nasty and im not surprised you're having troubles with it.. its horrible, utterly utterly horrid. – BugFinder Feb 06 '18 at 12:29
  • 2
    I think "throw it away and start again from scratch" is honestly the best approach here! – Matthew Watson Feb 06 '18 at 12:30
  • "If I remove the th.Abort then the popup never closes." Usually you use Form.Close() to close any form, Modal Dialogs included. If you need to abort the thread insteand, something is very wrong in your design. – Christopher Feb 06 '18 at 12:31
  • A popup or splash screen is just a top-level screen. You *don't* need multithreading for this. They are *not* meant to process anything either, just *show* progress in response to eg events. Again, no need for threading. The easiest way to use them is to create them, wire the events and show them. While you process stuff in the background, raise the events you want. When your code is finished, close them – Panagiotis Kanavos Feb 06 '18 at 12:32
  • When this popup is shown, do you want the user to interact with the application and do other things? Or do you want the user to just wait? If just wait, remove the threading altogether. Do the downloading on a thread not showing the animation. – CodingYoshi Feb 06 '18 at 12:38

1 Answers1

1

You are opening a form on anotehr thread? That is some pretty bad design.

Usually all forms should be created by the GUI thread. If anotehr thread needs soemthing written, that is something Invoke is there for.

Without showing us the actuall code that is run in the thread (presumably contained in OpenForm()) we can not help you debugging at all.

While I can understand why the programmer used this shortcut (doing idle animations can require extra threads depending on teh Dispaly technolgoy), I still find this a bad design that should be reworked.

Christopher
  • 9,634
  • 2
  • 17
  • 31