Is my multithreaded form code correct?

Question

The basic idea is this:

There are two buttons: a DoWork button and a Cancel button.

The DoWork button should launch a thread to do some work, unless that thread was already launched and not canceled.

The Cancel button should cancel the worker thread, if it's running.

Please ignore the fact that it calls Thread.Abort() for the moment, that is a separate issue that I will address later.

Also, how does one generally show the correctness of one's own multithreaded code? It seems much more difficult to debug.

Here's the code:

 private Thread m_WorkingThread;
 private bool m_finishedWorking;
 public Form()
 {
 InitializeComponent(); // initialize all the form controls.
 m_WorkingThread = null;
 m_finishedWorking = false;
 }
 private void bDoWork_Click(object sender, EventArgs e)
 { 
 if (m_WorkingThread != null)
 if(m_WorkingThread.IsAlive || m_finishedWorking)
 return;
 ThreadStart ts = new ThreadStart(DoWork);
 Thread t = new Thread(ts);
 t.Start();
 m_WorkingThread = t;
 }
 private void bCancel_Click(object sender, EventArgs e)
 {
 AbortThread(m_WorkingThread);
 m_finishedWorking = false;
 }
 private void AbortThread(Thread t)
 {
 if (t != null)
 if (t.IsAlive)
 t.Abort();
 }
 private void DoWork()
 {
 // do some work here, maybe using Invokes / BeginInvokes to update any controls.
 m_finishedWorking = true;
 }

Answer 1

Have you considered using BackgroundWorker? It might save you some trouble.

However, it does use ThreadPool and you are better to create your own threads if you have any of the following requirements:

• You require a foreground thread.
• You require a thread to have a particular priority.
• You have tasks that cause the thread to block for long periods of time. The thread pool has a maximum number of threads, so a large number of blocked thread pool threads might prevent tasks from starting.
• You need to place threads into a single-threaded apartment. All ThreadPool threads are in the multithreaded apartment.
• You need to have a stable identity associated with the thread, or to dedicate a thread to a task.

I add my comment to the answer of "Peter K." here as it seemed to please you.

Mutex is a bit overkill as it works across multiple processes. One should use locks (i.e. Monitor) when there is no need to synchronize across processes. Anyway, I don't see here any need for the locks assuming that the bDoWork_Click() is always called from the UI thread. It only creates a new thread if the existing thread is not alive. However, the resources used by DoWork() must be protected with locks if they are shared with other threads.

• user420667: I have, but I am not too familiar with it except that it uses the threadpool. Ultimately I need to have threads that start when somebody clicks a button, and pause the other threads until that "top" thread finishes. Consequently there can be many running threads (like if the user keeps clicking various things) and I hear the threadpool is not so good for such things (many / long-running threads.) In general though you might be right, I really don't know anything about them.
• mgronber: I don't really understand what you mean by pausing the other threads. I hope that you are not going to suspend any background threads because you should not do that.
• user420667: Yeah, by pausing I don't mean in the traditional sense. Each thread calls something like while(paused[threadId]) Thread.Sleep(someAmount), before doing any substantial work. Or is that still a bad approach?
• mgronber: You could also use ManualResetEventSlim instance to control the state for each thread. Instead of while loop you would just call paused[threadId].Wait() and the thread will stay there paused until the corresponding ManualResetEventSlim instance is Set(). With Reset() you can request a pause again and the thread will pause when it reaches the Wait(). It should do what you want. Your current implementation may not work correctly unless there is a memory barrier that forces the value of paused[threadId] to be read from the memory and not from the cache.
• user420667: Great. I'll try to use those instead of a list of bools, unless you know how to get a list of volatile bools. :-P (Although I guess I could have a list of locks associated w/ each bool.) And the lists themselves would need to have locks associated w/ them. yay multithreading.

Answer 2

Trying to use shared variables is generally a Bad Thing(TM). The usual way to do cross-thread locking / resource checking is via a mutex.

Something like this, though I haven't compiled or checked it:

private Thread m_WorkingThread;
private static Mutex m_FinishedWorking = new Mutex(); 
public Form()
{
 InitializeComponent(); // initialize all the form controls.
 m_WorkingThread = null;
}
private void bDoWork_Click(object sender, EventArgs e)
{ 
 if (m_WorkingThread != null)
 if(m_WorkingThread.IsAlive)
 return;
 ThreadStart ts = new ThreadStart(DoWork);
 Thread t = new Thread(ts);
 t.Start();
 m_WorkingThread = t;
}
private void bCancel_Click(object sender, EventArgs e)
{
 AbortThread(m_WorkingThread);
}
private void AbortThread(Thread t)
{
 if (t != null)
 if (t.IsAlive)
 t.Abort();
}
private void DoWork()
{
 m_FinishedWorking.WaitOne();
 // do some work here, maybe using Invokes / BeginInvokes to update any controls.
 // Must be called if the thread is aborted.
 m_FinishedWorking.ReleaseMutex(); 
}

• mgronber: Mutex is a bit overkill as it works across multiple processes. One should use locks (i.e. Monitor) when there is no need to synchronize across processes. Anyway, I don't see here any need for the locks assuming that the bDoWork_Click() is always called from the UI thread. It only creates a new thread if the existing thread is not alive. However, the resources used by DoWork() must be protected with locks if they are shared with other threads.
• user420667: @mgronber: I would like to accept your comment. :-P

Answer 3

I could not find something wrong in your code.

But I would suggest, of course depending on your Framework Version, to use Tasks instead of Threads - System.Threading.Tasks offers you a very good API.

Here a quick example, maybe not bulletproof - but I think you will get the idea:

private Task task;
private CancellationTokenSource tokensource;
private CancellationToken token; 
public Form1() {
 InitializeComponent();
 tokensource = new CancellationTokenSource();
 token = tokensource.Token;
 this.Cancel.Enabled = false;
}
private void DoWork_Click(object sender, EventArgs e) {
 if (task != null && !task.IsCompleted) return;
 this.DoWork.Enabled = false;
 this.Cancel.Enabled = true;
 task = Task.Factory.StartNew(() => DoWorkAction(), token)
 .ContinueWith(_ => { this.DoWork.Enabled = true; this.Cancel.Enabled = false; }, 
 TaskScheduler.FromCurrentSynchronizationContext());
}
private void Cancel_Click(object sender, EventArgs e) {
 tokensource.Cancel();
}
private void DoWorkAction() {
 Thread.Sleep(5000);
}

• user420667: Thanks. I'll look into them. Unfortunately I think it uses ThreadPool, which I am hoping to avoid.