11
\$\begingroup\$

I have written this code to be able to suspend (or to cancel) a worker executed in a separate thread in Qt.

To do it, I have used an instance QWaitCondition and QMutex.

#ifndef MYWORKER_H
#define MYWORKER_H

#include <QObject>
#include <QMutex>
#include <QWaitCondition>

class MyWorker : public QObject
{
    Q_OBJECT
public:
    explicit MyWorker(QObject *parent = 0);
    ~MyWorker();
    void restart();
    void pause();

signals:
    void finished();

public slots:
    void doWork();
    void cancelWork();

private:
    QMutex m_continue;
    QWaitCondition m_pauseManager;
    bool m_cancelRequested;
    bool m_pauseRequired;
};

#endif // MYWORKER_H

Here is the code of myworker.cpp

#include "myworker.h"

#include <QDebug>

MyWorker::MyWorker(QObject *parent) :
    QObject(parent),
    m_cancelRequested(false),
    m_pauseRequired(false)
{
}

MyWorker::~MyWorker()
{
}

void MyWorker::restart()
{
    this -> m_pauseRequired = false;
    this -> m_pauseManager.wakeAll();

}

void MyWorker::pause()
{
    this -> m_pauseRequired = true;
}

void MyWorker::doWork()
{
    // Write your code here
    for(int i = 0; i < 4000000000; i++)
    {
        if (i % 100000 == 0) {
            qDebug() << i;
        }

        if (m_pauseRequired) {
            m_pauseManager.wait(&m_continue);
        }

        if (this -> m_cancelRequested()) {
            break;
        }
    }

    // Write your code above"

    emit finished();
}

void MyWorker::cancelWork()
{
    this -> restart();
    this -> m_cancelRequested = true;
}

Here is the snippet to use this code :

this -> m_thread = new QThread();
this -> m_worker = new MyWorker();
this -> m_worker-> moveToThread(this -> m_thread);
connect(m_thread, SIGNAL(started()), m_worker, SLOT(doWork()));
connect(m_worker, SIGNAL(finished()), m_thread, SLOT(quit()));

this -> m_thread -> start();

// To suspend the work
this -> m_worker -> pause();

// To stop the work
this -> m_worker -> cancelWork();

Do you have any advice on improving this code, or do you have a better solution?

\$\endgroup\$
9
\$\begingroup\$

The code you have is unsafe, because pause() and cancelWork() are called from outside the worker's thread, and they use no locking when modifying the worker's members.

In general, prefer to interact with objects in other threads by invoking their slots via queued connections, unless there's a pressing reason to act immediately. This means you don't need any of the synchronization facilities inside the worker. This simplifies the coding, and makes it easier to test.

To do that, the worker needs to periodically process events. I prefer to put this in a function of its own:

bool isCancelled() {
    auto dispatcher = QThread::currentThread()->eventDispatcher();
    if (!dispatcher) return false;
    dispatcher->processEvents();
    return m_cancelRequested;
}

In the work method, this can be called every time you want to poll for cancellation.

You need some protection against re-entering doWork() from this event queue. One way to manage this is with a tiny state-machine:

private:
    enum State { IDLE, RUNNING, PAUSED };
    State state = IDLE;

Used like this:

void MyWorker::doWork()
{
    if (state == RUNNING)
        return;

    state = RUNNING;
    emit started();

Here's my complete, tested version - no locks or atomics, all the synchronization is managed by Qt:

#include <QAbstractEventDispatcher>
#include <QObject>
#include <QThread>
#include <QDebug>

class Worker : public QObject
{
    Q_OBJECT

public:
    explicit Worker(QObject *parent = 0)
        : QObject(parent)
    {
    }

signals:
    void started();
    void finished();

public slots:
    void pause()
    {
        auto const dispatcher = QThread::currentThread()->eventDispatcher();
        if (!dispatcher) {
            qCritical() << "thread with no dispatcher";
            return;
        }

        if (state != RUNNING)
            return;

        state = PAUSED;
        qDebug() << "paused";
        do {
            dispatcher->processEvents(QEventLoop::WaitForMoreEvents);
        } while (state == PAUSED);
    }

    void resume()
    {
        if (state == PAUSED) {
            state = RUNNING;
            qDebug() << "resumed";
        }
    }

    void cancel() {
        if (state != IDLE) {
            state = IDLE;
            qDebug() << "cancelled";
        }
    }

protected:

    enum State { IDLE, RUNNING, PAUSED };
    State state = IDLE;

    bool isCancelled() {
        auto const dispatcher = QThread::currentThread()->eventDispatcher();
        if (!dispatcher) {
            qCritical() << "thread with no dispatcher";
            return false;
        }
        dispatcher->processEvents(QEventLoop::AllEvents);
        return state == IDLE;
    }

};


class MyWorker : public Worker
{
    Q_OBJECT

public:
    explicit MyWorker(QObject *parent = 0)
        : Worker(parent)
    {
    }

    void doWork()
    {
        if (state == PAUSED)
            // treat as resume
            state = RUNNING;

        if (state == RUNNING)
            return;

        state = RUNNING;
        qDebug() << "started";
        emit started();

        // This loop simulates the actual work
        for (auto i = 0u;  state == RUNNING;  ++i) {
            QThread::msleep(100);
            if (isCancelled()) break;
            qDebug() << i;
        }

        qDebug() << "finished";
        emit finished();
    }
};


#include <QCoreApplication>
int main(int argc, char **argv)
{
    QCoreApplication app(argc, argv);

    QThread thread;
    auto worker = new MyWorker();
    worker->moveToThread(&thread);
    QObject::connect(&thread, &QThread::started, worker, &MyWorker::doWork);
    QObject::connect(worker, &MyWorker::finished, worker, &QObject::deleteLater);
    QObject::connect(worker, &QObject::destroyed, &thread, &QThread::quit);
    QObject::connect(&thread, &QThread::finished, &app, &QCoreApplication::quit);

    thread.start();

    // To saved defining more signals, I'll just use invoke() here.
    // Normally, we connect some "driver" object's signals to the
    // pause/resume/cancel slots.
    auto const m = worker->metaObject();
    QThread::sleep(1);
    m->invokeMethod(worker, "pause");
    QThread::sleep(4);
    m->invokeMethod(worker, "resume");
    QThread::sleep(1);
    m->invokeMethod(worker, "cancel");

    app.exec();
}

Sorry this is a bit rushed; I hope the principle is clear.

\$\endgroup\$
2
  • \$\begingroup\$ Undeniably the most useful response, correctly ignoring stylistic differences in favour of pointing out the obvious elephant in the room: namely, that Qt's queued signal-slot connections obviate the need for error-prone thread synchronization. The current top answer is particularly painful in this regard. Pedantic screeds against explicit this-> usage reduce to "this-> Considered Harmful!" – a subjective (and dubious) claim at best. \$\endgroup\$ – Cecil Curry Mar 21 '18 at 7:33
  • 1
    \$\begingroup\$ Actually, this isn't simply useful; this is a brilliant general-purpose treatment of Qt-based multithreading. Your concrete Worker superclass is sufficiently versatile that I'm astonished Qt hasn't already implemented a similar core type (e.g., QThreadWorker). Existing StackOverflow questions on gracefully starting, pausing, resuming, and halting threads and threaded workers (e.g., this, this) now reduce to referencing this canonical answer. Bravo! \$\endgroup\$ – Cecil Curry Mar 21 '18 at 7:50
8
\$\begingroup\$

It's not at all clear why you've declared/defined this:

MyWorker::~MyWorker()
{
}

Unless there's something specific that this accomplishes (which I don't see) you're better off without it.

void MyWorker::restart()
{
    this -> m_pauseRequired = false;
    this -> m_pauseManager.wakeAll();
}

I know there are people here who disagree about this, but they're just wrong. Using this -> without a really good reason is a terrible idea. It makes code noisy and hurts readability. Don't do it if you have any reasonable choice (and in this case, you do).

void MyWorker::pause()
{
    this -> m_pauseRequired = true;
}

Same thing here, obviously.

void MyWorker::doWork()
{
    // Write your code here
    for(int i = 0; i < 4000000000; i++)
    {
        if (i % 100000 == 0) {

What (if anything) are the exact significance of 4000000000 and 100000? If they're significant, document that significance by giving them meaningful names.

It's not entirely clear, but based on the comments, maybe this code was intended to represent the work to be carried out by the "client" code. If so, it seems (to me) like it would be helpful (at least for review purposes) to point that out explicitly.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ I too don't see the need for that destructor. If C++11 was in use, then it could instead be default. \$\endgroup\$ – Jamal Mar 31 '14 at 3:39
  • 1
    \$\begingroup\$ @jamal: no real need for that either. =default is normally for constructors that would normally be synthesized by the compiler, but won't because you've defined some other constructor. \$\endgroup\$ – Jerry Coffin Mar 31 '14 at 3:44
  • \$\begingroup\$ Ah, okay. Would that still apply in cases where you need to maintain the Rule of Four/Five (since we're talking C++11)? \$\endgroup\$ – Jamal Mar 31 '14 at 3:46
  • \$\begingroup\$ @jamal: Not usually, anyway, no. For cases like that, you typically need a destructor and constructor to actually do things (e.g., manage memory owned by the class in question). \$\endgroup\$ – Jerry Coffin Mar 31 '14 at 3:48
  • 1
    \$\begingroup\$ I think m_pauseRequired should be something atomic. \$\endgroup\$ – Kyle Strand Feb 15 '17 at 20:51
3
\$\begingroup\$
  1. You're not locking m_continue before waiting with it.

From the docs of QWaitCondition::wait(QMutex *lockedMutex, ...):

Releases the lockedMutex and waits on the wait condition. The lockedMutex must be initially locked by the calling thread. If lockedMutex is not in a locked state, the behavior is undefined.

So at least you need to do:

        // [...]
        m_continue.lock();
        if (m_pauseRequired) {
            m_pauseManager.wait(&m_continue);
            // unlocks m_continue and blocks the thread until m_pauseManager.wakeAll()
        }
        m_continue.unlock();
  1. I would make m_pauseRequired and m_cancelRequested atomic, e.g. QAtomicInt or std::atomic<bool>. Not much risk here (depending on compiler), but it makes your intentions clearer.
  2. In MyWorker::cancelWork(), set m_cancelRequested before restarting. Thus, if the worker is currently paused, it will find itself canceled as soon as the QWaitCondition resumes the thread. If it's currently running, chances are higher that the worker will not perform another loop.

About your solution: Did you test it? Does it work as expected?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.