deleteLater as a slot causes Access Vialoation

jronald


void StockIDUpdateTask::run()
{
    QUrl url("http://www.test.com");
    networkReply = networkAccessManager.get(QNetworkRequest(url));
    connect(networkReply, &QNetworkReply::finished, this, &MyThread::WebPageDownloadSucceeded);
    connect(networkReply, &QNetworkReply::finished, networkReply, &QNetworkReply::deleteLater);
    exec();
}

void MyThread::WebPageDownloadSucceeded()
{
    QByteArray ba = networkReply->readAll();
    return;
}

Here connect a signal to two slots, Doesn't they executed one by one? Why does it cause access violation? If the 2nd connect is commented out, then no access violation.

BTW, if call networkReply->deleteLater() in MyThread::WebPageDownloadSucceeded(), networkReply need to be a member of the class, not so cute.

hskoglund

Hi, you can try rewriting your WebPageDownloadSucceeded() function so that it accepts one argument (the networkReply), say like this:

void StockIDUpdateTask::run()
{
    QUrl url("http://www.test.com");
    networkReply = networkAccessManager.get(QNetworkRequest(url));
    connect(networkReply, &QNetworkReply::finished, this, &MyThread::WebPageDownloadSucceeded);
    exec();
}

void MyThread::WebPageDownloadSucceeded(QNetworkReply* networkReply)
{
    QByteArray ba = networkReply->readAll();
    networkReply->deleteLater();
}

That way you can call deleteLater() without crashes.

raven-worx

@jronald said in deleteLater as a slot causes Access Vialoation:
why don't you simply call deleteLater() when you know you've read all data in the first place?

I guess the issue is that in the first case the signal is "transmitted" via the even-loop (queued connection) to the other thread. Thus it is not guaranteed to be processed before the deleteLater() in the same thread (where the QNetworkReply lives in).

hskoglund

@raven-worx good point, so another possible solution could be:

...
connect(networkReply, &QNetworkReply::finished, this, &MyThread::WebPageDownloadSucceeded,Qt::BlockingQueuedConnection);
connect(networkReply, &QNetworkReply::finished, networkReply, &QNetworkReply::deleteLater);
...

VRonin

I don't really agree with the users before me (which normally means I'm wrong 😉 ). This is a fundamental design flaw.

QThread is not the thread, it's a wrapper around it.

Things created inside run() are on the secondary thread but thing created in the QThread constructor and the QThread itself are in the main thread.

This means networkReply = networkAccessManager.get(QNetworkRequest(url)); is a race condition and even calling networkReply->deleteLater(); from WebPageDownloadSucceeded would be (but probably deleteLater is smart enough to avoid it)

The segfault comes from the fact that deleteLater is direct connection (receiver and sender are the same so by definition they are on the same thread) while WebPageDownloadSucceeded is queued connected as networkReply lives in the secondary thread while this lives in the main thread.

Please see https://mayaposch.wordpress.com/2011/11/01/how-to-really-truly-use-qthreads-the-full-explanation/ and refactor your code to apply that tutorial