[Development] QThread::finished() race → fixing QThread

[Olivier Goffart]

On Thursday 03 November 2011 11:37:25 Thiago Macieira wrote:
> Hello
> 
> First of all, required reading:
> http://labs.qt.nokia.com/2010/06/17/youre-doing-it-wrong/
> 
> == Background ==
> 
> Today, I was told that there was a timing sensitive test in
> QDBusPendingCall's unit testing. Looking at the code, it was something like
> this:
> 
> 	connect(&thread, SIGNAL(finished()), &loop, SLOT(quit()));
> 	thread.start();
> 	loop.exec();
> 	QVERIFY(!thread.isRunning());
> 
> Technically speaking, there's a race condition in *starting* the event loop
> above, but that's an error of the code above. Let's ignore it for the
> moment.
> 
> The problem we saw was QVERIFY would fail. If we look at the apparent causal
> sequence of events, we have:
> 
> 	event loop exited
> because
> 	finished() signal was emitted
> because
> 	thread's run() function exited
> 
> But isRunning() is still true. The reason for this is that there's a race
> condition in QThreadPrivate::finish, which emits the finished() signal
> before it sets the isRunning variable to false. The fix looks simple: move
> the code around.
> 
> Is it really?
> 
> == Larger implications ==
> 
> The crux of the issue is that finished() is emitted by
> QThreadPrivate::finish(), which is run from *inside* the thread that is
> finishing. Depending on how you interpret finished(), that is a
> contradiction. 

> If you interpret it as "the thread has finished", the fact
> that it's emitted from inside the thread means the thread was still running
> when it was emitted, which means it wasn't finished. If you interpret it as
> "the user code in the run() function has finished", then it's fine.

The documentation is not explicit, but the current is the second, which is 
fine.

In general, I think this is a documentation issue. You think finish is 
something that it is not.

I however agree that the API is not intuitive, but still correct.

> 
> So on IRC we discussed this and came to no conclusion yet. More issues came
> to light in the discussion:
> 
> - if finished() is emitted from inside the thread, are there other race
> conditions possible? 

Maybe, but there should not be in the Qt code (public API)
Race conditions are always possible if you shoot yourself in the foot.

> For example, imagine that the slot connected to
> finished() does start() again.

No race here. (this is even tested in tst_QThread::startFinishRace)

> - if we move the finished() from the thread to the QThread's owning thread,
> we ensure that the thread *has* finished, but we're likely to break
> existing code. After Brad's blog came out, people began writing code like:
> 
>      MyObject *obj = new MyObject(...);
>      QThread *thread = new QThread(...);
>      connect(...);
>      obj->moveToThread(thread);
>      thread->start();
> (in http://lists.qt.nokia.com/pipermail/qt-interest/2009-August/011022.html
> )
> 
> where is the object deleted? Some might write:
> 	connect(thread, SIGNAL(finished()), obj, SLOT(deleteLater()));
> 
> This code and other cleanup tasks that people may have will no longer work
> if we move the finished() to really mean the thread has finished: the slot
> invocations would be queued and never run.
> 
> Also, finished() would not be emitted at all if people had written
> 
> 	thread->moveToThread(thread);
> 
> but on IRC we all agreed we don't care about this case.
> 
> == What to do? ==
> 
> We have a couple of options before us:
> 
> 1) leave QThread unchanged, update the docs to say that finished() means the
> user code has finished, but the thread itself might still be running and
> performing clean up tasks. If synchronisation is required, one should call
> QThread::wait()

This is the easier.

> 2) move the code around and update the documentation as above
> 
> 3) make it so that finished() is emitted only after the thread has finished;
> maybe add a QThread::aboutToFinish() signal that is emitted from inside the
> thread like the current finished() is.

Both of them are behaviour change.
(For the record, i am not opposed to such a change)

> 
> In the cases of 1 and 2 above, we can also deprecate QThread and introduce a
> new class with the proper semantics.
> 
> In general, we believe we need to do some work for QThread anyway. Ever
> since we told people they were doing it wrong, we have failed to update the
> documentation to provide the "right way" to do and we still have no
> convenience for the common, right way we propose people use.

I beleive more work should be spend in QtConcurrent rather than in QThread.
I think QThread is fine as a low level class.

If the only problem is "should isFinished() return true or false when 
QThread::finished  is emit", this is just bikeshedding.

QtConcurrent however has some serious performence problems (unacceptable 
overhead sometimes) and huge lack in its API (no way to choose the threadpool 
is one)