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).

4 files changed, 125 insertions, 79 deletions

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 <QtNetwork/QNetworkAccessManager>
-#include <QtNetwork/QNetworkReply>
 #include <QtNetwork/QNetworkRequest>
+#include <QtNetwork/QNetworkReply>
+#include <QtNetwork/QSslError>
 #include <QtCore/QTimer>
 
 #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<QNetworkReply, NetworkReplyDeleter> 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<void(QNetworkReply::*)(QNetworkReply::NetworkError)>(&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" );
diff --git a/jobs/basejob.h b/jobs/basejob.h
index 9d00c0ac..8e17e18e 100644
--- a/jobs/basejob.h
+++ b/jobs/basejob.h
@@ -19,10 +19,14 @@
 #ifndef QMATRIXCLIENT_BASEJOB_H
 #define QMATRIXCLIENT_BASEJOB_H
 
+#include <QtCore/QObject>
 #include <QtCore/QJsonDocument>
 #include <QtCore/QJsonObject>
 #include <QtCore/QUrlQuery>
-#include <QtNetwork/QNetworkReply>
+#include <QtCore/QScopedPointer>
+
+class QNetworkReply;
+class QSslError;
 
 namespace QMatrixClient
 {
@@ -109,7 +113,35 @@ namespace QMatrixClient
             virtual QString apiPath() const = 0;
             virtual QUrlQuery query() const;
             virtual QJsonObject data() const;
-            virtual void parseJson(const QJsonDocument& data);
+
+            /**
+             * Checks the received reply for sanity; calls setError/setErrorText
+             * respectively. setError() with any argument except NoError prevents
+             * further parseReply()/parseJson() invocations.
+             *
+             * @param reply the reply received from the server
+             */
+            virtual bool checkReply(QNetworkReply* reply);
+
+            /**
+             * Processes the reply. By default, parses the reply into
+             * a QJsonDocument and calls parseJson() if it's a valid JSON.
+             * Overrides MUST ensure that fail() or emitResult() is called
+             * on every execution path exactly once.
+             *
+             * @param data raw contents of a HTTP reply from the server (without headers)
+             */
+            virtual void parseReply(QByteArray data);
+
+            /**
+             * Processes the JSON document received from the Matrix server.
+             * By default emits a successful result without analysing the JSON.
+             * Overrides MUST ensure that fail() or emitResult() is called
+             * on every execution path exactly once.
+             *
+             * @param json valid JSON document received from the server
+             */
+            virtual void parseJson(const QJsonDocument&);
             
             /**
              * Sets the error code.
@@ -126,12 +158,16 @@ namespace QMatrixClient
              * @see emitResult(), fail()
              */
             void setError(int errorCode);
+            /**
+             * Sets the error text. Usually is combined with a setError() call
+             * before it, as setErrorText() alone does not indicate the error status.
+             */
             void setErrorText(QString errorText);
 
             /**
-             * Utility function to emit the result signal, and suicide this job.
-             * It first notifies the observers to hide the progress for this job using
-             * the finished() signal.
+             * Emits the result signal, and suicides this job.
+             * It first notifies the observers to hide the progress for this job
+             * using the finished() signal.
              *
              * @note: Deletes this job using deleteLater().
              *
@@ -139,20 +175,27 @@ namespace QMatrixClient
              * @see finished()
              */
             void emitResult();
-            void fail( int errorCode, QString errorString );
-            QNetworkReply* networkReply() const;
 
+            /**
+             * Same as emitResult() but calls setError() and setErrorText()
+             * with respective arguments passed to it. Use it as a shortcut to
+             * finish the job with a failure status.
+             */
+            void fail( int errorCode, QString errorString );
             
+
         protected slots:
-            virtual void gotReply();
             void timeout();
             void sslErrors(const QList<QSslError>& errors);
 
+        private slots:
+            void gotReply();
+
         private:
             void finishJob(bool emitResult);
 
             class Private;
-            Private* d;
+            QScopedPointer<Private> d;
     };
 }
 
diff --git a/jobs/mediathumbnailjob.cpp b/jobs/mediathumbnailjob.cpp
index 48ed0ffb..3f5f9ebf 100644
--- a/jobs/mediathumbnailjob.cpp
+++ b/jobs/mediathumbnailjob.cpp
@@ -70,17 +70,9 @@ QUrlQuery MediaThumbnailJob::query() const
     return query;
 }
 
-void MediaThumbnailJob::gotReply()
+void MediaThumbnailJob::parseReply(QByteArray data)
 {
-    if( networkReply()->error() != QNetworkReply::NoError )
-    {
-        qDebug() << "NetworkError!!!";
-        qDebug() << networkReply()->errorString();
-        fail( NetworkError, networkReply()->errorString() );
-        return;
-    }
-
-    if( !d->thumbnail.loadFromData( networkReply()->readAll() ) )
+    if( !d->thumbnail.loadFromData(data) )
     {
         qDebug() << "MediaThumbnailJob: could not read image data";
     }
diff --git a/jobs/mediathumbnailjob.h b/jobs/mediathumbnailjob.h
index 541163ed..1af8ecd4 100644
--- a/jobs/mediathumbnailjob.h
+++ b/jobs/mediathumbnailjob.h
@@ -40,8 +40,7 @@ namespace QMatrixClient
             QString apiPath() const override;
             QUrlQuery query() const override;
 
-        protected slots:
-            void gotReply() override;
+            virtual void parseReply(QByteArray data) override;
 
         private:
             class Private;