How to Stop a Thread at a given point.

Question

I was trying to stop some threads, read some things about the proper way to do it gracefully, but I must be doind something wrong because it simply doesn't work. At first I tried without the lock() with _IsRunning being volatile, then tried with the locks. Here is what I've got.

private volatile bool _IsRunning;
private static readonly object runLock = new object();

public void Start()
{
    if (_IsRunning == true)
        return;
    _IsRunning = true;
    (new System.Threading.Thread(new System.Threading.ThreadStart(SendLoop))).Start();
}

public void Stop()
{
    lock (runLock)
    {
        _IsRunning = false;
    }
}

private void SendLoop()
{
    while (_IsRunning)
    {
        lock (runLock)
        {
            if (_sockets.Count > 0)
            {
                //some stuff
            }
            else
            {
                System.Threading.Thread.Sleep(10);
            }
        }
    }
}

I set a breakpoint at my while(), and _IsRunnig is still true even though I passed in Stop().

Answer 1

You need to reorganize your loop a little bit. Right now you are holding the lock on runLock for a very long period of time. This will cause anyone calling the Stop method to hang until the if block succeeds or the Sleep call returns. This can lead to issues because you cannot look at _isRunning when the Stop method is called, only when it returns. Try reorganizing your code as follows

private void SendLoop() {
  do {
    if ( _sockets.Count > 0 ) {
    } else { 
      System.Threading.Thread.Sleep(10);
    }
    bool shouldContinue;
    lock ( runLock ) { 
      shouldContinue = _IsRunning;
    }
  while(shouldContinue);
}

I'm not 100% sure this is the issue. But at least it should help clear things up a bit.

Answer 2

You don't need the lock for stopping the thread - a boolean assignment is atomic by itself. You must be doing something wrong, because this does work:

internal static class Program
{
    private static volatile Boolean _stop;

    private static void ThreadProc()
    {
        while (!_stop)
        {
            Console.Write('.');
            Thread.Sleep(100);
        }
    }

    private static void Main()
    {
        var thread = new Thread(ThreadProc);
        thread.Start();

        Console.ReadLine();
        _stop = true;

        thread.Join();
        Console.WriteLine("thread stopped");
    }
}

From your example I assume that your ultimate goal is to process things as they are inserted into some collection. You should most definitely not use the Thread.Sleep call. You will need a lock to protect the collection, and you should use it to wake the thread when new items become available.

internal abstract class Worker<T>
{
    private readonly Thread _thread;
    private volatile Boolean _stop;

    private readonly Object _syncRoot = new Object();
    private readonly Queue<T> _objects = new Queue<T>();

    public Worker()
    {
        _thread = new Thread(ThreadProc);
    }

    public void Start()
    {
        _thread.Start();
    }

    public void Stop()
    {
        _stop = true;
        _thread.Join();
    }

    public void Enqueue(T obj)
    {
        lock (_syncRoot)
        {
            _objects.Enqueue(obj);

            // tell the thread to wake up
            Monitor.Pulse(_syncRoot);
        }
    }

    private void ThreadProc()
    {
        while (!_stop)
        {
            T[] stuff;

            lock (_syncRoot)
            {
                // Wait for stuff to become available
                Monitor.Wait(_syncRoot);

                stuff = _objects.ToArray();
                _objects.Clear();
            }

            foreach (var item in stuff)
            {
                Process(item);
            }
        }
    }

    protected abstract void Process(T item);
}