[Interest] Annoying Bug - Qt 4.8.1 bug?

[Konrad Rosenbaum]

Hi Etienne,

if you are impatient: scroll at least halfway down.

On Thursday, Thursday 23 January 2014 at 17:41, Etienne Sandré-Chardonnal 
wrote:
> 2014/1/23 Konrad Rosenbaum <konrad at silmor.de>
> > If some of those connections are handled outside the main thread, it
> > could explain the crash: GUI events MUST NOT be executed outside the GUI
> > thread. It also is usually a bad idea to execute methods of the same
> > object in multiple threads (unless they are specifically protected by
> > locks).
> > 
> > 
> > 
> > Why is run() empty? As far as I understand QThread this means the thread
> > lives for only a few microseconds and then self-destructs. I have not
> > completely understood what that does to the timer and progress dialog
> > yet.
> 
>  Please read previous posts! This is a dummy lightweight minimalist project
> that emphasizes on a bug. It does nothing and run() is empty because you
> will not read my whole 200k lines project and it does not change the crash
> occurence to strip it out. Yes it terminates after a few µs.

So we can exclude anything that lives inside the thread as a culprit - except 
the thread itself.

> > I think I have a possible reason why the progress dialog may crash: the
> > finished() signal is emitted from within the new thread, since the
> > connection is direct (automatic means direct in this specific case) the
> > close() method is called inside the wrong thread.
> 
> No. QThread QObject lives in the main GUI thread and emits signals from
> there. Never from the new thread. Connecting it to the main GUI thread
> slots is a very common way to use QThread.

Yes, I also use this technique. But I always explicitly use QueuedConnection - 
just to make sure. I'm paranoid this way... ;-)

> By the way, if you emit finished() from run() , that's wrong. It will call
> direct connections from the wrong thread and signal that the thread is
> finished while this is not the case. That's bad.

Neither I nor you emit finished. The implementation of QThread inside Qt emits 
finished. According to documentation it is emitted last thing from the thread 
before it exits. To make sure, I experimented with it (adding a few qDebug 
statements outputting (qint64)QThread::currentThread()) - I could confirm the 
docu. The finished() signal is DEFINITELY emitted from the new thread!

I tried to read the Unix implementation of QThread in Qt 5 (sorry, I deleted 
the Qt4 sources and I can't read Windows code very well):

QThread::start causes the underlying thread facility of the OS to create a new 
thread and run a private method in it, this private (already running in the 
new thread) method looks approximately like this (pseudo code):

//initialize
register_cleanup_to_run(mycleanup);
create_event_dispatcher();
emit started();
//do my thing
realthread->run();
//return control to OS
return 0;

The cleanup routine is then called by the OS (inside the still running thread) 
before the thread exits and does:

//write my orbituary
emit finished();
//clean up
delete_event_dispatcher();

As you can see: both the started() and finished() signals are emitted from 
inside the new thread, even though they belong to an object that officially 
"lives" inside the main thread.

This is a common pattern: you can emit signals from whereever you want, even 
if it is not the thread that the object is supposed to live in.

However, the good news is that the AutoConnections you create seem to be 
QueuedConnections after all. I haven't dug deeper into QThread to find out 
why, but it's a useful property of QThread...

> > Somewhere else in this mailthread: it does not matter whether the program
> > crashes on Linux, valgrind (helgrind, drd) detects race conditions no
> > matter whether they are triggering a problem right now by detecting
> > memory access that is not protected by locks/semaphores.
> 
> The new threads runs NOTHING (ie run() is empty) so it does not live, does
> not emit anything, and does not access anything. This can't be the problem,
> which very likely is in the main thread event loop.

As written above: it does. Even if you didn't believe you told it to.


The simple truth is (at least on my Linux laptop): for some odd reason 
QProgressDialog::setValue gets called after the progress dialog has already 
been deleted. In your case it probably decides to self-destruct because it is 
at 100% and at almost the same time gets the finished() signal causing a 
double-free. 

Normally Qt should disconnect everything and delete queued events for this 
object when a QObject destructs. I guess that in this case an event sneaks in 
because it comes from a different thread - I vaguely remember having big 
problems with this a few years ago - I solved them with a lot of semaphores 
and mutexes and occasional delays before deleting objects (not nice, but works 
most of the time).

Hint: this is a good reason to migrate to Qt5 - this problem has been solved 
there (at least as far as I can tell).

In the case of your program I could solve the problem this way:

1) I extended the run method to simulate an actual storage object: sleep(2);
  -> sleep blocks the thread, there is no risk of doing anything on the event 
    loop

2) I added a boolean variable: bool _done;
   and initialized it in the constructor: _done=false;
   _done==true will simply mean it is no longer safe to signal any dependent
   object in the main thread from the new worker thread

3) the last thing run() does is: _done=true;
   i.e.: this thread will self-destruct in a few nano-seconds

4) any method emitting signals that may cross threads (e.g.
   updateProgress(int)) now contains: if(_done)return;
   as the first line

5) To make sure nothing bad happens, you should also add
   progress->setAutoClose(false); to startWriting()
   -> otherwise the progress dialog may self-destruct prematurely

The effect is that no signals (except finished()) can cross the threads after 
the thread is done doing actual work. This way it is guaranteed that 
finished() is the last signal to cross over.

Hint: If you have too many exit points from run() - try a helper class:
template<class T> class MySetter{
        T&var; //don't worry, the compiler will optimize these
        const T val; //two variables away
public:
        MySetter(T&_var,T _val):var(_var),val(_val){} //remember
        ~MySetter(){var=val;} //do the setting at exit
};

void BackgroundFileWorker::run()
{
	//schedule _done=false for later...
        MySetter<bool> donesetter(_done,true);
	//do complex work
        sleep(2);
}




	Konrad
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/interest/attachments/20140124/9cc33494/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.qt-project.org/pipermail/interest/attachments/20140124/9cc33494/attachment.sig>