Randomly getting "QThread: Destroyed while thread is still running"

Alvein

Hello!

Short code:

void MainWindow::some_slot() {
    int iK;
    // ...
    do
        for(iK=0;iK<TOTAL_WORKERS;iK++)
            if(!thlWorkers.at(iK)->isRunning())
                break;
    while(iK==TOTAL_WORKERS);
    delete thlWorkers.at(iK);
    thlWorkers[iK]=QThread::create([]{/*...*/});
    thlWorkers.at(iK)->start();
}

From time to time, I am unable to destroy a thread because of that given error.

It should be safe since I'm explicitly picking one which is not running.

Isn't this condition enough to safely delete?

stretchthebits

What are the threads doing? What does the code for the thread look like?

ChrisW67

All the examples I see use the finished() signal and deleteLater(), but then they are not polling in an event driven system.

Kent-Dorfman

Using IsRunning() is dangerous as there is no guarantee that the thread remains running even a few milliseconds after the call. Use catch the finished() signal and use deletelater as others have mentioned. Also even start() doesn't guarantee immediate execution. It just adds the thread to the scheduler.

jsulm

@Alvein You should stop your threads properly. Use https://doc.qt.io/qt-5/qthread.html#requestInterruption and https://doc.qt.io/qt-5/qthread.html#isInterruptionRequested to ask each thread to stop and then call https://doc.qt.io/qt-5/qthread.html#wait on each thread.

Alvein

@stretchthebits said

What are the threads doing? What does the code for the thread look like?

Originally, calling some REST service, but I've just replaced that with some random QThread::msleep() and the problem persisted.

@ChrisW67 said

All the examples I see use the finished() signal and deleteLater(), but then they are not polling in an event driven system.

Thanks for the suggestions. I've modified the code (and now I have another problem). See below.

@Kent-Dorfman said

Using IsRunning() is dangerous as there is no guarantee that the thread remains running even a few milliseconds after the call. Use catch the finished() signal and use deletelater as others have mentioned. Also even start() doesn't guarantee immediate execution. It just adds the thread to the scheduler.

Thanks for the suggestions. TBH, I suspected that but just gave it a go.

One thing that bothers me a lot is the fact that isRunning() and isFinished() are not something to trust in. Then what do they exist for in first place?

@jsulm said

You should stop your threads properly. Use https://doc.qt.io/qt-5/qthread.html#requestInterruption and https://doc.qt.io/qt-5/qthread.html#isInterruptionRequested to ask each thread to stop and then call https://doc.qt.io/qt-5/qthread.html#wait on each thread.

I'll leave this for later, since the threads I'm using don't need to be forcefully stopped. They just finish themselves.

....

I've modified the code to something simple. Now, I am not continuously creating and destroying threads. I create everything in a single shot, and later, start the available ones as soon as others finish.

This should be simpler, but I am having a new (random) error:

"...(first chance) in MSVCP140D!std::_Throw_future_error"
"...abort() has been called"

This is the new code:

#define TOTAL_WORKERS 10
#define TOTAL_ACTIVE   4

void MainWindow::some_slot() {
    int iK;
    // ...
    for(iK=0;iK<TOTAL_WORKERS;iK++) {
        thlWorkers[iK]=QThread::create([]{/*...*/});
        connect(thlWorkers[iK],SIGNAL(finished()),this,SLOT(on_finished()));
    }
    for(iK=0;iK<TOTAL_ACTIVE;iK++)
        thlWorkers.at(iK)->start();
    // ...
}

void MainWindow::on_finished() {
    int iK=getOneFreeWorkerIndex();
    thlWorkers.at(iK)->wait();
    thlWorkers.at(iK)->start();
}

For simplicity, let's assume that I don't need to stop the process once it starts.

Also, please consider getOneFreeWorkerIndex() as just a random pick. In practice, it should choose one worker based on its stats.

And the following wait() should avoid starting an already running thread, right?

OK, what's the problem here?

Thanks to everyone for investing time in this.

SGaist

Hi,

Might be a silly question but aren't you re-implementing QThreadPool ?

Kent-Dorfman

@Alvein said in Randomly getting "QThread: Destroyed while thread is still running":

One thing that bothers me a lot is the fact that isRunning() and isFinished() are not something to trust in. Then what do they exist for in first place?

those methods are only safe to syncronize operations "within the thread"...your attempted use is to use them to "control/manage" a threadpool. There is an important distinction because to effectively control or manage an item you need to have authoritative state information, but to syncronize an operation that on a thread the burden of safety is arguably much lower, especially if that synconization operation can be "error checked" as pass/fail.

ChrisW67

@SGaist said in Randomly getting "QThread: Destroyed while thread is still running":

Might be a silly question but aren't you re-implementing QThreadPool ?

Maybe there's some unspoken requirement, but it seems likely to me that user controlled threads are not required to implement this at all. The threads are just 'calling some REST service' so QNetworkAccessManager can probably deal with it.

Alvein

As I mentioned, this program is failing even if I use a simple QThread::msleep() per thread.

I just want to have a given number of threads (TOTAL_WORKERS) but only a smaller amount (TOTAL_ACTIVE) running at time.

Once one thread finishes, another one is started. That's all.

That looks very simple, right?

What's wrong with my implementation?

jeremy_k

@Alvein said in Randomly getting "QThread: Destroyed while thread is still running":

I just want to have a given number of threads (TOTAL_WORKERS) but only a smaller amount (TOTAL_ACTIVE) running at time.

What is having threads that are created but not immediately started intended to accomplish?

Alvein

@jeremy_k See them more as pending tasks. It's easier for me to set them all in advance, and the lambdas actually reference methods of other objects.

I modified the original approach to follow some of the suggestions, but that's not important now.

ANYWAY

Is there a problem in starting again a thread which has already finished? I suspect this is the source of the problem.

jeremy_k

@Alvein said in Randomly getting "QThread: Destroyed while thread is still running":

@jeremy_k See them more as pending tasks. It's easier for me to set them all in advance, and the lambdas actually reference methods of other objects.

That sounds inefficient. Restructure the tasks as jobs in a queue, with a set of threads that take from the queue. Or better yet, use QThreadPool or Qt Concurrent to process the tasks, and get out of low level thread management altogether.

I modified the original approach to follow some of the suggestions, but that's not important now.

ANYWAY

Is there a problem in starting again a thread which has already finished? I suspect this is the source of the problem.

From QThread::create():

Warning: do not call start() on the returned QThread instance more than once; doing so will result in undefined behavior.

Alvein

@jeremy_k So the thing has been flawed since the very beginning.
Thanks for your help.

jsulm

@Alvein said in Randomly getting "QThread: Destroyed while thread is still running":

I'll leave this for later, since the threads I'm using don't need to be forcefully stopped

Just a note: this is not forcefully stopping. You only ask the threads to stop, they don't have to do that immediately, they can even ignore this.