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

[Giuseppe D'Angelo]

2011/11/3 Thiago Macieira <thiago.macieira at intel.com>:
> == 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.

It's the current behaviour, so it just need to be clarified in the docs.

> 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.

Or simply deletes the QThread object ("while it's still running") --
even if the code has protections against all of these cases (f.i.
finish() and the QThread dtor lock the same mutex), it's still wrong
from a certain point of view, isn't it?

> - 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.

Like posting an event to the QThread object, the handling of which is
basically "wait(); emit finished()"?

> 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.

IMHO this is part of another set of "untold truths" -- why does it
that work in the first place? This "last round of cleanup" is not
documented anywhere, is it?

To put it in another way, given the fact that obj is living in the
thread managed by QThread, one could reasonably expect that code *not*
to work in any case -- there's no event loop picking up the
DeferredDelete event (in case of finished() emitted by the managed
thread, thus a direct connection used) or the metacall event (in case
of finished() emitted by the QThread thread).

> 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.

Ok...

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

I second that because it's simply documenting the current behaviour
(=> doesn't change it).

> 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.

This is being done:
http://doc.qt.nokia.com/4.8/thread-basics.html
(although that particular page has several issues at the moment).
There's also additional stuff on the wiki
http://developer.qt.nokia.com/wiki/Threads_Events_QObjects
and blog posts here and there
(cf. the qtassistant factoids http://tinyurl.com/qt-factoids )

Apart from that, would you oppose a QtConcurrent::run-like API inside
QThread like
static QThread * runInAnotherThread(QObject *objectToMove, const char
*methodToInvokeOnStart, const char *signalToTellTheThreadToShutDown,
bool autoDelete = false)
?

Or, if there are larger plans to have QtConcurrent support QThread
features like event loops, are they being discussed somewhere?

-- 
Giuseppe D'Angelo