From 3254963c1ea587921d2a434839aa703b9aaff6cc Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 26 Jul 2016 16:49:16 +0900 Subject: Split BaseJob::gotReply into checkReply and parseReply + internal tweaks to BaseJob 1. The externally (for derived classes) visible additions are checkReply() and parseReply() virtual methods, with gotReply becoming a mere dispatcher (and therefore a private method). Splitting gotReply() in that way allowed to remove boilerplate code from MediaThumbnailJob. 2. The internal tweak is using QScopedPointer<> to store pointers both to the Private object and to a QNetworkReply (with a special deleter that aborts the reply before destructing the object). This allows to remove desperate attempts to call reply->abort() wherever it's no more needed (and not aborting the in-flight replies seems to be a/the culprit of Quaternion after-exit hangs). --- jobs/basejob.cpp | 128 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 70 insertions(+), 58 deletions(-) (limited to 'jobs/basejob.cpp') diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index 2c95ef11..9ab43087 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -19,14 +19,25 @@ #include "basejob.h" #include -#include #include +#include +#include #include #include "../connectiondata.h" using namespace QMatrixClient; +struct NetworkReplyDeleter : public QScopedPointerDeleteLater +{ + static inline void cleanup(QNetworkReply* reply) + { + if (reply && reply->isRunning()) + reply->abort(); + QScopedPointerDeleteLater::cleanup(reply); + } +}; + class BaseJob::Private { public: @@ -35,7 +46,7 @@ class BaseJob::Private {} ConnectionData* connection; - QNetworkReply* reply; + QScopedPointer reply; JobHttpType type; bool needsToken; @@ -52,13 +63,6 @@ BaseJob::BaseJob(ConnectionData* connection, JobHttpType type, QString name, boo BaseJob::~BaseJob() { - if( d->reply ) - { - if( d->reply->isRunning() ) - d->reply->abort(); - d->reply->deleteLater(); - } - delete d; qDebug() << "Job" << objectName() << " destroyed"; } @@ -77,11 +81,6 @@ QUrlQuery BaseJob::query() const return QUrlQuery(); } -void BaseJob::parseJson(const QJsonDocument& data) -{ - emitResult(); -} - void BaseJob::start() { QUrl url = d->connection->baseUrl(); @@ -100,26 +99,75 @@ void BaseJob::start() switch( d->type ) { case JobHttpType::GetJob: - d->reply = d->connection->nam()->get(req); + d->reply.reset( d->connection->nam()->get(req) ); break; case JobHttpType::PostJob: - d->reply = d->connection->nam()->post(req, data.toJson()); + d->reply.reset( d->connection->nam()->post(req, data.toJson()) ); break; case JobHttpType::PutJob: - d->reply = d->connection->nam()->put(req, data.toJson()); + d->reply.reset( d->connection->nam()->put(req, data.toJson()) ); break; } - connect( d->reply, &QNetworkReply::sslErrors, this, &BaseJob::sslErrors ); - connect( d->reply, &QNetworkReply::finished, this, &BaseJob::gotReply ); + connect( d->reply.data(), &QNetworkReply::sslErrors, this, &BaseJob::sslErrors ); + connect( d->reply.data(), &QNetworkReply::finished, this, &BaseJob::gotReply ); QTimer::singleShot( 120*1000, this, SLOT(timeout()) ); // connect( d->reply, static_cast(&QNetworkReply::error), // this, &BaseJob::networkError ); // http://doc.qt.io/qt-5/qnetworkreply.html#error-1 } +void BaseJob::gotReply() +{ + if (checkReply(d->reply.data())) + parseReply(d->reply->readAll()); + // FIXME: we should not hold parseReply()/parseJson() responsible for + // emitting the result; it should be done here instead. +} + +bool BaseJob::checkReply(QNetworkReply* reply) +{ + switch( reply->error() ) + { + case QNetworkReply::NoError: + return true; + + case QNetworkReply::AuthenticationRequiredError: + case QNetworkReply::ContentAccessDenied: + case QNetworkReply::ContentOperationNotPermittedError: + qDebug() << "Content access error, Qt error code:" << reply->error(); + fail( ContentAccessError, reply->errorString() ); + return false; + + default: + qDebug() << "NetworkError, Qt error code:" << reply->error(); + fail( NetworkError, reply->errorString() ); + return false; + } +} + +void BaseJob::parseReply(QByteArray data) +{ + QJsonParseError error; + QJsonDocument json = QJsonDocument::fromJson(data, &error); + if( error.error == QJsonParseError::NoError ) + parseJson(json); + else + fail( JsonParseError, error.errorString() ); +} + +void BaseJob::parseJson(const QJsonDocument&) +{ + // Do nothing by default + emitResult(); +} + void BaseJob::finishJob(bool emitResult) { - if( d->reply->isRunning() ) - d->reply->abort(); + if (!d->reply) + { + qWarning() << objectName() + << ": empty network reply (finish() called more than once?)"; + return; + } // Notify those that are interested in any completion of the job (including killing) emit finished(this); @@ -132,6 +180,7 @@ void BaseJob::finishJob(bool emitResult) emit success(this); } + d->reply.reset(); deleteLater(); } @@ -169,47 +218,10 @@ void BaseJob::fail(int errorCode, QString errorString) { setError( errorCode ); setErrorText( errorString ); - if( d->reply && d->reply->isRunning() ) - d->reply->abort(); qWarning() << "Job" << objectName() << "failed:" << errorString; emitResult(); } -QNetworkReply* BaseJob::networkReply() const -{ - return d->reply; -} - -void BaseJob::gotReply() -{ - switch( d->reply->error() ) - { - case QNetworkReply::NoError: - break; // All good, go to the normal flow after the switch() - - case QNetworkReply::AuthenticationRequiredError: - case QNetworkReply::ContentAccessDenied: - case QNetworkReply::ContentOperationNotPermittedError: - qDebug() << "Content access error, Qt error code:" << d->reply->error(); - fail( ContentAccessError, d->reply->errorString() ); - return; - - default: - qDebug() << "NetworkError, Qt error code:" << d->reply->error(); - fail( NetworkError, d->reply->errorString() ); - return; - } - - QJsonParseError error; - QJsonDocument data = QJsonDocument::fromJson(d->reply->readAll(), &error); - if( error.error != QJsonParseError::NoError ) - { - fail( JsonParseError, error.errorString() ); - return; - } - parseJson(data); -} - void BaseJob::timeout() { fail( TimeoutError, "The job has timed out" ); -- cgit v1.2.3