Cleaning up after a thread correcly



  • Hello guys, :)

    I'm tired of trying to learn how to start and delete a thread correctly without residuals or problems. I posted other problems with this program before, and to make the thing easier to solve, please download a copy of the source from the following link.

    The program is a down-sampler. It simply reads a few blocks of a file, calculates the average, and writes the result. The purpose is simply making files smaller.

    http://www.4shared.com/file/Lh2KqF0Q/DownSampler.html

    The problem is, that whenever the start button and stop are clicked a few times, the program freezes. This happens now in Linux, but I don't know if this happens in windows too. I think it does.

    The program is compilable under windows and linux.

    Please guide me to the method to fix this problem. I tried calling QThread::quit(), exit(), terminate() with a deleteLater()... but all cause problems... with no exception! please tell me how to do this correctly.

    Thank you :-)



  • first of all, in my opinion, using of "quit()":http://doc.qt.nokia.com/latest/qthread.html#quit, "exit()":http://doc.qt.nokia.com/latest/qthread.html#exit, "terminate()":http://doc.qt.nokia.com/latest/qthread.html#terminate for threads is a bad style.
    You should design interaction with/within thread to avoid their use by any cost. Terminate() allowed to be only in destructors or functions with the same aim as destructor, where the immediate termination of thread is really necessary and reasonable.
    The preferable way - to foresee all quit point for thread and put there only return; statement.

    The second part is a code snippet overview
    @downsamplingthread.cpp

    this->terminate();
    this->exit();
    this->quit();
    return;@
    why is there so much thread-stopping functions? It's quite enough to use one of them. I advice to read manual about each of them.

    @if(outputTypeString == "int16")
    {
    ...
    }
    else if(outputTypeString == "int32")
    {
    ...
    }
    else if(outputTypeString == "int64")
    {
    ...
    }@
    use switch ()

    [quote author="TheDestroyer" date="1307294058"]
    The problem is, that whenever the start button and stop are clicked a few times, the program freezes. [/quote]
    you are using new to create "new":http://www.cplusplus.com/reference/std/new/ thread and forgot about "delete":http://www.cplusplus.com/reference/std/new/operator delete/ -> they block each other.



  • Thank you for your reply.

    You're right about the switch thingy. I'll use it! and about exit and quit, what should I use instead????

    but concerning the new and delete commands... whenever I try to delete the thread, the program freezes! have you succeeded with that yourself? because I tried that like 10000 times with different methods with no success, and that's why I'm asking about a plan to clean up the thread professionally.

    So how can I use delete without quit()? please try it and you'll see how the program freezes!!!!

    If you know the way to go around this and use the thread cleanly, please tell me!



  • [quote author="romankr" date="1307306289"]@if(outputTypeString == "int16")
    {
    ...
    }
    else if(outputTypeString == "int32")
    {
    ...
    }
    else if(outputTypeString == "int64")
    {
    ...
    }@
    use switch ()[/quote]
    The switch statement only works on integer constants, not on (c-)strings.

    [quote]If you know the way to go around this and use the thread cleanly, please tell me![/quote]
    Make sure you react properly to exiting. Use exit() only if you properly handle that. Don't use terminate() unless you have a good reason to terminate it.

    I would always go with a
    @connect(myThread, SIGNAL(finished()), myThread, SLOT(deleteLater()));
    myThread->stopWorking();@

    This will delete your thread after it has finished its work. Only thing to do now is to make sure your main thread stays alive long enough to delete the thread object.



  • Thank you for the answers guys. Sorry for the late reply, but my laptop got a problem and I had to format it!

    What do you mean with properly handle exiting? In the program, when the user clicks start, the thread is created, and the error could be issued in the thread immediately if the files don't exist, or if the user clicks the Stop button! So how could I handle exit() correctly here? is it just by issuing exit() on error? cuz this is what I'm doing. If the user clicks start again, a new thread is created and the old one is "lost" in memory... The problem is, as far as I see it, is that deleting threads take relatively long time. Isn't that true?

    so I can't tell the user "please wait till the thread is deleted".

    I'm thinking that since deleting threads takes long time, would it be a feasible solution to use a thread pool, in a way that whenever a thread fails, it's ignored by the program and handled by the pool for deletion, so that I could take another new thread from the pool when the user clicks Start again?



  • You could try and see if QtConcurrent helps you out.



  • [quote author="TheDestroyer" date="1307430591"]
    If the user clicks start again, a new thread is created and the old one is "lost" in memory... The problem is, as far as I see it, is that deleting threads take relatively long time. Isn't that true?
    [/quote]

    please define relatively long? The user recognizes it? Then it's a bug in your code. You could have something like:

    @
    MyThread::run()
    {
    // do something
    if(m_bStopThread)
    return;
    // do something
    if(m_bStopThread)
    return;
    // do something
    if(m_bStopThread)
    return;
    // do something
    if(m_bStopThread)
    return;
    }

    MyThreda::StopThread()
    {
    m_bStopThread = true;
    }
    @



  • Thank you for your answers, guys!

    @Franzk: QtConcurrent is very limited. It doesn't help my general purposes. I'm more interested in learning QThread.

    @Gerolf: So interestingly, Gerolf, I have the same idea in the code of my first post. Please check the program of my first post. It's a very small program. You can see that I try to use this "stop" idea exactly the same way you mentioned it, but I can't find the place where I have to place the delete thread statement. Whenever I introduce a thread deletion, the program freezes!!!! please take a look at the program code in my first post!



  • You can use it like this:

    @
    MyMainWnd::beginWork()
    {
    pThread = new MyThread();
    pThread->start();
    }

    MyMainWnd::stop()
    {
    pThread->StopThread()
    pThread->wait();
    delete pThread;
    pThread = 0;
    }
    @

    To use this, you have to ensure, that the stopped checks, will not have a too long time in between...



  • Thank you for the reply!

    I'll try this and tell you how it goes! I tried the command QThread::wait() before, but caused freezing as well. So I'll try it again and come with detailed description of what I get :)



  • It will cause freezing, if the thread doesn't respond quickly to being told to stop.



  • Use the deleteLater() approach. That won't cause you to wait for the thread to be finished. On the other hand, maybe you can come up with a way that will allow you to keep the thread around and not recreate it every time.



  • I would not call deleteLater on a QThread that is still running. Sounds like a recipe for all kinds of funny things to happen when you don't expect it.

    Edit:
    What might be a good approach, is something like this:
    @
    connect(myThread, SIGNAL(finished()), myThread, SLOT(deleteLater()));
    myThread->stopThisThread();
    myThread = 0;
    @



  • [quote author="Andre" date="1307457203"]I would not call deleteLater on a QThread that is still running. Sounds like a recipe for all kinds of funny things to happen when you don't expect it.[/quote]

    Neither would I. I was referring to what I've stated earlier:
    @connect(myThread, SIGNAL(finished()), myThread, SLOT(deleteLater()));@

    Edit: My point exactly :P



  • Ah, sorry, missed your previous suggestion for the same idea. I would put the connect call before the stop call though. Otherwise, you might get into a race condition where the thread is already stopped before the connection has been made. Then again, you should probably make the connection at creation time for the thread anyway, not when you want to stop it.



  • [quote author="Andre" date="1307457203"]I would not call deleteLater on a QThread that is still running. Sounds like a recipe for all kinds of funny things to happen when you don't expect it.

    Edit:
    What might be a good approach, is something like this:
    @
    connect(myThread, SIGNAL(finished()), myThread, SLOT(deleteLater()));
    myThread->stopThisThread();
    myThread = 0;
    @

    [/quote]

    This can also lead to unexpected behavior. You call delete in the main thread when the event loop comes to this point. While it reaches this, the thread could be suspended (depends on timing) or be finished, even if the emit is the last command in the run method.

    EDIT:

    So you would need a wait in the destructor of the thread….



  • Gerolf, you mean that QThread::finished() may be emitted before the thread is actually finished? That would be a bug, would it not?



  • From reading the QThread sources it seems on symbian, the finished() signal is emitted from the threaded function (after run() returns). On unix, the signal is emitted in the pthread cleanup handler. Didn't check the windows code. It seems Gerolf could have a point here. However, if that is the case, the only way to check if the thread is actually still running is by using some pthread_kill() magic.



  • [quote author="Andre" date="1307457823"]Gerolf, you mean that QThread::finished() may be emitted before the thread is actually finished? That would be a bug, would it not?[/quote]

    The point is: when is a thread finished?

    To emit a signal, you need code. This code is somewhere :-) If it is inside the threads function (even it is outside run()) you might get in timing problems.



  • I get that, and judging by Franzks analysis, you have a point at least in the Symbian case. It seems logical to me that Qts implementation would make sure the thread really has terminated before the signal is emitted. I understand that you need code to do the emit, but that code does not belong in the thread who's termination it is supposed to signal. That would mean that the Symbian implementation is wrong, but I have no idea if it is possible to do it the right way on that platform.



  • I even don't know if that is possible on windows.
    But the Qt code does not guarantee that.
    This is an excerpt of QThread for windows:

    @
    unsigned int __stdcall QThreadPrivate::start(void *arg)
    {
    // some initialization

    emit thr->started();
    
    thr->run();
    
    finish(arg);
    return 0;
    

    }

    void QThreadPrivate::finish(void *arg, bool lockAnyway)
    {
    // some other stuff
    emit thr->finished();
    // some other stuff
    }

    void QThread::start(Priority priority)
    {
    // some other stuff
    d->handle = (Qt::HANDLE) _beginthreadex(NULL, d->stackSize, QThreadPrivate::start,
    this, CREATE_SUSPENDED, &(d->id));

    // some other stuff
    

    }
    @

    This means, the thread function QThreadPrivate::start defines the thread. When it is finished, the thread dies. Within this method, the finished signal is emitted and it is not at the very end of the thread, but guaranteed after the run method....

    I'm not sure, whether there is a thread stopped signal on windows...



  • Apparently the windows thread performs exactly the same action as the symbian implementation. The linux implementation does pretty much the same when no pthread_cancel() is used. The cleanup function is called by a pthread_cleanup_pop(1); This is likely to happen within the thread though. Anyway, this being known, the solution would rather go towards

    @connect(t, SIGNAL(finished()), this, SLOT(cleanupThread()));
    ...
    void This::cleanupThread()
    {
    thread->wait();
    thread->deleteLater();
    }@
    provided of course that wait() is guaranteed to wait until the OS thread is actually finished. Details are different, but the general idea is still the same.



  • This should work.
    wait uses the variables that are set by the finished method before the signal finished is emitted, but there is a mutex around both functions. So wait should return when the thread is really finished.



  • I still think it is a bug in Qt then. Qt could use the same trick itself to make sure finished() is emitted after the thread has really terminated, could it not?



  • I can't. Otherwise it would need another thread to wait for this one :-)

    It can't use the main thread, or has to send an internal event to the QThread object which then would have to block the QThreads creator thread untill the real thread is destroyed and then emit the signal.... not very nice....

    but just calling wait in the slot for the finished signal would do the trick.



  • The wait time would be very short, if there at all. After all, the thread is supposed to be done, right? Now, you have to wait in the main thread anyway. Why not give that task to QThread instead? The real thread could (in absense of a signal from the OS), as it's last act, indeed just send an event to the QThread and then terminate. The QThread object could in its handler for that event wait for the thread to be finished, and only then emit the signal. I like it more than having to code constructions like these:

    @
    connect(t, SIGNAL(finished()), this, SLOT(cleanupThread()));
    ...
    void This::cleanupThread()
    {
    thread->wait();
    thread->deleteLater();
    }
    @

    You have gained nothing (still waiting for the thread) and you have to write additional code.



  • [quote author="Andre" date="1307523633"]You have gained nothing (still waiting for the thread) and you have to write additional code. [/quote]
    That is not entirely true. What you have gained is a non-blocking solution to the thread cleanup, because the thread is cleaned up at a point where you know for sure that QThread::wait() isn't going to last for more than a few milliseconds.



  • Considder this class then:

    @
    FixedQThread: public QThread
    {
    Q_OBJECT

    public:

    FixedQThread(QObject* parent = 0)
    : QThread(parent) {
        connect(this, SIGNAL(finished()), SLOT(waitForReallyFinished()));
    }
    

    signals:
    void reallyFinished();

    private slots:
    void waitForReallyFinished()
    {
    wait();
    emit reallyFinished();
    }
    };
    @

    After which you can do:
    @
    //thread of type FixedQThread now:
    connect(thread, SIGNAL(reallyFinished()), thread, SLOT(deleteLater()));
    @

    Right?



  • Aye, that ought to work. It's basically the approach mentioned earlier, just wrapped in a class, which is a good thing to do anyway.



  • So... Why could that same approach not be inside QThread instead?



  • That would be too obvious, wouldn't it?



  • Which I why I stated that I think that QThread should be fixed to always emit the finished signal after the thread has actually finished. It is not like it would be very hard to do, as demonstrated above...

    Edit:
    Actually, I guess even wait is not going to save the day. See "this bug report":http://bugreports.qt.nokia.com/browse/QTBUG-684 . I have filed a "bugreport":http://bugreports.qt.nokia.com/browse/QTBUG-19783 on the issue.



  • Thanks for the nice discussion guys. I think now I could implement the approach as best as possible. I'm gonna come back to tell you how it went :-)



  • I think it should work, as QThread::wait has a mutex inside, same as finished. In finished it depends on lockAnyway, whether it is locked or not. During normal thread exit, lockAnyway is true...

    @
    bool QThread::wait(unsigned long time)
    {
    Q_D(QThread);
    QMutexLocker locker(&d->mutex);

    // do the rest
    

    }

    void QThreadPrivate::finish(void *arg, bool lockAnyway)
    {
    QThread *thr = reinterpret_cast<QThread *>(arg);
    QThreadPrivate *d = thr->d_func();

    if (lockAnyway)
        d->mutex.lock();
    
    // do the rest
    
    if (lockAnyway)
        d->mutex.unlock();
    

    }

    @



  • Andre's last approach with "reallyFinished()" signal and slot worked perfectly! I like that approach because it's very clear and handles the problem professionally.

    Thanks to all of you guys! I suppose problem solved :D

    This was tested on Windows. I hope linux won't make nasty problems :D



  • It's always fascinating to see how complicated threads are. ;-)

    But one more question:
    Can or will the method FixedQThread::waitForReallyFinished() from Andre block the main thread? Because the instance of FixedQThread will live in the context of the main thread normally, just the run() method is executed in another context.



  • Actually I tried it in my down-sampler program, and the progress bar in the main window was active with no problems.

    The process of being finished or not will only be emitted after run() is done. The problem was, as far as I understood it, the accurate timing in firing the right finished signal, which happens normally (when not using this fix) after run() and before the end of call start().

    So the objective in all this, is to wait for start() to return.



  • [quote author="DeVeL-2010" date="1307617297"]
    Can or will the method FixedQThread::waitForReallyFinished() from Andre block the main thread?[/quote]
    Absolutely. The idea here however is to try and make the wait in the main thread be as short as possible.



  • Indeed, but it should only block for a very small time: the time it takes to switch to the almost finished thread to let it terminate, via perhaps some other running threads, and back to the thread where the FixedQThread object lives. Most of the time, it should not block at all, because the thread would already have terminated properly. That is, if all goes well and it works as I expect it to. Note that I wrote that example only to illustrate a point on that this should be fixed in QThread. I don't give any guarantees that it will work correctly 100% of the time in the real world :-)



  • Notice that from Qt 4.8 it is no longer required to wait() in the destructor of the thread if you want to connect the finished() signal to the deleteLater() slot

    (this was fixed by commit 25c9b6ed)


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.