QObject::killTimers() warning after timer stopped from its own thread

[astodolski]

I have seen this topic in many places, on this and another forum, but I think not in the way that a QTimer is being used here.
I have a timer running in a thread created by moving an object to an instance of QThread (not the old method of sub-classing QThread). Everything appears to run normal except the message in the debugger window: QObject::killTimer: timers cannot be stopped from another thread.

Relevant code follows:

Worker.h


#ifndef WORKER_H
#define WORKER_H

#include <QObject>
#include <QTimer>
#include <QThread>

class Worker : public QObject
{
Q_OBJECT
public:
explicit Worker(QObject *parent = 0);
virtual ~Worker();

signals:
void finished();
void updateProgress(int);

public slots:
void start();
void stop();
void doWork();

private:
QTimer* m_workerTimer;
};

class MThread : public QThread
{
Q_OBJECT
public:
MThread(QObject *parent) : QThread(parent) {}

static void sleep(unsigned long secs)
{
QThread::sleep(secs);
}

static void msleep(unsigned long msecs)
{
QThread::msleep(msecs);
}
};

#endif // WORKER_H


Worker.cpp


#include "worker.h"
#include <QThread>
#include <QDebug>

volatile bool stopWork;

Worker::Worker(QObject *parent) : QObject(parent)
{
m_workerTimer = new QTimer(this);
stopWork = false;
}

Worker::~Worker()
{
delete m_workerTimer;
}

void Worker::start()
{
connect(m_workerTimer, SIGNAL(timeout()), this, SLOT(doWork()));
m_workerTimer->start(100);
}

void Worker::stop()
{
m_workerTimer->stop();
emit finished();
stopWork = true;
}

void Worker::doWork()
{
for (int i = 0; i < 1000000; i++)
{
if (stopWork)
{
break;
}

if (i % 10000 == 0)
{
emit updateProgress((i / 10000) + 1);
qDebug() << ((i / 10000) + 1);
static_cast<MThread*>(QThread::currentThread())->msleep(10);
}
}

emit finished();
}


FlashWizard.cpp


#include "flashwizard.h"
#include "ui_flashwizard.h"
#include "worker.h"

#include <QThread>
#include <QDebug>
#include <QMessageBox>

FlashWizard::FlashWizard(QWidget *parent) : QWizard(parent), ui(new Ui::FlashWizard)
{
ui->setupUi(this);

ui->pbReading->reset();
ui->pbWriting->reset();

ui->lblReadingStatus->setText("");
ui->lblReadingStatus->setText("");
ui->lblProgStatus->setText("");

connect(ui->introPage, SIGNAL(p1_updateLabelOne(const QString&)), this, SLOT(updateLabel(const QString&)));
connect(this, SIGNAL(currentIdChanged(int)), this, SLOT(pageCleanUp(int)));


worker = new Worker;
workerThread = new QThread;

worker->moveToThread(workerThread);

connect(workerThread, SIGNAL(started()), worker, SLOT(start()));
connect(workerThread, SIGNAL(finished()), worker, SLOT(stop()));
connect(worker, SIGNAL(finished()), workerThread, SLOT(quit()));
connect(worker, SIGNAL(finished()), ui->programPage, SLOT(setDone()));
connect(worker, SIGNAL(updateProgress(int)), this, SLOT(updateWritingProgressBar(int)));
}

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

void FlashWizard::updateWritingProgressBar(const int value)
{
if (ui->pbWriting->maximum() == value)
ui->pbWriting->hide();
ui->pbWriting->setValue(value);
}

void FlashWizard::updateLabel(const QString &value)
{
ui->lblReadingStatus->setText(value);
}

void FlashWizard::pageCleanUp(int id)
{
if (id == Page_Program)
{
ui->lblProgStatus->setText("Updating ****");
workerThread->start();
qDebug() << "Now using thread: " << worker->thread();
}

ui->pbReading->setVisible(false);
ui->lblUpdating->setText("");
}


void FlashWizard::reject()
{
if (workerThread->isRunning())
{
worker->stop();
}

}


If I click the Cancel button on the Wizard, the loop stops and the thread with it. I get the message at the iteration of where it stops as to the timer being stopped from another thread. Is the context of the main UI thread attempting to stop the timer? It doesn't appear so.

[amleto]

What does this show?



FlashWizard::FlashWizard(...)
{
...
worker = new Worker;

qDebug() << worker->thread();

workerThread = new QThread;
worker->moveToThread(workerThread);

qDebug() << worker->thread();
qDebug() << worker->m_workerTimer->thread();
}

[astodolski]

I'm not sure but a check from a contributor who posted this (http://mayaposch.wordpress.com/2011/11/01/how-to-really-truly-use-qthreads-the-full-explanation/) suggests that perhaps it's due to creating the timer on the heap in the constructor for the worker object. If so, I got to figure out how to have a task (in this case just a do nothing loop) or several tasks that can be initiated by a timer tick and can that timer live in the worker thread.

Added after 28 minutes:


what does this show?



FlashWizard::FlashWizard(...)
{
...
worker = new Worker;

qDebug() << worker->thread();

workerThread = new QThread;
worker->moveToThread(workerThread);

qDebug() << worker->thread();
qDebug() << worker->m_workerTimer->thread();

...
}


Shows a build error:

flashwizard.cpp:30: error: C2248: 'Worker::m_workerTimer' : cannot access private member declared in class 'Worker'

I changed the timer to public, modified some qDebug messages and got the following:

Main thread: QThread(0x134ae48)
Worker Object Thread Id: QThread(0x124bbb0)
Timer object lives in Thread Id: QThread(0x124bbb0)

So it appears that the timer lives in the worker thread.

[amleto]

ok, next try. Does this give same thread timer warning?



FlashWizard::FlashWizard(...)
{
...
worker = new Worker;

qDebug() << worker->thread();

workerThread = new QThread;
worker->moveToThread(workerThread);

qDebug() << worker->thread();
qDebug() << worker->m_workerTimer->thread();

//connect(workerThread, SIGNAL(started()), worker, SLOT(start()));
connect(workerThread, SIGNAL(finished()), worker, SLOT(stop()));
connect(worker, SIGNAL(finished()), workerThread, SLOT(quit()));
connect(worker, SIGNAL(finished()), ui->programPage, SLOT(setDone()));
connect(worker, SIGNAL(updateProgress(int)), this, SLOT(updateWritingProgressBar(int)));

workerThread->start();
QMetaObject::invokeMethod(worker, "start");
QMetaObject::invokeMethod(worker, "stop");
}

[astodolski]

ok, next try. Does this give same thread timer warning?



FlashWizard::FlashWizard(...)
{
...
worker = new Worker;

qDebug() << worker->thread();

workerThread = new QThread;
worker->moveToThread(workerThread);

qDebug() << worker->thread();
qDebug() << worker->m_workerTimer->thread();

//connect(workerThread, SIGNAL(started()), worker, SLOT(start()));
connect(workerThread, SIGNAL(finished()), worker, SLOT(stop()));
connect(worker, SIGNAL(finished()), workerThread, SLOT(quit()));
connect(worker, SIGNAL(finished()), ui->programPage, SLOT(setDone()));
connect(worker, SIGNAL(updateProgress(int)), this, SLOT(updateWritingProgressBar(int)));

workerThread->start();
QMetaObject::invokeMethod(worker, "start");
QMetaObject::invokeMethod(worker, "stop");
}


Yes the same message. For what it's worth, I want to note that I get the warning both calling start in the constructor or calling start (where it originally resides) in the slot that's called for the page that displays the progress bar. I just wanted to see if there was a difference - there isn't.

I took out calling new on the timer in the constructor for the worker. So I now use a reference for the timer in the connect call in the rather than a pointer in start method in the worker:



void Worker::start()
{
connect(&m_workerTimer, SIGNAL(timeout()), this, SLOT(doWork()));
m_workerTimer.start(100);
doWork();
}


Unexpectedly, I don't see the warning now until the worker completes - not when I click the cancel button, which was the previous case.



void FlashWizard::reject()
{
if (workerThread->isRunning())
{
worker->stop();
}
}

[amleto]

seems like a Qt bug to me. I think the next step is to breakpoint the warning and debug the qt source.

[wysota]

When are you calling Worker::stop()? Is the worker thread still running then? Are you always calling Worker::stop() to stop the timer or can it happen that the timer is stopped when the thread object is being destroyed?

[amleto]

I took out calling new on the timer in the constructor for the worker. So I now use a reference for the timer in the connect call in the rather than a pointer in start method in the worker:



void Worker::start()
{
connect(&m_workerTimer, SIGNAL(timeout()), this, SLOT(doWork()));
m_workerTimer.start(100);
doWork();
}


Unexpectedly, I don't see the warning now until the worker completes - not when I click the cancel button, which was the previous case.



void FlashWizard::reject()
{
if (workerThread->isRunning())
{
worker->stop();
}
}


addressing your edit:
That is not a reference. It looks like you have changed the timer to be a QTimer member variable instead of a QTimer*. If you are going to change code, please show all the changes otherwise we have to guess what you have done... Specifically, maybe you are passing a parent argument, or maybe you are not. This will affect how objects are moved between threads!



&m_workerTimer

That is not taking or using or making a reference. That is the 'address of' operator you are using there.




void FlashWizard::reject()
{
if (workerThread->isRunning())
{
worker->stop();
}
}

This is wrong and will call the timer stop method from the wrong thread (assuming the timer has been correctly moved to workerThread).

[astodolski]

Ok. I made the illustration (with code) as to the point I was making - less the semantics. The change in use of the timer changes the outcome. I did this change from referencing the article which points to proper use of threads. The other prior suggestions didn't affect the outcome of the message warning.

[wysota]

Ok. I made the illustration (with code) as to the point I was making - less the semantics. The change in use of the timer changes the outcome. I did this change from referencing the article which points to proper use of threads. The other prior suggestions didn't affect the outcome of the message warning.

Hmmm.... whaaaat?

[astodolski]

Hmmm.... whaaaat?

Please read the thread to help with confusion. I don't understand the quip. I was replying to someone else's comment.

[wysota]

I had read the thread before posting. It didn't help with my confusion :)

So is your problem fixed or not?

[amleto]

Ok. I made the illustration (with code) as to the point I was making - less the semantics. The change in use of the timer changes the outcome. I did this change from referencing the article which points to proper use of threads. The other prior suggestions didn't affect the outcome of the message warning.

Unfortunately your semantics (talking about references) are plain wrong and need to be corrected if you want to continue talking in c++ parlance.

re bolded sentence: Well it will, or it wont, or it might. It all depends on whether you correctly pass parent pointer to the qtimer and whether there is truly a Qt bug here - you still haven't clarified your change (with code or without) - less or including semantics.

[wysota]

Guys, but where do you see a Qt bug here?

[astodolski]

Guys, but where do you see a Qt bug here?

This comment looks like the right question. I don't know if it's a bug. I was working on the suggestions of things to try. I then added a change on my own. This then devolved into trivial banter of what to call a parameter passed to connect. I just want to get the issue resolved as there seems to be lots of discussion on threading in general. I posted the change in the call to connect with a code snippet. It shows a different result. Is it a bug? I don't know. Did I sin in misquoting c++ scripture? Perhaps. All who have read this knows what was intended and I'm just trying to understand. The code posted in total I hope is enough (short of a complete app) to answer the relevant question as to whether the thread was moved, and/or whether the parent pointer was passed to the timer. This is shown at top code listing. Whether wrong or a bug IS the discussion.

przejrzystość

[wysota]

In my opinion there is no bug here. If I'm right then sometimes you are not stopping the timer before the thread ends. Thus when at some point the object is destructed from within the main thread, the destructor of the timer calls stop() and since the object has a different thread affinity, you get the warning message. You either have to move the worker object back to the main thread before the worker thread ends or you have to make sure the timer is stopped before the thread ends. You'd need some "aboutToFinish" signal that you can use to stop the timer.

For instance this line of your code is surely invalid:


connect(workerThread, SIGNAL(finished()), worker, SLOT(stop()));

workerThread and worker live in different threads (wT in main thread, w in worker thread) thus the connection is going to be queued and effectively stop() will likely never be called because it'd require workerThread's event loop to be running (which it isn't because the thread has just finished() (after exiting QThread::run()).

I suggest to sit down with a piece of paper, a pencil and draw some kind of graph or list of states and transitions that need to happen during the lifetime of the thread and the worker. Pay special attention to thread affinity so that you are 100% sure when a particular slot is going to be called. Then implement your state machine. It might be easiest to just push the object back to the original thread before the worker thread dies, e.g. by doing the following:


void MyThread::run() {
exec();
if(worker)
worker->moveToThread(QCoreApplication::instance()->thread());
}

Another possibility would be to connect to the finished() signal with Qt::DirectConnection to do some cleanup before the thread dies assuming that the signal is emitted in the context of the worker thread. If not, subclass QThread and make run() emit some signal (e.g. aboutToFinish()) before it returns.

[astodolski]

In my opinion there is no bug here. If I'm right then sometimes you are not stopping the timer before the thread ends.

That was related to a previous question I WAS going to attempt to answer



Thus when at some point the object is destructed from within the main thread, the destructor of the timer calls stop() and since the object has a different thread affinity, you get the warning message. You either have to move the worker object back to the main thread before the worker thread ends or you have to make sure the timer is stopped before the thread ends. You'd need some "aboutToFinish" signal that you can use to stop the timer.


aboutToFinish would be more convenient.




It might be easiest to just push the object back to the original thread before the worker thread dies, e.g. by doing the following:


void MyThread::run() {
exec();
if(worker)
worker->moveToThread(QCoreApplication::instance()->thread());
}



This approach then suggests the use of subclassing QThread doesn't it?



Another possibility would be to connect to the finished() signal with Qt::DirectConnection to do some cleanup before the thread dies assuming that the signal is emitted in the context of the worker thread. If not, subclass QThread and make run() emit some signal (e.g. aboutToFinish()) before it returns.

Another reason to continue use of subclass of QThread.
Thank you.

[wysota]

This approach then suggests the use of subclassing QThread doesn't it?
Yes. There is nothing wrong with subclassing QThread. It gets to be wrong when you start adding slots to the subclass :)

However you don't have to subclass QThread at all. You can emit the signal from the Worker object as well when it's done its work or just immediately move it to the main thread. It breaks some OOP concepts but it's still a valid approach.

[amleto]

ok, next try. Does this give same thread timer warning?



FlashWizard::FlashWizard(...)
{
...
worker = new Worker;

qDebug() << worker->thread();

workerThread = new QThread;
worker->moveToThread(workerThread);

qDebug() << worker->thread();
qDebug() << worker->m_workerTimer->thread();

//connect(workerThread, SIGNAL(started()), worker, SLOT(start()));
connect(workerThread, SIGNAL(finished()), worker, SLOT(stop()));
connect(worker, SIGNAL(finished()), workerThread, SLOT(quit()));
connect(worker, SIGNAL(finished()), ui->programPage, SLOT(setDone()));
connect(worker, SIGNAL(updateProgress(int)), this, SLOT(updateWritingProgressBar(int)));

workerThread->start();
QMetaObject::invokeMethod(worker, "start");
QMetaObject::invokeMethod(worker, "stop");
}



Yes the same message. <SNIP>


Guys, but where do you see a Qt bug here?

That is a bug if that truly happened. But it seems the OP is not being honest/accurate.

[wysota]

This works fine for me on Qt 4.8 and 5.0 RC2. I'm not getting any warnings.


include <QtCore>

class Worker : public QObject {
Q_OBJECT
public:
Worker() : QObject(){
timer = new QTimer(this);
timer->setInterval(1000);
}
public slots:
void start() { qDebug() << Q_FUNC_INFO; timer->start(); }
void stop() { qDebug() << Q_FUNC_INFO; timer->stop(); }
private:
QTimer *timer;
};

#include "main.moc"

int main(int argc, char **argv) {
QCoreApplication app(argc, argv);
QThread thread;
Worker *worker = new Worker;
worker->moveToThread(&thread);
thread.start();
QMetaObject::invokeMethod(worker, "start");
QMetaObject::invokeMethod(worker, "stop");
QObject::connect(&app, SIGNAL(aboutToQuit()), &thread, SLOT(quit()));
QTimer t;
t.start(1000);
QObject::connect(&t, SIGNAL(timeout()), &app, SLOT(quit()));
app.exec();
thread.wait();
}

You can build on this example to check your actual code (with timers and stuff).

Edit: I even added the timer stuff to the code, still works as expected.

[astodolski]

I've compiled a complete application with the changes that have been posted in this thread. It's possible as you have suggested that the app is attempting to stop the timer before the thread completes.

test app attached here ->> 8510

[wysota]

Does the code you posted still contain the bug? I'm not getting any warnings.

[astodolski]

Does the code you posted still contain the bug? I'm not getting any warnings.

The code example I posted still reports the warning message. Are you able to compile and run it? I am using 4.8.4 with Win SDK 7.1 compiler under Creator 2.6

[wysota]

I'm not getting any warnings. I use Qt 4.8.3 x64 Linux. Same output with Qt 5.0 RC2.


$ ./FlashWizard
Main thread: QThread(0xcde710)
Worker Object Thread Id: QThread(0x7ffff75eff00)
Now using thread: QThread(0x7ffff75eff00)
1
2
3
...
97
98
99
100

[astodolski]

I'm not getting any warnings. I use Qt 4.8.3 x64 Linux. Same output with Qt 5.0 RC2.

Oh I guess the problem wasn't fully realized. It's only when you click the cancel button before the loop finishes that the message is shown - sorry. I should have re-iterated that. I though that was evident within the thread.

[wysota]

Still, no warning :)

An idea: maybe you shouldn't be using a thread at all? What do you need it for?

[astodolski]

Still, no warning :)

An idea: maybe you shouldn't be using a thread at all? What do you need it for?

So let me understand. You run this app. Before the count gets to 100, hit the brakes with the Cancel button. You're telling me no message like "QObject::killTimer: timers cannot be stopped from another thread"?

This is a stripped out app that is used as a prototype for use with a hardware DLL. That library has functions that are called for each timer tick and have to perform tasks in the background that would otherwise tie up the UI. I created this to perform a task (a loop counter in this case) and update a progress bar. I find it incredible that this test app loaded on a host PC and on a virtual machine and on my own laptop all perform exactly the same - that is, the killTimer warning is displayed in the debug window after the Cancel button is clicked and before the counter gets to 100, except for you.

I read your article on keeping the UI responsive as well as general considerations when making calls to hardware while updating a UI with background workers. I feel that background tasks belong in another thread so as to keep the UI responsive. That's my reason for this prototype. That's pretty much boilerplate practice. I don't have a Linux distro prepared for testing on there.

:confused:

[wysota]

So let me understand. You run this app. Before the count gets to 100, hit the brakes with the Cancel button. You're telling me no message like "QObject::killTimer: timers cannot be stopped from another thread"?
Nope. The last thing I get is a number. Maybe I should wait an hour for the message to appear ;)


That library has functions that are called for each timer tick and have to perform tasks in the background that would otherwise tie up the UI.
It doesn't yet mean you need to do any threading.


I created this to perform a task (a loop counter in this case) and update a progress bar. I find it incredible that this test app loaded on a host PC and on a virtual machine and on my own laptop all perform exactly the same - that is, the killTimer warning is displayed in the debug window after the Cancel button is clicked and before the counter gets to 100, except for you.
Do you want me to shoot a video? :) Maybe the thing is that you're using Windows and I'm not (however I doubt it).


I feel that background tasks belong in another thread so as to keep the UI responsive.
You can always delegate the task to an external process and talk to it using QProcess or QLocalSocket. However I still think threading is not required here, especially if it's a source of some problems. Nevertheless I think the code you have is easy to fix to work within a thread. Maybe you just need to restructure the code.

[astodolski]

You can always delegate the task to an external process and talk to it using QProcess or QLocalSocket.

My DLL is only functions in a library. It is not an external program which is what QProcess takes care of. QLocalSocket would be helpful but not really what I need.

Your article even supports threads for external libraries http://doc.qt.digia.com/qq/qq27-responsive-guis.html#usingaworkerthread



However I still think threading is not required here, especially if it's a source of some problems.

But not necessarily a Qt bug.



Nevertheless I think the code you have is easy to fix to work within a thread. Maybe you just need to restructure the code.
I still think this is viable if I can get the thread stopped properly. That is what was previously suspected.

[wysota]

I still think this is viable if I can get the thread stopped properly. That is what was previously suspected.

A possible fix is very easy. Use QObject::startTimer() and QObject::killTimer() instead of an external QTimer instance.


void MyWorker::start() { m_tid = startTimer(100); } // slot
void MyWorker::stop() { killTimer(m_tid); } // slot
void MyWorker::timerEvent(QTimerEvent *ev) {
if(ev->timerId() == m_tid) doWork();
else QObject::timerEvent(ev);
}

[astodolski]

A possible fix is very easy. Use QObject::startTimer() and QObject::killTimer() instead of an external QTimer instance.


void MyWorker::start() { m_tid = startTimer(100); } // slot
void MyWorker::stop() { killTimer(m_tid); } // slot
void MyWorker::timerEvent(QTimerEvent *ev) {
if(ev->timerId() == m_tid) doWork();
else QObject::timerEvent(ev);
}

Thanks anyway. This didn't change anything. I changed the slots in the worker as shown. No improvement. Perhaps another viewer running Windows can try the sample I posted with the changes mentioned since it seems to be Windows only related.

[wysota]

You shouldn't be getting that error with the code I posted. Unless maybe you delete the worker object from within a wrong thread (if it gets deleted at all). Try cleaning and rebuilding the project from scratch.

[astodolski]

You shouldn't be getting that error with the code I posted. Unless maybe you delete the worker object from within a wrong thread (if it gets deleted at all). Try cleaning and rebuilding the project from scratch.

Done that. Still as is. Should I post the updated project?

BTW for reference, it seems as others have experienced the same on Windows though I dont know any further detail.
http://qt-project.org/forums/viewthread/22916

I found something that may be helpful.

With all the current changes, I note that the stop slot gets called twice. The first time through the current thread is the main thread - hence the message. The second time through the current thread is the worker thread.

In line 957 of qeventdispatcher_win.cpp

thread() != QThread::currentThread;

We get here from killTimer which calls unregisterTimer.

Note that on a Linux system, you may never get to that line of code because it is in a different module altogether.