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

[Thiago Macieira]

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.

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? For example, imagine that the slot connected to finished() 
does start() again.

- 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()

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.

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.
	
-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center
     Intel Sweden AB - Registration Number: 556189-6027
     Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.qt-project.org/pipermail/development/attachments/20111103/1e673ff4/attachment.sig>