Merge pull request #3432 from uklotzde/webtask_error

Detect and prevent duplicate signals on network errors

4 files changed, 87 insertions, 51 deletions

diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp
index fd191c996d..864b1a8695 100644
--- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp
+++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp
@@ -118,22 +118,14 @@ bool MusicBrainzRecordingsTask::doStart(
         return false;
     }
 
+    // It is not necessary to connect the QNetworkReply::errorOccurred signal.
+    // Network errors are also received through the QNetworkReply::finished signal.
     connect(m_pendingNetworkReply,
             &QNetworkReply::finished,
             this,
             &MusicBrainzRecordingsTask::slotNetworkReplyFinished,
             Qt::UniqueConnection);
 
-    connect(m_pendingNetworkReply,
-#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
-            &QNetworkReply::errorOccurred,
-#else
-            QOverload<QNetworkReply::NetworkError>::of(&QNetworkReply::error),
-#endif
-            this,
-            &MusicBrainzRecordingsTask::slotNetworkReplyFinished,
-            Qt::UniqueConnection);
-
     return true;
 }
 
diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp
index 07d45b8edd..bac64db6bc 100644
--- a/src/network/jsonwebtask.cpp
+++ b/src/network/jsonwebtask.cpp
@@ -143,6 +143,7 @@ JsonWebTask::~JsonWebTask() {
 void JsonWebTask::onFinished(
         JsonWebResponse&& response) {
     kLogger.info()
+            << this
             << "Response received"
             << response.replyUrl
             << response.statusCode
@@ -153,6 +154,7 @@ void JsonWebTask::onFinished(
 void JsonWebTask::onFinishedCustom(
         CustomWebResponse&& response) {
     kLogger.info()
+            << this
             << "Custom response received"
             << response.replyUrl
             << response.statusCode
@@ -170,6 +172,7 @@ QNetworkReply* JsonWebTask::sendNetworkRequest(
         DEBUG_ASSERT(m_request.content.isEmpty());
         if (kLogger.debugEnabled()) {
             kLogger.debug()
+                    << this
                     << "GET"
                     << url;
         }
@@ -180,6 +183,7 @@ QNetworkReply* JsonWebTask::sendNetworkRequest(
         const auto body = content.toJson(QJsonDocument::Compact);
         if (kLogger.debugEnabled()) {
             kLogger.debug()
+                    << this
                     << "PUT"
                     << url
                     << body;
@@ -192,6 +196,7 @@ QNetworkReply* JsonWebTask::sendNetworkRequest(
         const auto body = content.toJson(QJsonDocument::Compact);
         if (kLogger.debugEnabled()) {
             kLogger.debug()
+                    << this
                     << "POST"
                     << url
                     << body;
@@ -204,6 +209,7 @@ QNetworkReply* JsonWebTask::sendNetworkRequest(
         const auto body = m_request.content.toJson(QJsonDocument::Compact);
         if (kLogger.debugEnabled()) {
             kLogger.debug()
+                    << this
                     << "PATCH"
                     << url
                     << body;
@@ -226,6 +232,7 @@ QNetworkReply* JsonWebTask::sendNetworkRequest(
         DEBUG_ASSERT(content.isEmpty());
         if (kLogger.debugEnabled()) {
             kLogger.debug()
+                    << this
                     << "DELETE"
                     << url;
         }
@@ -245,6 +252,7 @@ bool JsonWebTask::doStart(
     DEBUG_ASSERT(networkAccessManager);
     VERIFY_OR_DEBUG_ASSERT(!m_pendingNetworkReply) {
         kLogger.warning()
+                << this
                 << "Task has already been started";
         return false;
     }
@@ -264,26 +272,19 @@ bool JsonWebTask::doStart(
             m_request.content);
     VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply) {
         kLogger.warning()
+                << this
                 << "Request not sent";
         return false;
     }
 
+    // It is not necessary to connect the QNetworkReply::errorOccurred signal.
+    // Network errors are also received through the QNetworkReply::finished signal.
     connect(m_pendingNetworkReply,
             &QNetworkReply::finished,
             this,
             &JsonWebTask::slotNetworkReplyFinished,
             Qt::UniqueConnection);
 
-    connect(m_pendingNetworkReply,
-#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
-            &QNetworkReply::errorOccurred,
-#else
-            QOverload<QNetworkReply::NetworkError>::of(&QNetworkReply::error),
-#endif
-            this,
-            &JsonWebTask::slotNetworkReplyFinished,
-            Qt::UniqueConnection);
-
     return true;
 }
 
@@ -351,6 +352,7 @@ void JsonWebTask::emitFailed(
     VERIFY_OR_DEBUG_ASSERT(
             isSignalFuncConnected(&JsonWebTask::failed)) {
         kLogger.warning()
+                << this
                 << "Unhandled failed signal"
                 << response;
         deleteLater();
diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp
index 534c582b22..b1481df2d6 100644
--- a/src/network/webtask.cpp
+++ b/src/network/webtask.cpp
@@ -92,10 +92,14 @@ WebTask::~WebTask() {
 
 void WebTask::onAborted(
         QUrl&& requestUrl) {
-    DEBUG_ASSERT(m_status == Status::Aborted);
+    VERIFY_OR_DEBUG_ASSERT(m_status == Status::Aborting) {
+        return;
+    }
+    m_status = Status::Aborted;
     VERIFY_OR_DEBUG_ASSERT(
             isSignalFuncConnected(&WebTask::aborted)) {
         kLogger.warning()
+                << this
                 << "Unhandled abort signal"
                 << requestUrl;
         deleteLater();
@@ -107,7 +111,13 @@ void WebTask::onAborted(
 
 void WebTask::onTimedOut(
         QUrl&& requestUrl) {
-    DEBUG_ASSERT(m_status == Status::TimedOut);
+    VERIFY_OR_DEBUG_ASSERT(m_status == Status::Pending) {
+        return;
+    }
+    if (m_timeoutTimerId != kInvalidTimerId) {
+        killTimer(m_timeoutTimerId);
+        m_timeoutTimerId = kInvalidTimerId;
+    }
     onNetworkError(
             std::move(requestUrl),
             QNetworkReply::TimeoutError,
@@ -120,10 +130,21 @@ void WebTask::onNetworkError(
         QNetworkReply::NetworkError errorCode,
         QString&& errorString,
         QByteArray&& errorContent) {
+    DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
+    VERIFY_OR_DEBUG_ASSERT(m_status == Status::Pending) {
+        return;
+    }
+    DEBUG_ASSERT(errorCode != QNetworkReply::NoError);
+    if (errorCode == QNetworkReply::TimeoutError) {
+        m_status = Status::TimedOut;
+    } else {
+        m_status = Status::Failed;
+    }
     VERIFY_OR_DEBUG_ASSERT(
             isSignalFuncConnected(&WebTask::networkError)) {
         kLogger.warning()
-                << "Unhandled network error signal"
+                << this
+                << "Unhandled network error:"
                 << requestUrl
                 << errorCode
                 << errorString
@@ -168,8 +189,13 @@ void WebTask::invokeAbort() {
 
 void WebTask::slotStart(int timeoutMillis) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    DEBUG_ASSERT(m_status != Status::Pending);
+    VERIFY_OR_DEBUG_ASSERT(m_status != Status::Pending) {
+        return;
+    }
+    m_status = Status::Idle;
+
     VERIFY_OR_DEBUG_ASSERT(m_networkAccessManager) {
+        m_status = Status::Pending;
         onNetworkError(
                 QUrl(),
                 QNetworkReply::NetworkSessionFailedError,
@@ -179,13 +205,14 @@ void WebTask::slotStart(int timeoutMillis) {
     }
 
     kLogger.debug()
+            << this
             << "Starting...";
-    m_status = Status::Idle;
     if (!doStart(m_networkAccessManager, timeoutMillis)) {
         // Still idle, because we are in the same thread.
         // The callee is not supposed to abort a request
         // before it has beeen started successfully.
         DEBUG_ASSERT(m_status == Status::Idle);
+        m_status = Status::Pending;
         onNetworkError(
                 QUrl(),
                 QNetworkReply::OperationCanceledError,
@@ -240,8 +267,9 @@ QUrl WebTask::abort() {
         killTimer(m_timeoutTimerId);
         m_timeoutTimerId = kInvalidTimerId;
     }
-    m_status = Status::Aborted;
+    m_status = Status::Aborting;
     kLogger.debug()
+            << this
             << "Aborting...";
     QUrl url = doAbort();
     onAborted(QUrl(url));
@@ -256,16 +284,16 @@ void WebTask::timerEvent(QTimerEvent* event) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     const auto timerId = event->timerId();
     DEBUG_ASSERT(timerId != kInvalidTimerId);
-    if (timerId != m_timeoutTimerId) {
-        // ignore
+    VERIFY_OR_DEBUG_ASSERT(timerId == m_timeoutTimerId) {
         return;
     }
     killTimer(m_timeoutTimerId);
     m_timeoutTimerId = kInvalidTimerId;
-    if (m_status != Status::Aborted) {
-        m_status = Status::TimedOut;
+    VERIFY_OR_DEBUG_ASSERT(m_status == Status::Pending) {
+        return;
     }
-    kLogger.debug()
+    kLogger.info()
+            << this
             << "Timed out";
     onTimedOut(doTimeOut());
 }
@@ -280,34 +308,16 @@ QPair<QNetworkReply*, HttpStatusCode> WebTask::receiveNetworkReply() {
     }
     networkReply->deleteLater();
 
-    if (m_timeoutTimerId != kInvalidTimerId) {
-        killTimer(m_timeoutTimerId);
-        m_timeoutTimerId = kInvalidTimerId;
-    }
-
-    if (m_status == Status::Aborted) {
-        onAborted(networkReply->request().url());
-        return qMakePair(nullptr, statusCode);
-    }
-    m_status = Status::Finished;
-
-    if (networkReply->error() != QNetworkReply::NetworkError::NoError) {
-        onNetworkError(
-                networkReply->request().url(),
-                networkReply->error(),
-                networkReply->errorString(),
-                networkReply->readAll());
-        return qMakePair(nullptr, statusCode);
-    }
-
     if (kLogger.debugEnabled()) {
         if (networkReply->url() == networkReply->request().url()) {
             kLogger.debug()
+                    << this
                     << "Received reply for request"
                     << networkReply->url();
         } else {
             // Redirected
             kLogger.debug()
+                    << this
                     << "Received reply for redirected request"
                     << networkReply->request().url()
                     << "->"
@@ -315,9 +325,39 @@ QPair<QNetworkReply*, HttpStatusCode> WebTask::receiveNetworkReply() {
         }
     }
 
+    if (m_status == Status::Aborted ||
+            m_status == Status::TimedOut) {
+        // Already aborted or timed out by the client
+        DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
+        kLogger.debug()
+                << this
+                << "Ignoring obsolete network reply";
+        return qMakePair(nullptr, statusCode);
+    }
+    VERIFY_OR_DEBUG_ASSERT(m_status == Status::Pending) {
+        DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
+        return qMakePair(nullptr, statusCode);
+    }
+
+    if (m_timeoutTimerId != kInvalidTimerId) {
+        killTimer(m_timeoutTimerId);
+        m_timeoutTimerId = kInvalidTimerId;
+    }
+
+    if (networkReply->error() != QNetworkReply::NetworkError::NoError) {
+        onNetworkError(
+                networkReply->request().url(),
+                networkReply->error(),
+                networkReply->errorString(),
+                networkReply->readAll());
+        return qMakePair(nullptr, statusCode);
+    }
+    m_status = Status::Finished;
+
     DEBUG_ASSERT(statusCode == kHttpStatusCodeInvalid);
     VERIFY_OR_DEBUG_ASSERT(readStatusCode(networkReply, &statusCode)) {
         kLogger.warning()
+                << this
                 << "Failed to read HTTP status code";
     }
 
diff --git a/src/network/webtask.h b/src/network/webtask.h
index 8735d3c824..98f60880ac 100644
--- a/src/network/webtask.h
+++ b/src/network/webtask.h
@@ -148,8 +148,10 @@ class WebTask : public QObject {
     enum class Status {
         Idle,
         Pending,
+        Aborting,
         Aborted,
         TimedOut,
+        Failed,
         Finished,
     };