diff options
author | Uwe Klotz <uklotz@mixxx.org> | 2020-12-13 11:07:27 +0100 |
---|---|---|
committer | Uwe Klotz <uklotz@mixxx.org> | 2020-12-13 18:50:26 +0100 |
commit | 909b2e890918535e9203d6e080d268c83fd6cfd7 (patch) | |
tree | 158676be154d925de8360b3a1181f27240601f50 /src | |
parent | 8ff5d47214ba4048d7dbad7436a57eb60850157f (diff) |
Networking: Move shared code into WebTask base class
Diffstat (limited to 'src')
-rw-r--r-- | src/musicbrainz/web/musicbrainzrecordingstask.cpp | 87 | ||||
-rw-r--r-- | src/musicbrainz/web/musicbrainzrecordingstask.h | 13 | ||||
-rw-r--r-- | src/network/jsonwebtask.cpp | 89 | ||||
-rw-r--r-- | src/network/jsonwebtask.h | 14 | ||||
-rw-r--r-- | src/network/webtask.cpp | 266 | ||||
-rw-r--r-- | src/network/webtask.h | 56 |
6 files changed, 184 insertions, 341 deletions
diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp index 864b1a8695..5afec1225e 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp +++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp @@ -76,28 +76,18 @@ MusicBrainzRecordingsTask::MusicBrainzRecordingsTask( musicbrainz::registerMetaTypesOnce(); } -MusicBrainzRecordingsTask::~MusicBrainzRecordingsTask() { - VERIFY_OR_DEBUG_ASSERT(!m_pendingNetworkReply) { - m_pendingNetworkReply->deleteLater(); - } -} - -bool MusicBrainzRecordingsTask::doStart( +QNetworkReply* MusicBrainzRecordingsTask::doStartNetworkRequest( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) { - m_parentTimeoutMillis = parentTimeoutMillis; DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); DEBUG_ASSERT(networkAccessManager); - VERIFY_OR_DEBUG_ASSERT(!m_pendingNetworkReply) { - kLogger.warning() - << "Task has already been started"; - return false; - } + + m_parentTimeoutMillis = parentTimeoutMillis; VERIFY_OR_DEBUG_ASSERT(!m_queuedRecordingIds.isEmpty()) { kLogger.warning() << "Nothing to do"; - return false; + return nullptr; } const auto recordingId = m_queuedRecordingIds.takeFirst(); DEBUG_ASSERT(!recordingId.isNull()); @@ -110,73 +100,24 @@ bool MusicBrainzRecordingsTask::doStart( << "GET" << networkRequest.url(); } - m_pendingNetworkReply = - networkAccessManager->get(networkRequest); - VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply) { - kLogger.warning() - << "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, - &MusicBrainzRecordingsTask::slotNetworkReplyFinished, - Qt::UniqueConnection); - - return true; + return networkAccessManager->get(networkRequest); } -QUrl MusicBrainzRecordingsTask::doAbort() { - QUrl requestUrl; - if (m_pendingNetworkReply) { - requestUrl = abortPendingNetworkReply(m_pendingNetworkReply); - if (requestUrl.isValid()) { - // Already finished - m_pendingNetworkReply->deleteLater(); - m_pendingNetworkReply = nullptr; - } - } - return requestUrl; -} - -QUrl MusicBrainzRecordingsTask::doTimeOut() { +void MusicBrainzRecordingsTask::doNetworkReplyFinished( + QNetworkReply* finishedNetworkReply, + network::HttpStatusCode statusCode) { DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - QUrl requestUrl; - if (m_pendingNetworkReply) { - requestUrl = timeOutPendingNetworkReply(m_pendingNetworkReply); - // Don't wait until finished - m_pendingNetworkReply->deleteLater(); - m_pendingNetworkReply = nullptr; - } - return requestUrl; -} - -void MusicBrainzRecordingsTask::slotNetworkReplyFinished() { - DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - const QPair<QNetworkReply*, network::HttpStatusCode> - networkReplyWithStatusCode = receiveNetworkReply(); - auto* const networkReply = networkReplyWithStatusCode.first; - if (!networkReply) { - // already aborted - return; - } - const auto statusCode = networkReplyWithStatusCode.second; - VERIFY_OR_DEBUG_ASSERT(networkReply == m_pendingNetworkReply) { - return; - } - m_pendingNetworkReply = nullptr; - const QByteArray body = networkReply->readAll(); + const QByteArray body = finishedNetworkReply->readAll(); QXmlStreamReader reader(body); // HTTP status of successful results: // 200: Found // 301: Found, but UUID moved permanently in database // 404: Not found in database, i.e. empty result - if (statusCode != 200 && statusCode != 301 && statusCode != 404) { + if (statusCode != 200 && + statusCode != 301 && + statusCode != 404) { kLogger.info() << "GET reply" << "statusCode:" << statusCode @@ -184,7 +125,7 @@ void MusicBrainzRecordingsTask::slotNetworkReplyFinished() { auto error = musicbrainz::Error(reader); emitFailed( network::WebResponse( - networkReply->url(), + finishedNetworkReply->url(), statusCode), error.code, std::move(error.message)); @@ -205,7 +146,7 @@ void MusicBrainzRecordingsTask::slotNetworkReplyFinished() { << "Failed to parse XML response"; emitFailed( network::WebResponse( - networkReply->url(), + finishedNetworkReply->url(), statusCode), -1, "Failed to parse XML response"); diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.h b/src/musicbrainz/web/musicbrainzrecordingstask.h index 953298eb4c..39678d65ff 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.h +++ b/src/musicbrainz/web/musicbrainzrecordingstask.h @@ -19,7 +19,7 @@ class MusicBrainzRecordingsTask : public network::WebTask { QNetworkAccessManager* networkAccessManager, QList<QUuid>&& recordingIds, QObject* parent = nullptr); - ~MusicBrainzRecordingsTask() override; + ~MusicBrainzRecordingsTask() override = default; signals: void succeeded( @@ -29,15 +29,13 @@ class MusicBrainzRecordingsTask : public network::WebTask { int errorCode, const QString& errorMessage); - private slots: - void slotNetworkReplyFinished(); - private: - bool doStart( + QNetworkReply* doStartNetworkRequest( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) override; - QUrl doAbort() override; - QUrl doTimeOut() override; + void doNetworkReplyFinished( + QNetworkReply* finishedNetworkReply, + network::HttpStatusCode statusCode) override; void emitSucceeded( QList<musicbrainz::TrackRelease>&& trackReleases); @@ -55,7 +53,6 @@ class MusicBrainzRecordingsTask : public network::WebTask { QMap<QUuid, musicbrainz::TrackRelease> m_trackReleases; - QPointer<QNetworkReply> m_pendingNetworkReply; int m_parentTimeoutMillis; }; diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index bac64db6bc..0e355db369 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -128,18 +128,11 @@ JsonWebTask::JsonWebTask( QObject* parent) : WebTask(networkAccessManager, parent), m_baseUrl(baseUrl), - m_request(std::move(request)), - m_pendingNetworkReply(nullptr) { + m_request(std::move(request)) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); DEBUG_ASSERT(!m_baseUrl.isEmpty()); } -JsonWebTask::~JsonWebTask() { - if (m_pendingNetworkReply) { - m_pendingNetworkReply->deleteLater(); - } -} - void JsonWebTask::onFinished( JsonWebResponse&& response) { kLogger.info() @@ -244,18 +237,12 @@ QNetworkReply* JsonWebTask::sendNetworkRequest( return nullptr; } -bool JsonWebTask::doStart( +QNetworkReply* JsonWebTask::doStartNetworkRequest( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) { Q_UNUSED(parentTimeoutMillis); DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); DEBUG_ASSERT(networkAccessManager); - VERIFY_OR_DEBUG_ASSERT(!m_pendingNetworkReply) { - kLogger.warning() - << this - << "Task has already been started"; - return false; - } DEBUG_ASSERT(m_baseUrl.isValid()); QUrl url = m_baseUrl; @@ -265,83 +252,29 @@ bool JsonWebTask::doStart( } DEBUG_ASSERT(url.isValid()); - m_pendingNetworkReply = sendNetworkRequest( + return sendNetworkRequest( networkAccessManager, m_request.method, url, 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); - - return true; } -QUrl JsonWebTask::doAbort() { - DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - QUrl requestUrl; - if (m_pendingNetworkReply) { - requestUrl = abortPendingNetworkReply(m_pendingNetworkReply); - if (requestUrl.isValid()) { - // Already finished - m_pendingNetworkReply->deleteLater(); - m_pendingNetworkReply = nullptr; - } - } - return requestUrl; -} - -QUrl JsonWebTask::doTimeOut() { - DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - QUrl requestUrl; - if (m_pendingNetworkReply) { - requestUrl = timeOutPendingNetworkReply(m_pendingNetworkReply); - // Don't wait until finished - m_pendingNetworkReply->deleteLater(); - m_pendingNetworkReply = nullptr; - } - return requestUrl; -} - -void JsonWebTask::slotNetworkReplyFinished() { - DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - const QPair<QNetworkReply*, HttpStatusCode> networkReplyWithStatusCode = - receiveNetworkReply(); - auto* const networkReply = networkReplyWithStatusCode.first; - if (!networkReply) { - // already aborted - return; - } - const auto statusCode = networkReplyWithStatusCode.second; - VERIFY_OR_DEBUG_ASSERT(networkReply == m_pendingNetworkReply) { - return; - } - m_pendingNetworkReply = nullptr; - +void JsonWebTask::doNetworkReplyFinished( + QNetworkReply* finishedNetworkReply, + HttpStatusCode statusCode) { QJsonDocument content; if (statusCode != kHttpStatusCodeInvalid && - networkReply->bytesAvailable() > 0 && - !readJsonContent(networkReply, &content)) { + finishedNetworkReply->bytesAvailable() > 0 && + !readJsonContent(finishedNetworkReply, &content)) { onFinishedCustom(CustomWebResponse{ WebResponse{ - networkReply->url(), + finishedNetworkReply->url(), statusCode}, - networkReply->readAll()}); + finishedNetworkReply->readAll()}); } else { onFinished(JsonWebResponse{ WebResponse{ - networkReply->url(), + finishedNetworkReply->url(), statusCode}, std::move(content)}); } diff --git a/src/network/jsonwebtask.h b/src/network/jsonwebtask.h index ca83ccea30..544fd97760 100644 --- a/src/network/jsonwebtask.h +++ b/src/network/jsonwebtask.h @@ -57,15 +57,12 @@ class JsonWebTask : public WebTask { const QUrl& baseUrl, JsonWebRequest&& request, QObject* parent = nullptr); - ~JsonWebTask() override; + ~JsonWebTask() override = default; signals: void failed( const network::JsonWebResponse& response); - private slots: - void slotNetworkReplyFinished(); - protected: // Customizable in derived classes virtual QNetworkReply* sendNetworkRequest( @@ -86,18 +83,17 @@ class JsonWebTask : public WebTask { virtual void onFinishedCustom( CustomWebResponse&& response); - bool doStart( + QNetworkReply* doStartNetworkRequest( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) override; - QUrl doAbort() override; - QUrl doTimeOut() override; + void doNetworkReplyFinished( + QNetworkReply* finishedNetworkReply, + HttpStatusCode statusCode) override; // All member variables must only be accessed from // the event loop thread!! const QUrl m_baseUrl; const JsonWebRequest m_request; - - QPointer<QNetworkReply> m_pendingNetworkReply; }; } // namespace network diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index b1481df2d6..cc41984431 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -30,22 +30,19 @@ void registerMetaTypesOnce() { CustomWebResponse::registerMetaType(); } -bool readStatusCode( - const QNetworkReply* reply, - int* statusCode) { - DEBUG_ASSERT(statusCode); - const QVariant statusCodeAttr = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute); +int readStatusCode( + const QNetworkReply* networkReply) { + const QVariant statusCodeAttr = + networkReply->attribute(QNetworkRequest::HttpStatusCodeAttribute); bool statusCodeValid = false; - const int statusCodeValue = statusCodeAttr.toInt(&statusCodeValid); - VERIFY_OR_DEBUG_ASSERT(statusCodeValid && HttpStatusCode_isValid(statusCodeValue)) { + const int statusCode = statusCodeAttr.toInt(&statusCodeValid); + VERIFY_OR_DEBUG_ASSERT(statusCodeValid && HttpStatusCode_isValid(statusCode)) { kLogger.warning() << "Invalid or missing status code attribute" << statusCodeAttr; + return kHttpStatusCodeInvalid; } - else { - *statusCode = statusCodeValue; - } - return statusCodeValid; + return statusCode; } } // anonymous namespace @@ -56,10 +53,10 @@ bool readStatusCode( QDebug operator<<(QDebug dbg, const WebResponse& arg) { return dbg - << "WebResponse{" - << arg.replyUrl - << arg.statusCode - << '}'; + << "WebResponse{" + << arg.replyUrl + << arg.statusCode + << '}'; } /*static*/ void CustomWebResponse::registerMetaType() { @@ -68,10 +65,10 @@ QDebug operator<<(QDebug dbg, const WebResponse& arg) { QDebug operator<<(QDebug dbg, const CustomWebResponse& arg) { return dbg - << "CustomWebResponse{" - << static_cast<const WebResponse&>(arg) - << arg.content - << '}'; + << "CustomWebResponse{" + << static_cast<const WebResponse&>(arg) + << arg.content + << '}'; } WebTask::WebTask( @@ -80,7 +77,7 @@ WebTask::WebTask( : QObject(parent), m_networkAccessManager(networkAccessManager), m_timeoutTimerId(kInvalidTimerId), - m_status(Status::Idle) { + m_state(State::Idle) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); DEBUG_ASSERT(m_networkAccessManager); s_instanceCounter.increment(1); @@ -90,56 +87,28 @@ WebTask::~WebTask() { s_instanceCounter.increment(-1); } -void WebTask::onAborted( - QUrl&& requestUrl) { - 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(); - return; - } - emit aborted( - std::move(requestUrl)); -} - -void WebTask::onTimedOut( - QUrl&& requestUrl) { - 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, - tr("Client-side network timeout"), - QByteArray()); -} - void WebTask::onNetworkError( QUrl&& requestUrl, QNetworkReply::NetworkError errorCode, QString&& errorString, QByteArray&& errorContent) { DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); - VERIFY_OR_DEBUG_ASSERT(m_status == Status::Pending) { + VERIFY_OR_DEBUG_ASSERT(m_state == State::Pending) { return; } + DEBUG_ASSERT(errorCode != QNetworkReply::NoError); - if (errorCode == QNetworkReply::TimeoutError) { - m_status = Status::TimedOut; - } else { - m_status = Status::Failed; + switch (errorCode) { + case QNetworkReply::OperationCanceledError: + m_state = State::Aborted; + break; + case QNetworkReply::TimeoutError: + m_state = State::TimedOut; + break; + default: + m_state = State::Failed; } + VERIFY_OR_DEBUG_ASSERT( isSignalFuncConnected(&WebTask::networkError)) { kLogger.warning() @@ -189,13 +158,14 @@ void WebTask::invokeAbort() { void WebTask::slotStart(int timeoutMillis) { DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - VERIFY_OR_DEBUG_ASSERT(m_status != Status::Pending) { + VERIFY_OR_DEBUG_ASSERT(m_state != State::Pending) { return; } - m_status = Status::Idle; + DEBUG_ASSERT(!m_pendingNetworkReply); + m_state = State::Idle; VERIFY_OR_DEBUG_ASSERT(m_networkAccessManager) { - m_status = Status::Pending; + m_state = State::Pending; onNetworkError( QUrl(), QNetworkReply::NetworkSessionFailedError, @@ -207,73 +177,74 @@ void WebTask::slotStart(int timeoutMillis) { kLogger.debug() << this << "Starting..."; - 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, - tr("Start of network task has been aborted"), - QByteArray()); + + m_pendingNetworkReply = doStartNetworkRequest( + 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_state == State::Idle); + if (!m_pendingNetworkReply) { + kLogger.debug() + << "Network task has not been started"; return; } - // Still idle after the request has been started - // successfully, i.e. nothing happened yet in this - // thread. - DEBUG_ASSERT(m_status == Status::Idle); - m_status = Status::Pending; + m_state = State::Pending; DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); if (timeoutMillis > 0) { m_timeoutTimerId = startTimer(timeoutMillis); DEBUG_ASSERT(m_timeoutTimerId != kInvalidTimerId); } -} -QUrl WebTask::abortPendingNetworkReply( - QNetworkReply* pendingNetworkReply) { - DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - DEBUG_ASSERT(pendingNetworkReply); - if (pendingNetworkReply->isRunning()) { - pendingNetworkReply->abort(); - // Suspend until finished - return QUrl(); - } - return pendingNetworkReply->request().url(); + // 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, + &WebTask::slotNetworkReplyFinished, + Qt::UniqueConnection); } -QUrl WebTask::timeOutPendingNetworkReply( - QNetworkReply* pendingNetworkReply) { +void WebTask::abort() { DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - DEBUG_ASSERT(pendingNetworkReply); - if (pendingNetworkReply->isRunning()) { - //pendingNetworkReply->abort(); - // Don't suspend until finished, i.e. abort and then - // delete the pending network request instantly + if (m_state != State::Pending) { + DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); + return; } - return pendingNetworkReply->request().url(); -} - -QUrl WebTask::abort() { - DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - if (m_status != Status::Pending) { + VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply) { + return; DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); - return QUrl(); } if (m_timeoutTimerId != kInvalidTimerId) { killTimer(m_timeoutTimerId); m_timeoutTimerId = kInvalidTimerId; } - m_status = Status::Aborting; + m_state = State::Aborting; kLogger.debug() << this << "Aborting..."; - QUrl url = doAbort(); - onAborted(QUrl(url)); - return url; + if (m_pendingNetworkReply->isRunning()) { + m_pendingNetworkReply->abort(); + doNetworkReplyAborted(m_pendingNetworkReply); + // Suspend and await finished signal + return; + } + doNetworkReplyAborted(m_pendingNetworkReply); + m_state = State::Aborted; + const auto requestUrl = m_pendingNetworkReply->request().url(); + VERIFY_OR_DEBUG_ASSERT( + isSignalFuncConnected(&WebTask::aborted)) { + kLogger.warning() + << this + << "Unhandled abort signal" + << requestUrl; + deleteLater(); + return; + } + emit aborted( + std::move(requestUrl)); } void WebTask::slotAbort() { @@ -289,54 +260,67 @@ void WebTask::timerEvent(QTimerEvent* event) { } killTimer(m_timeoutTimerId); m_timeoutTimerId = kInvalidTimerId; - VERIFY_OR_DEBUG_ASSERT(m_status == Status::Pending) { + VERIFY_OR_DEBUG_ASSERT(m_state == State::Pending) { + return; + } + VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply) { return; } + if (m_pendingNetworkReply->isFinished()) { + // Nothing to do + } kLogger.info() << this - << "Timed out"; - onTimedOut(doTimeOut()); + << "Aborting after timed out"; + DEBUG_ASSERT(m_pendingNetworkReply->isRunning()); + m_pendingNetworkReply->abort(); + // Aborting the network reply might finish it + // immediately. It will be destroyed when handling + // the finished signal, i.e. m_pendingNetworkReply + // could be nullptr here! } -QPair<QNetworkReply*, HttpStatusCode> WebTask::receiveNetworkReply() { +void WebTask::slotNetworkReplyFinished() { DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - DEBUG_ASSERT(m_status != Status::Idle); - auto* const networkReply = qobject_cast<QNetworkReply*>(sender()); - HttpStatusCode statusCode = kHttpStatusCodeInvalid; - VERIFY_OR_DEBUG_ASSERT(networkReply) { - return qMakePair(nullptr, statusCode); - } - networkReply->deleteLater(); + auto* const finishedNetworkReply = qobject_cast<QNetworkReply*>(sender()); + VERIFY_OR_DEBUG_ASSERT(finishedNetworkReply) { + return; + } + finishedNetworkReply->deleteLater(); if (kLogger.debugEnabled()) { - if (networkReply->url() == networkReply->request().url()) { + if (finishedNetworkReply->url() == finishedNetworkReply->request().url()) { kLogger.debug() << this << "Received reply for request" - << networkReply->url(); + << finishedNetworkReply->url(); } else { // Redirected kLogger.debug() << this << "Received reply for redirected request" - << networkReply->request().url() + << finishedNetworkReply->request().url() << "->" - << networkReply->url(); + << finishedNetworkReply->url(); } } - if (m_status == Status::Aborted || - m_status == Status::TimedOut) { - // Already aborted or timed out by the client + if (!m_pendingNetworkReply) { + DEBUG_ASSERT(m_state == State::Aborted || m_state == State::TimedOut); DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); kLogger.debug() << this << "Ignoring obsolete network reply"; - return qMakePair(nullptr, statusCode); + return; } - VERIFY_OR_DEBUG_ASSERT(m_status == Status::Pending) { + VERIFY_OR_DEBUG_ASSERT(m_pendingNetworkReply == finishedNetworkReply) { + return; + } + m_pendingNetworkReply = nullptr; + + VERIFY_OR_DEBUG_ASSERT(m_state == State::Pending) { DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); - return qMakePair(nullptr, statusCode); + return; } if (m_timeoutTimerId != kInvalidTimerId) { @@ -344,24 +328,18 @@ QPair<QNetworkReply*, HttpStatusCode> WebTask::receiveNetworkReply() { m_timeoutTimerId = kInvalidTimerId; } - if (networkReply->error() != QNetworkReply::NetworkError::NoError) { + if (finishedNetworkReply->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"; + finishedNetworkReply->request().url(), + finishedNetworkReply->error(), + finishedNetworkReply->errorString(), + finishedNetworkReply->readAll()); + return; } - return qMakePair(networkReply, statusCode); + m_state = State::Finished; + const auto statusCode = readStatusCode(finishedNetworkReply); + doNetworkReplyFinished(finishedNetworkReply, statusCode); } } // namespace network diff --git a/src/network/webtask.h b/src/network/webtask.h index 2078e77d0c..48a3942ebc 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -3,11 +3,11 @@ #include <QMetaMethod> #include <QNetworkAccessManager> #include <QNetworkReply> -#include <QPair> #include <QPointer> #include <QUrl> #include "network/httpstatuscode.h" +#include "util/optional.h" namespace mixxx { @@ -113,13 +113,16 @@ class WebTask : public QObject { void invokeAbort(); /// Cancel a pending request from the event loop thread. - QUrl abort(); + void abort(); public slots: void slotStart( int timeoutMillis); void slotAbort(); + private slots: + void slotNetworkReplyFinished(); + signals: /// The receiver is responsible for deleting the task in the /// corresponding slot handler!! Otherwise the task will remain @@ -144,7 +147,7 @@ class WebTask : public QObject { void timerEvent(QTimerEvent* event) final; - enum class Status { + enum class State { Idle, Pending, Aborting, @@ -153,21 +156,9 @@ class WebTask : public QObject { Failed, Finished, }; - - QUrl abortPendingNetworkReply( - QNetworkReply* pendingNetworkReply); - QUrl timeOutPendingNetworkReply( - QNetworkReply* pendingNetworkReply); - - QPair<QNetworkReply*, HttpStatusCode> receiveNetworkReply(); - - /// Handle status changes and ensure that the task eventually - /// gets deleted. The default implementations emit a signal - /// if connected or otherwise implicitly delete the task. - virtual void onAborted( - QUrl&& requestUrl); - virtual void onTimedOut( - QUrl&& requestUrl); + State state() const { + return m_state; + } /// Handle the abort and ensure that the task eventually /// gets deleted. The default implementation logs a warning @@ -179,26 +170,33 @@ class WebTask : public QObject { QByteArray&& errorContent); private: - /// Try to compose and send the actual network request. If - /// true is returned than the network request is running - /// and a reply is pending. - virtual bool doStart( + QUrl abortPendingNetworkReply(); + + /// Try to compose and send the actual network request. + /// Return nullptr on failure. + virtual QNetworkReply* doStartNetworkRequest( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) = 0; - /// Handle status change requests by aborting a running request - /// and return the request URL. If no request is running or if - /// the request has already been finished the QUrl() must be - /// returned. - virtual QUrl doAbort() = 0; - virtual QUrl doTimeOut() = 0; + /// Optional: Do something after aborted. + virtual void doNetworkReplyAborted( + QNetworkReply* abortedNetworkReply) { + Q_UNUSED(abortedNetworkReply); + } + + /// Handle network response. + virtual void doNetworkReplyFinished( + QNetworkReply* finishedNetworkReply, + HttpStatusCode statusCode) = 0; /// All member variables must only be accessed from /// the event loop thread!! const QPointer<QNetworkAccessManager> m_networkAccessManager; int m_timeoutTimerId; - Status m_status; + State m_state; + + QPointer<QNetworkReply> m_pendingNetworkReply; }; } // namespace network |