How to start thread if button pressed and stop it if pressed again?

Question

I'm using the next code to do what I'm asking for :

private delegate void CallerDelegate(object e);
CallerDelegate caler = new CallerDelegate(MethodToCall);

on button click event :

if (currBusyThrd != null && currBusyThrd.IsAlive)
   {
    currBusyThrd.Abort();
   }
ThreadPool.SetMaxThreads(1, 1);
//queue the work for thread processing
ThreadPool.QueueUserWorkItem(new WaitCallback(WaitCallbackMethod))

"WaitCallbackMethod" Method is :

void WaitCallbackMethod(object stateInfo)
  {
     // argList : i put some argument in a list to use it in "MethodToCall" ...
     BeginInvoke(caler,argList);
  }

and the method i'm calling by the thread is :

void MethodToCall(object args)
 {
 //Here I get the thread I'm calling to stop it when btn clicked again
 currBusyThrd = Thread.CurrentThread;

 // The rest of the code ...
 }

I feel that this is wrong ... How to do it right ?

Actually the calling will be by TextBox_KeyUp .. so every time the user enter a char the code will execute again .. and the BackgroundWorker didn't work .

Answer 1

One problem to this approach is that it's very dangerous to arbitrarily Abort a thread (in pretty much any language). There are too many issues that can popup around unfreed resources and misheld locks. It's typically best to set some kind of flag to ask the Thread to safely abort itself or to forget about the thread and let it run to completion.

Additionally, Aborting a Thread in the ThreadPool is very dangerous and I believe not a supported operation. The Threads in the ThreadPool are not owned by you and Aborting them cold have serious implications for the ThreadPool.

Here is the solution I would take.

private object m_lock = new object();
private bool m_isRunning = false;
private bool m_isAbortRequested = false;

public void OnButtonClick(object sender, EventArgs e) {
  lock ( m_lock ) {
    if ( m_isRunning ) {
      m_isAbortRequested = true;
    } else {
      m_isAbortRequested = false;
      m_isRunning = true;
      ThreadPool.QueueUserWorkItem(BackgroundMethod);
    }
  }
}

private void BackgroundMethod() {
  try {
    DoRealWork();
  } finally {
    lock (m_lock) {
      m_isRunning = false;
    }
  }
}

private void DoRealWork() {
  ...
  if ( m_isAbortRequested ) {
    return;
  }
}

Answer 2

Yes, this is very wrong. You should never try to manually control a ThreadPool thread. If you need this sort of control, you should be using your own Thread object. In addition, Abort() is not the recommended way of ending a thread; you should have a control volatile bool on your form that the code in MethodToCall checks at various points and exits gracefully when it's true. While you can use the same approach with the ThreadPool, the fact that you need to be able to cancel seems to indicate that the process is long-running, or at least has the potential to be. The ThreadPool shouldn't be used for long-running processes.

For example...

private volatile bool stopThread = false;
private Thread workThread;

private void StartThread()
{
    if(workThread == null)
    {
        stopThread = false;
        workThread = new Thread(new ThreadStart(MethodToCall));

        workThread.Start();
    }
}

private void StopThread()
{
    if(workThread != null)
    {
        stopThread = true;

        workThread.Join(); // This makes the code here pause until the Thread exits.

        workThread = null;
    }
}

Then in MethodToCall, just check the stopThread boolean at frequent intervals and do any cleanup work that you need to do and exit the method. For example...

private void MethodToCall()
{
    // do some work here and get to a logical stopping point

    if(stopThread)
    {
        // clean up your work

        return;
    }

    // do some more work and get to another stopping point

    if(stopThread)
    {
        // clean up your work

        return;
    }
}

And just repeat that pattern.