How to stop a running thread safely on user request?

Question

I'm in a scenario when I have to terminate a thread while the thread is running according to user action on GUI. I'm using Qt 4.5.2 on Windows. One way to do that is the following:

class MyThread : public QThread
{
    QMutex mutex;
    bool stop;

    public:
        MyThread() : stop(false) {}

        void requestStop()
        {
            QMutexLocker(&mutex);
            stop = true;
        }

        void run()
        {
            while(counter1--)
            {
                QMutexLocker(&mutex);
                if (stop) return;

                while(counter2--)
                {
                }
            }
        }
};

Please note that the above code is minimal. The run function can take upto 20 seconds before finish so I want to avoid locking and unlocking the mutex variable in the loop. Is there any other way faster than this method.

Thanks in advance.

Answer 1

It doesn't directly answer your need, but can't you scope your mutex much tighter ?

while(counter1--) {
    {
      QMutexLocker(&mutex);
      if (stop) return;
    } // End locking scope : we won't read it anymore until next time
    while(counter2--)
...

Answer 2

The main problem in your code is that you are holding the lock for much longer than you actually need. You should unlock it after you check the stop variable. That should make it much faster (depending on what is done in the inner loop). A lock-free alternative is to use QAtomicInt.

Answer 3

Why not use an event that can be checked periodically and let the underlying platform worry about whether a mutex is needed or not to handle the event (I assume that Qt has event objects - I'm not all that familiar with it). If you use an event object, the platform will scope any critical section need to handle that event to as short a time period as necessary.

Also, since there's likely not going to be much contention for that mutex (the only time would be when something wants to kill the thread), grabbing and releasing the mutex will likely have little performance impact. In a loop that's taking 20 seconds to run, I'd be surprised if the impact were anything that could even be measured. But maybe I'm wrong - try measuring it by timing the thread with and without the mutex being taken. See if it's something you really need to concern yourself with.

Qt doesn't seem to have the kind of event object I'm talking about (one along the lines of Win32's event objects), but a QSemaphore can be used just as easily:

class MyThread : public QThread
{
    QSemaphore stopFlag;

    public:
        MyThread() : stopFlag( 1) {}

        void requestStop()
        {
            stopFlag.tryAcquire();  // decrement the flag (if it hasn't been already)
        }

        void run()
        {
            while(counter1--)
            {
                if (!stopFlag.available()) return;

                while(counter2--)
                {
                }
            }
        }
};

Answer 4

Firstly it doesn't look like you need a mutex around your entire inner loop, just around the if (stop) expression as the others say, but I may be missing some of your app context to definitively say that. Maybe you need requestStop() to block until the thread exits.

If the reduced mutex scope is adequate for you, then you don't need a mutex at all if you declare your stop variable as "volatile". The "volatile" keyword causes (at least under VC++) a read/write memory barrier to be placed around accesses to stop, which means your requestStop() call is guaranteed to be communicated to your thread and not cached away. The following code should work just fine on multicore processors.

class MyThread : public QThread
{
    volatile bool stop;

    public:
        MyThread() : stop(false) {}

        void requestStop()
        {
            stop = true;
        }

        void run()
        {
            while(counter1--)
            {
                if (stop) return;

                while(counter2--)
                {
                }
            }
        }
};

Answer 5

You could use a critical section instead of a mutex. They have a bit less overhead.

Otherwise you have to use this approach. If you want the worker thread to terminate within some interval t seconds, then it needs to check for a termination event at least once every t seconds.