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();
}

#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();
}

To copy to clipboard, switch view to plain text mode 

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

#ifndef MYTHREAD_H
#define MYTHREAD_H

#include <QtCore>

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

#endif // MYTHREAD_H

To copy to clipboard, switch view to plain text mode 

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;
    }
}

#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;
    }
}

To copy to clipboard, switch view to plain text mode 

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;
    }
}

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;
    }
}

To copy to clipboard, switch view to plain text mode 

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);
    }
}

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);
    }
}

To copy to clipboard, switch view to plain text mode 

[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?



Thread_with_terminate.pro

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

HEADERS += \
    threaddialog.h \
    mythread.h

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

To copy to clipboard, switch view to plain text mode 

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();
}

#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();
}

To copy to clipboard, switch view to plain text mode 

mythread.h

To copy to clipboard, switch view to plain text mode 

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

#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

To copy to clipboard, switch view to plain text mode 

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

#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

To copy to clipboard, switch view to plain text mode 

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));
}

#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));
}

To copy to clipboard, switch view to plain text mode 

[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

#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

To copy to clipboard, switch view to plain text mode 

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);
}

#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);
}

To copy to clipboard, switch view to plain text mode 

[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