[Solved] QThread blocking GUI

[eMixam]

Hello everybody,

I have an application that does some work on data recieved through a serial port to display it on a QTableWidget.
The GUI is able to start and stop the sending of data with two buttons.

I am not the initial developper of the application.
I am trying to implement the "good way" of doing threaded actions as it isn't thread-safe for now.

Here is how it currently (doesn't) work with my modifications.
It can be simplified by a worker object and a mainwindow application, note that the worker object owns a pointer on its gui.

@worker.h
class Worker : Public QObject
{
QObject

public:
Worker();
~Worker();

public slots:
void read_data();
void process_data();
void pause_processing();

signals:
void emit_data(QStringList);

private:
Gui *gui; // (QWidget *)
bool running;
// VARS

bool cmd_SendData(); // Commands the device to start sending data
void cmd_StopSendData(); // ...
};
@

@worker.cpp
// CONSTRUCTOR
Worker::Worker()
{
gui = new Gui();

connect(gui, SIGNAL(start()), this, SLOT(read_data()));
connect(gui, SIGNAL(stop()), this, SLOT(pause_processing()));

connect(this,SIGNAL(emit_data(QString)), gui, SLOT(setRow(QString)));
}

void read_data()
{
if(cmd_SendData())
{
running = true;
while(running)
{
process_data();
// needs a way to sleep for 10ms
}
}
}

void pause_processing()
{
running = false;
cmd_StopSendData();
}

// other functions
@

@mainwindow.cpp

// ...

MainWindow::createWorker()
{
worker = new Worker();
thread = new QThread();

worker->moveToThread(thread);

thread->start();
}

// ...
@

I know the implementation isn't "the good way" but I tried to comply to the existing code.

Now I have a thread started, within it signals are connected from the gui to my object processing data but, when my application starts processing data, the main gui doesn't respond anymore.

Does it have a relation to the fact that the worker's gui is allocated within it's constructor ?
I read that when allocating objects on the constructor of the worker these objects are then handled by the main thread (or the thread in which the parent object is allocated).
So my GUI shouldn't be blocked by operations of the processing thread right ?

I also tried by putting the QThread in the Worker object but the results are the same.
I think I misunderstood something in QThread(s) but I can't seem find what.

I hope I made myself clear.
Thanks in advance for any help.

[MuldeR]

Regarding mainwindow.cpp: How do you actually give the worker some work to do?

I see you start() a standard QThread, which will simply start the EventLoop inside the new thread.

Also you "moved" the Worker to the context of the new thread.

But where does the Worker object get the signal to actually begin doing some work?

Finally: Why inside the Worker you create a "Gui"? Do not do any GUI stuff in a non-main thread!

[kuzulis]

Use "QtSerialPort":http://qt-project.org/wiki/QtSerialPort.

[eMixam]

[quote author="MuldeR" date="1357666209"]Regarding mainwindow.cpp: How do you actually give the worker some work to do?

I see you start() a standard QThread, which will simply start the EventLoop inside the new thread.

Also you "moved" the Worker to the context of the new thread.

But where does the Worker object get the signal to actually begin doing some work?[/quote]

The worker is given work by the signal start() emitted by the gui when the user press a "read" button.

[quote author="MuldeR" date="1357666209"]Finally: Why inside the Worker you create a "Gui"? Do not do any GUI stuff in a non-main thread![/quote]

As I said, I am not the original creator of this application and if possible, I would like to avoid rewriting everything.

This is used in a QTabWidget so that every QWidget has it's own class, then a *QWidget getWidget() method is used to set the gui in the main window QTabWidget's content QWidget.

@worker.cpp
QWidget *Worker::getWidget()
{
return (QWidget *) gui;
}@
@mainwindow.cpp
//...
ui->worker_widget = worker->getWidget();
//...
@

What I actually thought is that when creating the gui with new the gui would remain in the main thread. When the Worker object is created, it is owned by the main threaded right ? Then so is his child Gui Object.

My real question would then be :
Will worker->moveToThread(thread) move also the child Gui QWidget in the new thread or will it stay in the main thread ?

[quote author="kuzulis" date="1357667593"]Use "QtSerialPort":http://qt-project.org/wiki/QtSerialPort.[/quote]

It's a previous version than the one in Qt5 but it's actually what is used.

Thanks for helping.

[KA51O]

Move everything you do in the constructor of the Worker into the MainWindow::createWorker() function. You are not allowed to do anything GUI related in another thread then the main thread.
Did you already see "this example":http://qt-project.org/wiki/QThreads_general_usage for a worker object approach with QThread ?

[eMixam]

[quote author="KA51O" date="1357718103"]Move everything you do in the constructor of the Worker into the MainWindow::createWorker() function. You are not allowed to do anything GUI related in another thread then the main thread.
Did you already see "this example":http://qt-project.org/wiki/QThreads_general_usage for a worker object approach with QThread ?[/quote]

I already read this example from the original blog.

I also moved everything gui-related from my Worker to my MainWindow.
Nothing changed, I really don't understand : now the Gui object is owned by the MainWindow and everything is handled by signals...

@mainwindow.cpp
void MainWindow::createWorker()
{

worker = new Worker();
worker_gui = new Gui();
thread = new QThread();

worker->moveToThread(thread);

ui->worker_widget = worker_gui;

connect(worker_gui, SIGNAL(start()), worker, SLOT(read_data()));
connect(worker_gui, SIGNAL(stop()), worker, SLOT(pause_processing()));
connect(worker, SIGNAL(emitData(QStringList)), worker_gui, SLOT(setRow(QStringList)));

thread->start();

}@

[KA51O]

Hmm. What does process_data() do ? Can you please post the updated Worker code ?

[eMixam]

process_data() recieves the data from the serial bus using a Communication class and adapts it to the desired format. It finally sends the data to the gui using emit_data(QStringList).

What surprises me is that the old design worked, except for the segfaults (probably coming from the non-thread-safe implementation).

The worker code wouldn't be relevant as there is nothing in the constructor now. The main concern would be the while(running) infinite loop in read_data().

[KA51O]

Infinite loops in an event-driven framework are not really fitting. They block the event queue! Maybe in the old version this was no problem because the worker (or subclassed thread or whatever) didn't need to receive any signals or didn't rely on the eventloop.
So the last thing I can recommend would be to try and refactor the code so you don't need the inifinite loop (try solving it with signals and slots).

[KA51O]

But then again this should only block the eventloop of the worker thread and not the main thread.

[eMixam]

Ok thank you very much.

My gui now reacts, I used the QCoreApplication::processEvents() that I found "here":http://qt-project.org/wiki/Threads_Events_QObjects. So now my signals are correctly sent.
But that does not seem to be the correct way of handling things and, as you said I still don't understand why it kept blocking the main thread.

[KA51O]

Do you call anything that is living in the context of the main thread directly (not using signal slot) from out of the process_data() function in your worker thread ?

Be aware that calling QCoreApplication::processEvents() in an infinite loop will raise the processor workload to 100 %.

EDIT: It almost seems as if the infinite loop in your worker is running in the context of the main thread (although you moved it to the worker thread).

[eMixam]

Okay, now I think I found it, the communication module is of course shared between each functionalities.

Looks like I'll have to refactor quite a lot of code.

Again, thank you for your help.

[KA51O]

Glad I could help a little. I hope its not too much lines of code you have to refactor. The most important thing is to always use signal slot connections to communicate between different threads.

[eMixam]

I just finished refactoring the read and pause features (now I need to refactor all the others so they are complying to this model). Everything is now handled by signals.
I remplaced the infinite loop by a QTimer so now I can stop() and start() in an easy and most convenient way.