Why I cannot stop a thread?

[8Observer8]

Hi,

I cannot stop a thread.

main.cpp


#include <QCoreApplication>
#include "mythread.h"

int main(int argc, char *argv[])
{
QCoreApplication a(argc, argv);

MyThread mThread1;
mThread1.name = "mThread1";

mThread1.start();

mThread1.stop = true;

return a.exec();
}


mythread.h


#ifndef MYTHREAD_H
#define MYTHREAD_H

#include <QtCore>

class MyThread : public QThread
{
public:
MyThread();
void run();
QString name;
bool stop;
};

#endif // MYTHREAD_H


mythread.cpp


#include "mythread.h"
#include <QDebug>

MyThread::MyThread()
{
}

void MyThread::run() {
qDebug() << this->name << " Running";

QMutex mutex;
mutex.lock();
this->stop = false;
mutex.unlock();

for (int i = 0; i < 1000; i++) {
QMutex mutex;
mutex.lock();
if (this->stop) {
break;
}
mutex.unlock();
this->sleep(1);
qDebug() << this->name << " " << i;
}
}


It's the example from this video: http://www.youtube.com/watch?v=5WEiQ3VJfxc

Thank you!

[alizadeh91]

Write a stop() method in thread and call terminate method of thread

void stop()
{
terminate();
wait(500);
}

[wysota]

QMutex mutex;
mutex.lock();
this->stop = false;
mutex.unlock();
for (int i = 0; i < 1000; i++) {
QMutex mutex;
mutex.lock();
if (this->stop) {
break;
}
mutex.unlock();
this->sleep(1);
qDebug() << this->name << " " << i;
}
}


This code makes no sense, by the way, especially use of the mutexes.

[8Observer8]

Write a stop() method in thread and call terminate method of thread

void stop()
{
terminate();
wait(500);
}

Yes, I will do it later. Why is "wait(500);" here?

[wysota]

If you "do that later" then "later" it might work and now it won't.

[8Observer8]

Why is it incorrect code?



void MyThread::run () {
QMutex mutex;
mutex.lock();
this->stop = false;
mutex.unlock();

for (int i = 0; i < 1000; i++) {
QMutex mutex;
mutex.lock();
if (this->stop) break;
mutex.unlock();

this->msleep(100);

emit numberChanged(i);
}
}

[wysota]

Why is it incorrect code?

Your mutexes are not protecting anything. For a mutex to work two or more threads have to access the same mutex.

By the way, you're code can be rewritten to work without any mutex, without any for loop and without having to check any stop condition. And definitely without this ugly sleep call.

[8Observer8]

Thank you!

Is it right?

http://i.pixs.ru/storage/1/0/3/58png_7343135_8863103.png (http://pixs.ru/?r=8863103)

Thread_with_terminate.pro


HEADERS += \
threaddialog.h \
mythread.h

SOURCES += \
threaddialog.cpp \
mythread.cpp \
main.cpp


main.cpp


#include "threaddialog.h"
#include <QApplication>

int main(int argc, char *argv[])
{
QApplication a(argc, argv);

ThreadDialog w;
w.resize(100, 50);
w.show();

return a.exec();
}


mythread.h




mythread.cpp


#ifndef MYTHREAD_H
#define MYTHREAD_H

#include <QtCore>

class MyThread : public QThread
{
Q_OBJECT
public:
explicit MyThread(QObject *parent = 0);
void run();
void stop();

signals:
void numberChanged(int);

public slots:

};

#endif // MYTHREAD_H


threaddialog.h


#ifndef THREADDIALOG_H
#define THREADDIALOG_H

#include <QDialog>
#include <QtGui>
#include "mythread.h"

class ThreadDialog : public QDialog
{
Q_OBJECT
public:
explicit ThreadDialog(QWidget *parent = 0);

signals:

public slots:
void numberChanged(int);

private slots:
void on_startButton_clicked();
void on_stopButton_clicked();

private:
QLabel *numberLabel;
MyThread *myThread;
};

#endif // THREADDIALOG_H


threaddialog.cpp


#include "threaddialog.h"

ThreadDialog::ThreadDialog(QWidget *parent) :
QDialog(parent)
{
numberLabel = new QLabel(tr("Number"));
QPushButton *startButton = new QPushButton(tr("Start"));
QPushButton *stopButton = new QPushButton(tr("Stop"));

QHBoxLayout *mainLayout = new QHBoxLayout(this);
mainLayout->addWidget(numberLabel);
mainLayout->addStretch();
mainLayout->addWidget(startButton);
mainLayout->addWidget(stopButton);

connect(startButton, SIGNAL(clicked()), this, SLOT(on_startButton_clicked()));
connect(stopButton, SIGNAL(clicked()), this, SLOT(on_stopButton_clicked()));

myThread = new MyThread(this);
connect(myThread, SIGNAL(numberChanged(int)), this, SLOT(numberChanged(int)));
}

void ThreadDialog::on_startButton_clicked()
{
myThread->start();
}

void ThreadDialog::on_stopButton_clicked()
{
myThread->stop();
}

void ThreadDialog::numberChanged(int number)
{
numberLabel->setText(QString::number(number));
}

[wysota]

I don't see code for the thread anywhere but regardless, it's still likely not what I had in mind (based on what I see).

[8Observer8]

Sorry!

mythread.h


#ifndef MYTHREAD_H
#define MYTHREAD_H

#include <QtCore>

class MyThread : public QThread
{
Q_OBJECT
public:
explicit MyThread(QObject *parent = 0);
void run();
void stop();

signals:
void numberChanged(int);

public slots:

};

#endif // MYTHREAD_H


mythread.cpp


#include "mythread.h"
#include <QtCore>

MyThread::MyThread(QObject *parent) :
QThread(parent)
{
}

void MyThread::run () {
for (int i = 0; i < 1000; i++) {

this->msleep(100);

emit numberChanged(i);
}
}

void MyThread::stop() {
this->terminate();
wait(500);
}

[wysota]

Yeah... not what I had in mind.

[anda_skoa]

Write a stop() method in thread and call terminate method of thread

void stop()
{
terminate();
wait(500);
}

That is probably the worst possible suggestion ever given to someone wanting to stop a thread.
Repeat after me: never ever call terminate() on a thread!

The correct way is to indicate the wish to stop, e.g. by setting a stop flag, and then have the thread exit itself. Termination can leave mutexes locked, leak memory due to shutdown code not being executed, etc.
If you call terminate() you should be aware that your program might crash or lock up.

The original program's problem is that MyThread::run() resets the stop flag to false. Initialisation of the stop flag has to be done in MyThread's constructor.
Obviously setting it from main without locking is also an issue, but not as grave as overwriting the value set by the main thread.

So the actually recommendable solution to the problem is to
1) initialize the stop flag in the constructor
2) make the stop() method set the stop flag to true using mutex protection
3) check the flag in run() using mutex protection.

Cheers,
_

[8Observer8]

Thank you very much :)