Doing QThread-ing the "right" way

Question

With this program I press the 'run' button and a for loop cycles 100 times (with a 100ms delay) and prints the cycle count in a txt field

I have successfully done it with a MyThread object derived from QThread. It works. I can interrupt the cycle with the 'stop' button.

However it is sternly warned that deriving an object from QThread is very bad. So I did it the other way they suggested, the "right" way.

And it doesn't work. I can get the loop cycle numbers out on the console but not into the text box

The 'run' button goes down and never comes up again until the 100 cycles are done. And the text field shows 99.

Here is the code, what am I doing wrong?

// MyWidget.h   SF022

#ifndef MYWIDGET_H_
#define MYWIDGET_H_

#include 
#include 
#include 

#include "../MyThread.h"
#include "../MyObject.h"
#include 

class MyWidget : public QWidget {

    Q_OBJECT

public:
    MyWidget(QWidget* parent = 0);
    ~MyWidget();

    QPushButton* pbRun;
    QPushButton* pbStop;
    QPushButton* pbExit;
    QLineEdit*   txtCount;

    QThread* cThread;

    MyObject* myObject;

public slots:
    void onNumberChanged(int);

private slots:
    void pbRun_Slot();
    void pbStop_Slot();
    void pbExit_Slot();
};

#endif /* MYWIDGET_H_ */
------------------------------------------------
// MyWidget.cpp

#include "MyWidget.h"
#include "../K.h"
#include 

MyWidget::MyWidget(QWidget* parent) : QWidget(parent) {

    pbRun = new QPushButton(this);
    pbRun->setObjectName(QStringLiteral("pbRun"));
    pbRun->setGeometry(QRect(20, 20, 80, 40));
    pbRun->setText("Run");

    connect(pbRun, SIGNAL(clicked()), this, SLOT(pbRun_Slot()));

    pbStop = new QPushButton(this);
    pbStop->setObjectName(QStringLiteral("pbStop"));
    pbStop->setGeometry(QRect(20, 80, 80, 40));
    pbStop->setText("Stop");

    connect(pbStop, SIGNAL(clicked()), this, SLOT(pbStop_Slot()));

    pbExit = new QPushButton(this);
    pbExit->setObjectName(QStringLiteral("pbExit"));
    pbExit->setGeometry(QRect(20, 140, 80, 40));
    pbExit->setText("Exit");

    connect(pbExit, SIGNAL(clicked()), this, SLOT(pbExit_Slot()));

    txtCount = new QLineEdit(this);
    txtCount->setGeometry(QRect(20, 200, 80, 40));
    txtCount->setStyleSheet("QLineEdit{background: white;}");

//  myObject holds the cycling mechanism
    myObject = new MyObject(this);

//  the myObject sends each new cycle number out here
    connect(myObject, SIGNAL(numberChanged(int)), this, SLOT(onNumberChanged(int)));
}

MyWidget::~MyWidget() {
}

void MyWidget::pbRun_Slot() {

//  start thread

    cThread = new QThread(this);
    myObject->doSetup(*cThread);
    myObject->moveToThread(cThread);
    cThread->start();
}

void MyWidget::pbStop_Slot() {

//   stop the thread

   myObject->Stop = true;
}

void MyWidget::pbExit_Slot() {

//  a static pointer to the main window

   (K::SfMainWin)->close();
}

// a slot
void MyWidget::onNumberChanged(int j) {

//  output the cycle count to a text field
    txtCount->setText(QString::number(j));
}
----------------------------------------------------------
// MyObject.h

#ifndef MYOBJECT_H_
#define MYOBJECT_H_

#include 
#include 

class MyObject : public QObject {

    Q_OBJECT

public:
    explicit MyObject(QObject* parent = 0);
    ~MyObject();

    void doSetup(QThread&);

    bool Stop;

signals:
    void numberChanged(int);

public slots:
    void doWork();
};

#endif /* MYOBJECT_H_ */
----------------------------------------------------------
// MyObject.cpp    

#include "MyObject.h"
#include 
#include 
#include "string.h"

MyObject::MyObject(QObject* parent) : QObject(parent) {

    Stop = false;
}

MyObject::~MyObject() {

}

void MyObject::doSetup(QThread& cThread) {

    Stop = false;

    connect(&cThread, SIGNAL(started()), this, SLOT(doWork()));
}

void MyObject::doWork() {

    for (int i = 0; i < 100; i++) {

        QMutex mutex;

        mutex.lock();
        if (this->Stop) {

            break;
        }

//  output into a text field
        emit numberChanged(i);

//  output on the console
        std::cout << "running " << (QString::number(i)).toStdString() << std::endl;

        mutex.unlock();

        QThread::msleep(100);

    }
}

UmNyobe · Accepted Answer

myObject is never moved to the thread you created. Everything is being executed in the main thread. Because

myObject = new MyObject(this);

To move a QObject to another thread, he should not have a parent. If it does Qt will silently tell you something went wrong ( by printing on the output, same with incorrect connections). It is the framework design to not panic over this type of warnings...

It should have been

myObject = new MyObject(0);

Now that this is cleared, you have other defects in the code.

QMutex mutex; is local and will always be acquired by the same thread. which means he has no purpose. Instead it should be a private member of MyObject
MyWidget::pbStop_Slot should be a method of MyObject instead, otherwise you have a race condition when accessing Stop member. Remember the mutex above? It is the time to use it. By the way, with your implementation call directly the method, because the even loop of cThread is only executing doWork
```
MyObject::pbStop_Slot()
{
    mutex.lock()
    Stop = true;
    mutex.unlock()
}
```

Now your program should be technically correct. But It suck that you cannot use signals and slots because your thread is blocked executing doWork. Also, can we do something about that lock. In fact, yes. The way I will go is to use a Qtimer as a heartbeat ech 100ms rather than asking the thread to sleep. But to not change much your code you can use QAbstractEventDispatcher * QAbstractEventDispatcher::instance ( QThread * thread = 0 ) directly

MyObject::pbStop_Slot() //becomes a real slot again
{
    // no more mutex
    Stop = true;
}
....
//this connection is changed
connect(pbStop, SIGNAL(clicked()), myObject, SLOT(pbStop_Slot()));
....

void MyObject::doWork() {

for (int i = 0; i < 100; i++) {

    //no mutex
    if (this->Stop) {

        break;
    }

    //  output into a text field
    emit numberChanged(i);

    //  output on the console
    std::cout << "running " << (QString::number(i)).toStdString() << std::endl;

   //process events, to allow stop to be processed using signals and slots
   QAbstractEventDispatcher::instance(cThread)->processEvents();

    QThread::msleep(100);    
}

}

A warning about processEvents. As it is now, if the user presses run while dowork is being executed it will be called within itself. You have a nasty code right now. An easy way to circumvent this is to put a boolean which is checked and set at the beginning of dowork.
```
dowork(){
  if(isdoingwork)
    return;
  isdoingwork = true
  for(...
```

This is a poor man way of achieving reentrancy . You will see the word reentrant quite often in Qt documentation.

Good luck in your multithreading trip.

Doing QThread-ing the "right" way

Answers (2)

Related Questions

Doing QThread-ing the &quot;right&quot; way

Answers (2)

Related Questions

Doing QThread-ing the "right" way