Thread isFinished() metod returns false
-
Hi
I have a thread which run() metod looks like:
@
..
stopRequest = false;
while(1)
{
...
if(true==stopRequest){return;}
}
@and ones metod in class
@
void closeRequest(){
stopRequest = true;
}
@Both together are connected via signal&slot:
@ this->connect(this, SIGNAL(finished()), this, SLOT(stop()));@where:
@
void stop(){
if(isFinished() == false) {//error}
else{//OK}
@//--------------
My goal:
When closeRequest() is invoked run() metod is ended so finished() signal is emitted so stop slot is executed where thread's status is checked. Supposedly isFinished() shall always returns true but sometimes/often isFinished returns false. Why?Thanks
-
Might be a race condition!
If different threads access the same variable, it must be protected with a mutex, so all access is serialized.
Try this:
@class MyThread: plublic QThread
{
/* ... /
static QMutex s_mutex;
/ ... */
};@
@stopRequest = false;
while(1)
{
s_mutex.lock();
if(true==stopRequest){return;}
s_mutex.unlock();
}@
@void closeRequest()
{
s_mutex.lock();
stopRequest = true;
s_mutex.unlock();
}@
@void stop()
{
s_mutex.lock();
bool result = stopRequest;
s_mutex.unlock();if(result == false) {//error} else{//OK}@
-
Well, there might be another reason !!!
I have noticed before that, when a QThread finishes, it first emits the finished() signal and then sets the "running" status to FALSE. So, it may happen that, while the slot, which is connected to the finished() signal, is executing, the "running" status is still on TRUE. Or it is still in the process of changing from TRUE to FALSE. Which means that calling isRunning() inside the slot may indeed return TRUE. Same issue with the "finished" status flag...
--
From the Qt code:
@void QThreadPrivate::finish(void arg, bool lockAnyway)
{
/ ... */if (terminated) emit thr->terminated(); emit thr->finished(); /* ... */ d->running = false; d->finished = true; /* ... */
}@
--
My workaround for this:
@#define THREAD_RUNNING(THRD) (((THRD)->isRunning()) ? (!((THRD)->wait(50))) : false)@ -
@
stopRequest = false;
while(1)
{
s_mutex.lock();
if(true==stopRequest){return;}
s_mutex.unlock();
}@
This one cause bug(return is in the middle lock& unlock mutex) - I assume that
@
stopRequest = true;
@
is done atomically.But you have right - I have used your THREAD_RUNNING now its works much better :D :D. But this behaviour - emits signal beforehand sets status- is pretty strange for me.
Thanks for you great help.
-
You are right about the bug. I didn't think enough about that code snipped ;-)
Anyway, you are better off with QMutexLocker:
@stopRequest = false;
while(1)
{
/* do some work here */QMutexLocker lock(&s_mutex); if(stopRequest) { return; }
}@
--
bq. But this behaviour – emits signal beforehand sets status- is pretty strange for me.
Well, technically the thread is still running at the moment when it emits the signal. It obviously cannot emit a signal, after it already has terminated. Also, if you use a direct connection, then the slot connected to finished() will even be executed in the context of the thread that is about to finish.
So I think there are good arguments for the current behavior, although it can be confusing, indeed!
--
A better name for the QThread's signals would be aboutToFinish() and aboutToTerminate(), I believe...
-
bq. A better name for the QThread’s signals would be aboutToFinish() and aboutToTerminate(), I believe…
Yes indeed.
I'm totally new in this game and I wonder why you use static mutex in class, like this:
@ class MyThread: plublic QThread
{
/* ... /
static QMutex s_mutex;
/ ... */
};
@In this case all instances of this class have the same mutex and I come to conclusion that
below quote might a reason:bq. The class is thread-safe if its member functions can be called safely from multiple threads, even if all the threads use the same instance of the class.
-
Actually I think that, as long as the variable to be protected is non-static, i.e. each instance has its own separate variable, it is fine to make the QMutex non-static too, i.e. give each instance its own separate mutex. In this specific case, making the QMutex static, doesn't hurt. By making the QMutex static, you serialize the access to the variables of all instances, allowing less parallelism. By giving each instance its own QMutex, only the access to the variable of the same instance are serialized, allowing access to different instances happen in parallel...