0

I've got a GUI interface which has a start and a cancel button. After starting, the main thread which is the GUI thread, is creating a second thread which will do the actual work. When pressing the cancel button, all it does is set a boolean value which tells the working thread to stop its work and end. The problem is that the main GUI thread remain stuck even though I'm sure that the working thread has finished what it was doing. Why is that?

Here is some of the code:

    private Thread workerThread;
    private SomeClass fs;

    private void buttonSearch_Click(object sender, EventArgs e)
    {
      //do some initializations
      fs = new SomeClass();
      workerThread = new Thread(fs.WorkMethod);
      workerThread.Start();
    }

    private void buttonCancel_Click(object sender, EventArgs e)
    {
         fs.StopWork();
         workerThread.Join();
    }


    inside SomeClass:

    private bool keepWorking;

    public void StopWork()
    {
        keepWorking= false;
    }

    public void WorkMethod()
    {
       if (keepWorking)
        {
            //do some stuff with recursion
         }
    }

does someone know why won't the main thread wake up after calling join? I have also tried debugging to see what happens if I change the keepWorking variable to false manually and the method does reach its' end.

CodeMonkey
  • 11,196
  • 30
  • 112
  • 203

1 Answers1

4

Your WorkMethod has a call to Invoke in there that is invoking a delegate to run on the UI thread and then block until it finishes. Since your UI thread is currently blocking on the call to Join waiting for the background thread, the UI thread is unable to call that delegate.

You now have both threads each waiting on the other, and no progress is being made. This is called a "deadlock".

Also, keepWorking should be marked as volatile as it's being accessed from multiple threads; as it stands the background thread can be accessing an outdated/cached value of that variable for quite some time after the main thread changes it. Marking it as volatile prevents the runtime from making such optimizations.

The solution here is to not block the UI thread with a call to Join. If you need to have some code execute when the background thread ends then you'll need to asynchronously fire that code when the thread finishes instead of synchronously blocking.

Servy
  • 202,030
  • 26
  • 332
  • 449
  • @YonatanNir That will work fine so long as it doesn't need to run in the UI thread. If it *does* need to run in the UI thread you can still do that, you'll just need to invoke. I'd also like to take a second to remind you that it's important to separate the UI from the business logic; that you're doing this is an indication (it's hard to say without more code) that you're probably mixing the two more than you should be. You may also want to use a `BackgroundWorker`, which can help make synchronizing a long running task with UI interactions easier. It even has support for cancellation. – Servy May 24 '13 at 17:44
  • Where exactly does the line cross between the division of the UI and the business logic? What I want to do is just changing some texts of buttons and labels to indicate that the work has been finished. Is that something that in your opinion shouldn't be done by the working thread? (since it's the one who knows when everything is done and can update accordingly) – CodeMonkey May 24 '13 at 17:50
  • @YonatanNir That is UI interaction and indeed it should be separated from the business logic. I'll stand by my suggestion to use a background worker. The `DoWork` event is where you can place your long running non-UI work, and the `RunWorkerCompleted` event is fired for you to update the UI based on the results (which you can set via the `Result` property). There is also built in support for cancellation that handles the synchronization issues of cancellation for you, as well as support for progress reporting and more. See the MSDN page for examples/details. – Servy May 24 '13 at 17:52