QTimer / QThread question

[Galen]

I have an application that sorts arrays of ints (6 at a time). There is a main QWidget that has 6 subclassed QWidgets that handle the drawing of the sort progress. There is also another thread that handles all the signals from buttons and sliders. This one creates the 6 sort threads and also a "ThreadWatcher" thread. This last one calls update() for the corrosponding QWidget while the sort threads are alive and updates QLabels after the sort threads exit. Originally I just had a loop that checked each one and called update() and then slept for 10ms (which worked), but I decided to try to do it all with slots.

The code below gives the following errors:

"QObject: Cannot create children for a parent that is in a different thread. (Parent is ThreadWatcher2(0x221b888), parent's thread is QThread(0x3f4ae0), current thread is ThreadWatcher2(0x221b888)" -- on start but this still works fine.

"QObject::killlTimer: timers cannot be stopped from another thread" -- when I try to pause, this doesn't work.

Attempting to kill (setting control[1]) doesn't work (here, on the loop-based method it did), but I can probably figure that one out.

I'll leave out everything before run(), so here's the revelant variables. I also left out the other 5 finished() handlers. If there's some way to create a single handler with an (int) to differentiate which I'd be interested in that as well, as they are all identical except the array index. I'm guessing QOBject::moveToThread() is the solution (if there is one) but I'm unclear on the specifics. updateTimer->moveToThread(this) after allocation in run() doesn't seem to help.

alive[6] = bool array, marked to false in finished() handler for each thread
window[6] = references to the QWidgets to call update() on
thread[6] = array of sort thread references
control[2] = volatile bool array, [0] for pause and [1] for kill, set in (ommitted) parent thread
threadsLeft = starts a 6, decremented in each finished() handler
lock / condition = QMutex / QWaitCondition set by parent to pause


void ThreadWatcher2::run() {
updateTimer = new QTimer(this);
connect(updateTimer, SIGNAL(timeout()), this, SLOT(timeoutHandler()));
connect(thread[0], SIGNAL(finished()), this, SLOT(finishedHandler0()));
connect(thread[1], SIGNAL(finished()), this, SLOT(finishedHandler1()));
connect(thread[2], SIGNAL(finished()), this, SLOT(finishedHandler2()));
connect(thread[3], SIGNAL(finished()), this, SLOT(finishedHandler3()));
connect(thread[4], SIGNAL(finished()), this, SLOT(finishedHandler4()));
connect(thread[5], SIGNAL(finished()), this, SLOT(finishedHandler5()));
updateTimer->start(delay);

QThread::exec();
status->setText("Status: Done");
}
void ThreadWatcher2::timeoutHandler() {
for (int i=0; i<6; i++) {
if (alive[i])
window[i]->update(); }

if (threadsLeft==0) {
QThread::exit(0); }

if (control[1])
QThread::exit(1);
else if (control[0]) {
updateTimer->stop();
lock->lock();
condition->wait(lock);
lock->unlock();
updateTimer->start(delay);}
}
void ThreadWatcher2::finishedHandler0() {
disconnect(SIGNAL(finished()), this, SLOT(finishedHandler0()));
alive[0] = false;
threadsLeft--;
seconds[0] = (float)runTime[0] / 1000.0f;
time[0]->setText("Time: "+QString::number(seconds[0],'g',3));
}
void ThreadWatcher2::finishedHandler1() {
.
.
.

thanks

[JohannesMunk]

Hi Galen!

1. You should have a look at QtConcurrent (http://doc.trolltech.com/latest/qtconcurrent.html) and the qt concurrent examples. That would save you a lot of effort!

2. instead of your 6 finishedHandlers look at QSignalMapper (http://doc.trolltech.com/latest/qsignalmapper.html).

3.
void ThreadWatcher2::finishedHandler0() {
disconnect(SIGNAL(finished()), this, SLOT(finishedHandler0()));
There is no need here, to manually disconnect this. Finished is only called once from the thread.
And signal/slots are automatically disconnected as soon as one of the connected
objects is destroyed.

4. As for your QObject Warning. It says, what you did wrong. Do it otherwise :-> If you can't find it, you need to provide your setup code..

HIH

Johannes

[Galen]

Thanks for the info, QSignalMapper does look like the solution to that issue, I will give it a try.

I definately want OS-based threads as this is a code sample based around that. I did find that I could get around all of this by creating the timer in the parent class and just stopping it there in the pause button handler. Accessing the control array in the threadwatcher handler just didn't seem to want to work right (is there something special about the scope from within a slot as opposed to an average function?) so I guess its just as well. I'm not sure how much having a 10ms timer taxes the parent (if at all) if the signals are all going to a child thread. The signal is actually connected in the child thread if that makes any difference.

One more question... I'm confused whether I should be choosing Qt::DirectConnection or Qt::QueuedConnection with these slots. Is there something potentially unsafe about direct connections? What happens if the thread is executing another slot when the signal is delivered?

thanks

[JohannesMunk]

1. Don't specify the connection type! Qt choses it well! Direct-Connection slots are directly executed in the context of the calling thread, whereas queued-connections are queued into the destination threads message queue and executed when they are retrieved by that destination thread. If you don't specify anything, qt choses automatically. If source and destination reside in different threads queuedconnection is used, otherwise direct.

2. QtConcurrent spawns threads.

3. I don't get what you are doing with your timers.

Johannes

[Galen]

Thanks, I'll take a look, though It's working well now.. just not the way I have it above.

Well, basically I want the screen to refresh every 10 ms. So I made a threadwatcher class with a 10 ms timer. I decided I wanted to seperate it from the button handlers and such, so I made it an independent thread that creates a QTimer. Starting the timer there complained but still worked. The only thing is that you can pause the entire application. I suppose I could just let it keep refreshing the screen anyway but I'd rather not. So when the pause button is pressed, the timer needs to be stopped or interrupted somehow. The puse button handler is in the parent thread. With the sort threads, and interrupt is requested by the parent changing a shared variable (control[0], which is control[i][0] in the parent, with i being the thread number). In this code sample, that same mechanism doesn't work. I have no idea why, but changes to the variable don't propograte to the timeout slot. They do get to run(), but in the slot function they're always false. Also even if that would work I would need to stop the timer from within the timeout slot to pause it, and that doesn't seem to want to work either. I can see from the "Similar Threads" below that using QTimers in threads is problematic, so maybe I should just be glad I found another way to make it work even if it seems a bit clunkier (parent controls timer externally).

There really isn't anything more to the code for this class. The construtor sets lots of variables, but they are all JLabel references and the like. The timer is a member variable reference and the actual object is created in run().

[Galen]

Looks like new QTimer() instead of new QTimer(this) takes care of the error message, but it seems to consider the slot a different thread, so that doesn't help too much with stopping the timer from within the slot.

[Galen]

Well, here's another problem. When all delay is removed the sorting is instantaneous. In this case 1-2 of the signals dissapear. The commented out portion uses the QSignalMapper, which has the same problem. I tried the Qt::QueuedConnection option but no help. Any ideas? I'd guess you can't max out the event loop so easily.


void ThreadWatcher2::run() {
/* QSignalMapper* signalMapper = new QSignalMapper();
for (int i=0; i<6; i++) {
connect(thread[i], SIGNAL(finished()), signalMapper, SLOT(map()));
signalMapper->setMapping(thread[i], i); }
connect(signalMapper, SIGNAL(mapped(int)), this, SLOT(finishedHandler(int)));
*/
connect(updateTimer, SIGNAL(timeout()), this, SLOT(timeoutHandler()));
connect(thread[0], SIGNAL(finished()), this, SLOT(finishedHandler0()));
connect(thread[1], SIGNAL(finished()), this, SLOT(finishedHandler1()));
connect(thread[2], SIGNAL(finished()), this, SLOT(finishedHandler2()));
connect(thread[3], SIGNAL(finished()), this, SLOT(finishedHandler3()));
connect(thread[4], SIGNAL(finished()), this, SLOT(finishedHandler4()));
connect(thread[5], SIGNAL(finished()), this, SLOT(finishedHandler5()));

QThread::exec();
}
void ThreadWatcher2::timeoutHandler() {
for (int i=0; i<6; i++)
window[i]->update();

if (threadsLeft==0)
QThread::exit(0);
}
void ThreadWatcher2::finishedHandler(int i) {
updateLabels(i);
}
void ThreadWatcher2::finishedHandler0() {
updateLabels(0);
}
void ThreadWatcher2::finishedHandler1() {
updateLabels(1);
}
void ThreadWatcher2::finishedHandler2() {
updateLabels(2);
}
void ThreadWatcher2::finishedHandler3() {
updateLabels(3);
}
void ThreadWatcher2::finishedHandler4() {
updateLabels(4);
}
void ThreadWatcher2::finishedHandler5() {
updateLabels(5);
}
inline void ThreadWatcher2::updateLabels(int t) {
alive[t] = false;
threadsLeft--;
seconds[t] = (float)runTime[t] / 1000.0f;
if (seconds[t] == 0.0f)
seconds[t] = 0.001f;
time[t]->setText("Time: "+QString::number(seconds[t],'g',3));

if (isFirst) {
isFirst = false;
long minTime = runTime[t];
min = t;
long x;

for (int k=0; k<6; k++) {
x = runTime[k];
if ((x > -1) && (x < minTime)) {
minTime = x;
min = k; } }

if (min != t) {
seconds[min] = (float)runTime[min] / 1000.0f;
if (seconds[min] == 0.0f)
seconds[min] = 0.001f; }

if (min == t)
ratio[t]->setText("Ratio: 1x");
else {
ratio[t]->setText("Ratio: "+QString::number(seconds[t]/seconds[min],'g',3)+"x"); } }
else
ratio[t]->setText("Ratio: "+QString::number(seconds[t]/seconds[min],'g',3)+"x");
}

thanks

[Galen]

Duh, nevermind, I still had the sorters starting before the threadwatcher, reversing that fixed it.

[Galen]

Apparently moving the thread object to the thread was what I really needed to do, otherwise the slots run in the parent's thread.


threadWatcher->moveToThread(threadWatcher);
threadWatcher->start(QThread::NormalPriority);
I'm still a little unclear how bad making function calls to gui objects (like QLabels) from another thread is. Seems to work fine, but I guess I can always use signals or make mutex-protected mutator functions in the gui thread instead.

[squidge]

I put the "moveToThread" call in the threads constructor, that way I don't forget the call if I create multiple copies of the class.

[wysota]

I'm still a little unclear how bad making function calls to gui objects (like QLabels) from another thread is.
It's VeryBad(TM).


Seems to work fine,
Then maybe the calls are not made from worker threads after all.

but I guess I can always use signals or make mutex-protected mutator functions in the gui thread instead.
Hard to say without knowing what you are trying to do but I can tell you one thing - you won't be able to protect widgets with mutexes.

[Galen]

Thanks. Well, "VeryBad(TM)" sounds bad, so I guess I'll just emit signals with a QString argument to the gui instead. What I meant was make methods in the main qwdiget to change the contents and lock a mutex at the beginning of it and unlock at the end. That would protect everything inside in a rather crude way, and as long as that is the only method that changes those objects that would be sufficient, right? But then again, this still involves function calls to the gui from another thread. Is this inherently problematic?

I think the slots must be running in the threadwatcher thread. Here's QThread::currentThread() at various points.

main 0x6f51a8
sw 0x6f51a8
sorter 0x6f51a8
tw-r 0x2222e40
tw-h 0x2222e40

Main creates a SortWindow (extends QWidget) and a Sorter (extends QObject) object. The latter has handlers for the buttons and such (I actually tried to make this a thread for awhile but realized this wasn't a great idea). The start button handler launched 6x sort threads and the threadWatcher thread. Tw-r is sampled in run() and Tw-h is in the finishedHandler slot. Pretty conclusive, no?


void ThreadWatcher::run() {
QTextStream cout(stdout, QIODevice::WriteOnly);
cout << "tw-r " << QThread::currentThread() << endl;
QSignalMapper threadMapper;
updateTimer = new QTimer();

for (int i = 0; i < 6; i++) {
connect(thread[i], SIGNAL(finished()), &threadMapper, SLOT(map()));
threadMapper.setMapping(thread[i], i);}

connect(&threadMapper, SIGNAL(mapped(int)), this, SLOT(finishedHandler(int)));
connect(updateTimer, SIGNAL(timeout()), this, SLOT(timeoutHandler()));

connect(this, SIGNAL(refresh(int)), window, SLOT(draw(int)));

updateTimer->start(timerVal);

QThread::exec();

delete updateTimer;
}
void ThreadWatcher::timeoutHandler() {

for(int i=0; i<6; i++) {
if (alive[i])
emit refresh(i); }

if ((threadsLeft==0) || (control[1])) {
QThread::exit(0); }

else if (control[0]) {
lock->lock();
updateTimer->stop();
condition->wait(lock);
lock->unlock();
updateTimer->start(timerVal); }
}
void ThreadWatcher::finishedHandler(int t) {
QTextStream cout(stdout, QIODevice::WriteOnly);
cout << "tw-h " << QThread::currentThread() << endl;
emit refresh(t);
alive[t] = false;
threadsLeft--;

seconds[t] = (float)runTime[t] / 1000.0f;
if (seconds[t] == 0.0f)
seconds[t] = 0.001f;
time[t]->setText("Time: "+QString::number(seconds[t],'g',3));
if (isFirst) {
isFirst = false;
long minTime = runTime[t];
min = t;
long x;

for (int k=0; k<6; k++) {interval)
x = runTime[k];
if ((x > -1) && (x < minTime)) {
minTime = x;
min = k; } }

if (min != t) {
seconds[min] = (float)runTime[min] / 1000.0f;
if (seconds[min] == 0.0f)
seconds[min] = 0.001f; }

if (min == t)
ratio[t]->setText("Ratio: 1x");
else {
ratio[t]->setText("Ratio: "+QString::number(seconds[t]/seconds[min],'g',3)+"x"); } }

else
ratio[t]->setText("Ratio: "+QString::number(seconds[t]/seconds[min],'g',3)+"x");
}

[wysota]

What I meant was make methods in the main qwdiget to change the contents and lock a mutex at the beginning of it and unlock at the end. That would protect everything inside in a rather crude way, and as long as that is the only method that changes those objects that would be sufficient, right?
Unfortunately not. This protects code, not the shared resource. Qt will still access the widget from within other methods "behind your back" and you can't protect that.


But then again, this still involves function calls to the gui from another thread. Is this inherently problematic?
Yes, GUI resources just can't be protected from within application code.


I think the slots must be running in the threadwatcher thread. Here's QThread::currentThread() at various points.
The thread-watcher thread is redundant - all it does is to monitor other threads so it can be done from the main thread.

Take a look at QtConcurrent and especially QFutureWatcher, by the way.

[Galen]

Well, true, but its my redundant thread and I love it. Plus I want to use emit in this code somewhere. So now I've stripped out all the function calls. The emit signal system is really pretty much the same thing so no biggie. Everything seems to be running pretty well now, the inline function at the bottom fixes a problem whereby threads with longer running time somehow end up signalling first.

Thanks, I'll take a look at those classes, but this isn't even a real app so I probably won't be switching over even if it would run better. For this code sample I'm pretty much assuming an unlimited number of cores.



void ThreadWatcher::run() {
QSignalMapper threadMapper;
updateTimer = new QTimer();

for (int i = 0; i < 6; i++) {
connect(thread[i], SIGNAL(finished()), &threadMapper, SLOT(map()));
threadMapper.setMapping(thread[i], i); }

connect(&threadMapper, SIGNAL(mapped(int)), this, SLOT(finishedHandler(int)));
connect(updateTimer, SIGNAL(timeout()), this, SLOT(timeoutHandler()));
connect(this, SIGNAL(refresh(int)), window, SLOT(draw(int)));
connect(this, SIGNAL(time(int, QString)), window, SLOT(setTimeLabel(int, QString)));
connect(this, SIGNAL(ratio(int, QString)), window, SLOT(setRatioLabel(int, QString)));

updateTimer->start(timerVal);

QThread::exec();

delete updateTimer;
}
void ThreadWatcher::timeoutHandler() {

for(int i=0; i<6; i++) {
if (alive[i])
emit refresh(i); }

if ((threadsLeft==0) || (control[1])) {
QThread::exit(0); }

else if (control[0]) {
updateTimer->stop();
lock->lock();
condition->wait(lock);
lock->unlock();
updateTimer->start(timerVal); }
}
void ThreadWatcher::finishedHandler(int t) {
emit refresh(t);
alive[t] = false;
threadsLeft--;

seconds[t] = (float)runTime[t] / 1000.0f;
if (seconds[t] == 0.0f)
seconds[t] = 0.001f;
emit time(t, "Time: "+QString::number(seconds[t],'g',3));

if (isFirst) {
isFirst = false;
min = t;
emit ratio(t, "Ratio: 1x"); }
else {
float timeRatio = seconds[t]/seconds[min];
if (timeRatio < 1.0f)
fixRatios(t);
else
emit ratio(t, "Ratio: "+QString::number(timeRatio,'g',3)+"x"); }
}
inline void ThreadWatcher::fixRatios(int t) {
long minTime = runTime[min];
long x;

for (int i=0; i<6; i++) {
if (!alive[i]) {
x = runTime[i];
if ((x > -1) && (x < minTime)) {
minTime = x;
min = i; } } }

for (int j=0; j<6; j++) {
if (!alive[j]) {
if (j==min)
emit ratio(j, "Ratio: 1x");
else
emit ratio(j, "Ratio: "+QString::number(seconds[j]/seconds[min],'g',3)+"x"); } }
}