QNetworkAccessManager Crashing in 5.15 when used in another thread

rhb327

Are there any limitations regarding using QNetworkAccessManager in a separate thread where that thread has its own event loop?

I seem to be able to repeatedly create a crash in <1hr of operation using and by either moving the QNetworkAcessManager from the thread init() to the constructor or just not creating a separate thread stability returns.

Anyone else have insight as to why this might be?

Thanks,
-Rich

Christian Ehrlicher

@rhb327 said in QNetworkAccessManager Crashing in 5.15 when used in another thread:

Are there any limitations regarding using QNetworkAccessManager in a separate thread where that thread has its own event loop?

No

moving the QNetworkAcessManager from the thread init() to the constructor or just not creating a separate thread stability returns.

So it no longer runs in the thread you created and the separate thread is superfluous

I don't see a need for a QNetworkAccessManager in a separate thread at all - why do you think you need it?

It's most likely a threading problem on your side but without code...

rhb327

I can probably remove the separate thread as it primarily has a QProcess (monitoring an MQTT Python library) and a QNetworkAccessManager handling for cloud communications and most of it seems event driven. Testing this now.

That said, I've worked 15+ crashes over the last two days and simply moving the QNetworkAccessManager to the main thread in one of the two ways mentioned in the OP has yet to crash.

I may try to create a small project to demo this but I see no consistency in the crashes other than 1) I have to enable my cloud communications feature and 2) QNetworkAccessManager must run in the separate thread.

Thanks,
-Rich

rhb327

I'm thinking this could be involved here: https://bugreports.qt.io/browse/QTBUG-38309

I'm still seeing some crashes after moving to main thread only but they are reduced. I think the QT source for QNAM finished() is as so:


void QNetworkAccessManagerPrivate::_q_replyFinished()
{
    Q_Q(QNetworkAccessManager);
    QNetworkReply *reply = qobject_cast<QNetworkReply *>(q->sender());
    if (reply) {
        emit q->finished(reply);
        if (reply->request().attribute(QNetworkRequest::AutoDeleteReplyOnFinishAttribute, false).toBool())
            QMetaObject::invokeMethod(reply, [reply] { reply->deleteLater(); }, Qt::QueuedConnection);
    }
#ifndef QT_NO_BEARERMANAGEMENT
    // If there are no active requests, release our reference to the network session.
    // It will not be destroyed immediately, but rather when the connection cache is flushed
    // after 2 minutes.
    activeReplyCount--;
    if (networkSessionStrongRef && activeReplyCount == 0)
        networkSessionStrongRef.clear();
#endif
}

I think if the deleteLater() call here does not complete before I deleteLater() in my QNAM finished() SLOT then there could be an issue.

Considering adding the following in my finished() signal slot:

QTimer::singleShot(1000, reply, SLOT(deleteLater()));

jeremy_k

@rhb327 said in QNetworkAccessManager Crashing in 5.15 when used in another thread:

I'm thinking this could be involved here: https://bugreports.qt.io/browse/QTBUG-38309

I'm still seeing some crashes after moving to main thread only but they are reduced. I think the QT source for QNAM finished() is as so:
void QNetworkAccessManagerPrivate::_q_replyFinished()
{
    Q_Q(QNetworkAccessManager);
    QNetworkReply *reply = qobject_cast<QNetworkReply *>(q->sender());
    if (reply) {
        emit q->finished(reply);
        if (reply->request().attribute(QNetworkRequest::AutoDeleteReplyOnFinishAttribute, false).toBool())
            QMetaObject::invokeMethod(reply, [reply] { reply->deleteLater(); }, Qt::QueuedConnection);
[...]
}

That bug report mentions Qt 5.0.2. The deleteLater() based on QNetworkRequest::AutoDeleteReplyOnFinishAttribute
is a feature introduced in 5.14.0. They are unlikely to be related.

I think if the deleteLater() call here does not complete before I deleteLater() in my QNAM finished() SLOT then there could be an issue.

deleteLater() isn't attempted until after emit q->finished(reply);. At that point, all direct and auto connections in the same thread have completed. If the crashing code is using a queued connection in the same thread, it can't use the autodelete option. If it is in a different thread, it should not dereference the reply pointer. QNetworkReply is derived from QObject, and therefore not thread safe.

Considering adding the following in my finished() signal slot:
QTimer::singleShot(1000, reply, SLOT(deleteLater()));

Is your code using the AutoDeleteReplyOnFinishAttribute attribute or QNetworkAccessManager::setAutoDeleteReplies()?

If not, QNAM is not deleting the replies. In that case, this safe but a pointless delay.
If yes, the reply should be treated as deleted immediately after all direct connections that receive the reply finish. Setting a timer to attempt a deleteLater() will try to access an object that has already been deleted.

rhb327

Hmmm, not sure I'm 100% following.

I do not have autoDelete set to true and it appears to default to false. I do plan to test this though and remove my deleteLater(). I've stubbed in OPTION4.
I'll note that my original code OPTION1 below seemed to be just fine on 5.12 and my issue is in 5.15.

Here's QNAMgr setup. I intially had this in the thread init() so QNAMgr would be owned by the thread but have subsequently moved it to the class constructor so the main thread will own QANMgr. I've been testing 1) not using any separate thread at all and 2) using a separate thread but letting QANMgr be owned by main. Both of these options increased observed stability.

    networkManager = new QNetworkAccessManager(this);
#ifdef OPTION4
    networkManager->setAutoDeleteReplies(true);
    connect(networkManager, SIGNAL(finished(QNetworkReply*)), this, SLOT(responseFrom9000Port(QNetworkReply*)), Qt::DirectConnection);
#else
    connect(networkManager, SIGNAL(finished(QNetworkReply*)), this, SLOT(responseFrom9000Port(QNetworkReply*)));
#endif

Here's an example of my receive SLOT:

void CloudData::responseFrom9000Port(QNetworkReply* reply) {
    qDebug() << "9000 Response Thread: " << QThread::currentThreadId();
    if(reply) {
        QNetworkReply::NetworkError error_type = QNetworkReply::NetworkError::NoError;
        error_type = reply->error();
        QByteArray resp = reply->readAll();
        qDebug() << "9000 Response port and error type: " << resp << error_type << networkManager->autoDeleteReplies();
        int httpStatusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();

#ifdef OPTION1
        //OPTION1:  Must not have autoDelete true
        reply->deleteLater();
#endif

#ifdef OPTION2
        //OPTION2:  Must not have autoDelete true
        // See: https://bugreports.qt.io/browse/QTBUG-38309 and
        //      https://stackoverflow.com/questions/5863957/qt-qnetworkreply-delete-operator-runtime-crash
        QTimer::singleShot(1000, reply, SLOT(deleteLater()));
#endfi

#ifdef OPTION3
        //OPTION3:  Must not have autoDelete true
        while (!reply->isFinished());
        reply->deleteLater();
#endif

        //OPTION4:  Must not use a queued connection with this option
        // Enable reply auto delete via networkManager->setAutoDeleteReplies(true) and remove deleteLater
        // This option should be tested with CLOUD_SEPTHREAD active and inactive

        if(httpStatusCode != 200) {
            qWarning() << "Network Reply service error type: "<< httpStatusCode << error_type;
            /**
            * Stop posting messages when there is a network session or network unknown error  *
            * We can be sure that the wifi is not available due to some bad connection.       *
            **/
            if(error_type == QNetworkReply::NetworkError::NetworkSessionFailedError ||
                    error_type == QNetworkReply::NetworkError::UnknownNetworkError ||
                    error_type == QNetworkReply::NetworkError::ConnectionRefusedError) {
                qWarning()<<"Wait for the response reply from 9000 port";
            }
        }
    }
    else {
        qWarning() << "9000 Reponse is NULL";
    }
}

Here's a post:

    QUrl serviceIotUrl;
    serviceIotUrl.setScheme("http");
    serviceIotUrl.setHost("127.0.0.1");
    serviceIotUrl.setPath("/v1/iotrequest");
    serviceIotUrl.setPort(9000);
    networkIotRequest.setHeader(QNetworkRequest::ContentTypeHeader, "application/json");
    networkIotRequest.setUrl(serviceIotUrl);


//--------------------------------


QNetworkReply* CloudData::postJsonMessage(QJsonObject cloudObject, QString messageID) {
    if(wifiConnectionStatus && deepLaserRunning) {
        uuidQueue.append(messageID);
        qDebug() << "postJsonMsg(): " << QThread::currentThreadId() << networkManager << uuidQueue.count();
        if(networkManager) {
            return networkManager->post(networkIotRequest, QJsonDocument(cloudObject).toJson());
        }
    }
    return nullptr;
}

I have yet to test OPTION3 or OPTION4 as they are just stubbed in for now based on learnings to date. Thanks for the inputs from all as I'm under pressure with upcoming release...much appreciated by the Qt community!

Thanks,
-Rich

jeremy_k

@rhb327 said in QNetworkAccessManager Crashing in 5.15 when used in another thread:

Here's QNAMgr setup. I intially had this in the thread init() so QNAMgr would be owned by the thread but have subsequently moved it to the class constructor so the main thread will own QANMgr. I've been testing 1) not using any separate thread at all and 2) using a separate thread but letting QANMgr be owned by main. Both of these options increased observed stability.

Unfortunately neither this description nor the code that follows tells me anything about the threads of execution. There is no mention of which thread API is being used, eg QThread, QtConcurrent, pthread, std::thread... thread init() has no technical meaning in any of the Qt thread APIs.

A complete example that skips all of the unrelated steps (port numbers, headers, error handling, etc) would be much clearer.

rhb327

I can work on a smaller example. But to try and drive some understanding, the following code allows me to put my cryoCloud class in it's own thread or keep in the main. That's all I was trying to convey.

   cryoCloud = CloudData::instance();
#ifdef CLOUD_SEPTHREAD
    cloudThread = new QThread(nullptr);
    cloudThread->setParent(nullptr);
    cryoCloud->moveToThread(cloudThread);
    connect(cloudThread, SIGNAL(finished()), cloudThread, SLOT(deleteLater()));
    connect(cloudThread, SIGNAL(started()), cryoCloud, SLOT(startInitialize()));
#endif

#ifdef CLOUD_SEPTHREAD
    cloudThread->start();
#endif

cryoCloud constructor looks like this:

CloudData::CloudData(QObject* parent): QObject(parent) {
#ifndef CLOUD_SEPTHREAD
    startInitialize();
# endif
//#ifdef OPTION4
//    networkManager->setAutoDeleteReplies(true);
//    connect(networkManager, SIGNAL(finished(QNetworkReply*)), this, //SLOT(responseFrom9000Port(QNetworkReply*)), Qt::DirectConnection);
//#else
    networkManager->setAutoDeleteReplies(false);
    connect(networkManager, SIGNAL(finished(QNetworkReply*)), this, SLOT(responseFrom9000Port(QNetworkReply*)));
//#endif
}

So when I want to run cryoCloud in the main thread I simply do not define CLOUD_SEPTHREAD. If I want cryoCloud in its own thread then I define CLOUD_SEPTHREAD. In both cases I'm keeping networkManager in the cryoCloud constructor as I want it in main right now as that seems to add stability. Normally, the networkManager instantiation would be in startInitialize() for the case where cryoCloud runs in its own thread.

As I'm using moveToThread, cryoCloud would have it's own event loop. I'm not sure this will make things clearer but hope so.

My latest stability observations are interesting. When I run CLOUD_SEPTHREAD not defined where cryoCloud is in the main thread and I comment out deleteLater() in the networManager's finished() handler (intentional memory leak) I've not had a crash in >7hrs. I tired OPTION4 with just the one thread setup (CLOUD_SEPTHREAD not defined) where autoDelete is enabled and had a crash in ~4hrs.

So from what I can tell I have an issue with the reply->deleteLater() call in the networkManager's finished() handler. Searching legacy issues is where OPTION1-3 concepts came from but agree those are dated issues to your earlier input. Now I'm starting to look at 5.12.4 (where I was) vs. 5.15.2 (where I am) to see if I can better understand the differences.

(

I may certainly not have zeroed in on the crash cause yet but it definitely appears related to posting and receiving via networkManager.

Thanks,
-Rich

jeremy_k

@rhb327 said in QNetworkAccessManager Crashing in 5.15 when used in another thread:

In both cases I'm keeping networkManager in the cryoCloud constructor as I want it in main right now as that seems to add stability.

To quote the documentation:
Since QNetworkAccessManager is based on QObject, it can only be used from the thread it belongs to.

This means that all requests must come from the the thread the manager belongs to. As QNetworkReply is also derived from QObject, all replies must be received in the same thread.

rhb327

Yes and it's all in the main thread now.