Quit thread using signal/slot

[mounte]

Hi, I have a simple problem which I have not been able to solve yet. At least not in the way I think is nicer...

The setup is a simple worker object that should be run in a different thread. I connect the thread start signal to the worker slot function that should be run and I connect the worker done signal to the thread terminate slot. Then I move the worker to the thread using moveToThread().

I want to wait for the thread, t, to finish so I call:
t->start();
t->wait();

The work is done, but the program never gets through the wait-condition. So somehow the emitted signal is not triggering the thread terminate slot.

If I instead call thread()->quit() or thread->exit() instead of emitting the signal everything works.
If I try and connect to terminate or quit does not matter.

Here is some sample code, please help me/tell me what the "right" way to solve this problem is and how/why

Best regards //
Daniel

Header:

#ifndef MAINWINDOW_H
#define MAINWINDOW_H
 
#include <QMainWindow>
#include <QUrl>
#include <QThread>
#include <QNetworkAccessManager>
#include <QNetworkReply>
#include <QNetworkRequest>
#include <QDebug>
 
namespace Ui {
    class MainWindow;
}
 
class DownloadWorker : public QObject
{
    Q_OBJECT
public:
    explicit DownloadWorker(QObject *parent = 0)
        : QObject(parent)
    {
        qDebug() << "Creating worker";
    }
    QByteArray* getba() {return &ba;}
private:
    QString url;
    QByteArray ba;
public slots:
    void slotWork();
    void slotWorkDone(QNetworkReply *reply);
    void prepareDownload(QString url)
    {
        this->url = url;
    }
 
signals:
    void workDone();
};
 
class MainWindow : public QMainWindow
{
    Q_OBJECT
 
public:
    explicit MainWindow(QWidget *parent = 0);
    ~MainWindow();
 
private:
    Ui::MainWindow *ui;
};
 
#endif // MAINWINDOW_H

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include <QMainWindow>
#include <QUrl>
#include <QThread>
#include <QNetworkAccessManager>
#include <QNetworkReply>
#include <QNetworkRequest>
#include <QDebug>

namespace Ui {
    class MainWindow;
}

class DownloadWorker : public QObject
{
    Q_OBJECT
public:
    explicit DownloadWorker(QObject *parent = 0)
        : QObject(parent)
    {
        qDebug() << "Creating worker";
    }
    QByteArray* getba() {return &ba;}
private:
    QString url;
    QByteArray ba;
public slots:
    void slotWork();
    void slotWorkDone(QNetworkReply *reply);
    void prepareDownload(QString url)
    {
        this->url = url;
    }

signals:
    void workDone();
};

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    explicit MainWindow(QWidget *parent = 0);
    ~MainWindow();

private:
    Ui::MainWindow *ui;
};

#endif // MAINWINDOW_H

To copy to clipboard, switch view to plain text mode 

#include "mainwindow.h"
#include "ui_mainwindow.h"
 
void DownloadWorker::slotWork()
{
    QNetworkAccessManager *man = new QNetworkAccessManager(this);
    connect(man, SIGNAL(finished(QNetworkReply*)),
            this,SLOT(slotWorkDone(QNetworkReply*)));
    man->get(QNetworkRequest(QUrl(this->url)));
}
 
void DownloadWorker::slotWorkDone(QNetworkReply *reply)
{
    reply->deleteLater();
    ba = reply->readAll();
    qDebug() << "Download done, exit thread?";
    emit workDone();
    //thread()->exit(); //things work with this but is it the "right" way to do it?
}
 
MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent), ui(new Ui::MainWindow)
{
    ui->setupUi(this);
    QThread *t = new QThread();
    DownloadWorker *dw = new DownloadWorker();
    dw->prepareDownload("http://www.google.com");
    dw->moveToThread(t);
    connect(t, SIGNAL(started()),
            dw, SLOT(slotWork()));
    connect(dw, SIGNAL(workDone()),
            t, SLOT(terminate()));
    t->start();
    qDebug() << "Waiting for thread...";
    qDebug() << *dw->getba();
    t->wait();
    qDebug() << "...done!";
    qDebug() << *dw->getba();
 
 
}
 
MainWindow::~MainWindow()
{
    delete ui;
}

#include "mainwindow.h"
#include "ui_mainwindow.h"

void DownloadWorker::slotWork()
{
    QNetworkAccessManager *man = new QNetworkAccessManager(this);
    connect(man, SIGNAL(finished(QNetworkReply*)),
            this,SLOT(slotWorkDone(QNetworkReply*)));
    man->get(QNetworkRequest(QUrl(this->url)));
}

void DownloadWorker::slotWorkDone(QNetworkReply *reply)
{
    reply->deleteLater();
    ba = reply->readAll();
    qDebug() << "Download done, exit thread?";
    emit workDone();
    //thread()->exit(); //things work with this but is it the "right" way to do it?
}

MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent), ui(new Ui::MainWindow)
{
    ui->setupUi(this);
    QThread *t = new QThread();
    DownloadWorker *dw = new DownloadWorker();
    dw->prepareDownload("http://www.google.com");
    dw->moveToThread(t);
    connect(t, SIGNAL(started()),
            dw, SLOT(slotWork()));
    connect(dw, SIGNAL(workDone()),
            t, SLOT(terminate()));
    t->start();
    qDebug() << "Waiting for thread...";
    qDebug() << *dw->getba();
    t->wait();
    qDebug() << "...done!";
    qDebug() << *dw->getba();


}

MainWindow::~MainWindow()
{
    delete ui;
}

To copy to clipboard, switch view to plain text mode 

[wysota]

You can substitute your whole code with:

QNetworkAccessManager manager;
QEventLoop q;
connect(&manager, SIGNAL(finished(QNetworkReply*)), &q, SLOT(quit()));
QNetworkReply *reply = manager.get(QNetworkRequest(QUrl("http://www.google.com")));
q.exec();
QByteArray data = reply->readAll();
reply->deleteLater();

QNetworkAccessManager manager;
QEventLoop q;
connect(&manager, SIGNAL(finished(QNetworkReply*)), &q, SLOT(quit()));
QNetworkReply *reply = manager.get(QNetworkRequest(QUrl("http://www.google.com")));
q.exec();
QByteArray data = reply->readAll();
reply->deleteLater();

To copy to clipboard, switch view to plain text mode 

No threads, no blocked GUI, no synchronisation issues, no problem. You can also add a timer to the whole combination to prevent waiting too long until a failed connection timeouts.

[mounte]

Hi and thank you for your quick reply. The solution works... but if you are downloading large files or over slow connections the application will wait until the event loop is finished which is not optimal.

I reckon the sample I provided may not be the best, but it is somewhat in the direction I am striving.
So still my question is yet to be answered: Why is not the terminate slot called from the emitted signal? What can be changed so that the sample I wrote works? Is the best solution to manually call quit/exit on the thread from within the worker?

In the complete application there will be one main thread which spawns two(or more) other threads. From one of these threads workers will be created and it is this thread that I would like to wait until the worker-thread is done.

[wysota]

Hi and thank you for your quick reply. The solution works... but if you are downloading large files or over slow connections the application will wait until the event loop is finished which is not optimal.

Correct me if I'm wrong but the same will happen with your code:

t.start();
t.wait(); // waits until "t" dies

t.start();
t.wait(); // waits until "t" dies

To copy to clipboard, switch view to plain text mode 

I reckon the sample I provided may not be the best, but it is somewhat in the direction I am striving.

Trust me, it is not.

So still my question is yet to be answered: Why is not the terminate slot called from the emitted signal?

Because the (main) thread that is to call the slot is waiting for your thread to finish which will never happen because quitting the slot requires the thead owning the thread controller to be alive which will never happen because you blocked it until the thread dies. Do you see the infinite loop?

What can be changed so that the sample I wrote works?

The only viable solution I see is to not use a thread because you don't need it for anything.

I strongly recommend to read this article: [wiki]Keeping the GUI Responsive[/wiki].

If you don't want the application to wait then simply remove the event queue from my code and use signals and slots. Threads don't solve anything here, especially if you block the calling thread.

[mounte]

Thank you for your answer.
It now makes sense, my "solution" (that really would not have worked very well or not at all) would have been really awful I realize.

Correct me if I'm wrong but the same will happen with your code:

t.start();
t.wait(); // waits until "t" dies

t.start();
t.wait(); // waits until "t" dies

To copy to clipboard, switch view to plain text mode 

Yes this would block, the idea was that the main thread and my other worker thread would still be alive. The blocking would be of just one thread.
And my own stupidity lead me in the wrong direction instead of keeping it simple.

Thank you very much for your help and taking your time with me, your recommended link was also appreciated.

[wysota]

A general rule is that if you block one thing to do another in a thread then you don't need the thread. Another is that network operations in Qt don't benefit from using threads.