diff options
author | Uwe Klotz <uklotz@mixxx.org> | 2020-12-11 14:28:49 +0100 |
---|---|---|
committer | Uwe Klotz <uklotz@mixxx.org> | 2020-12-11 15:33:39 +0100 |
commit | 61b8fb005edcbb13502a6c8f3f747f4f53b4512a (patch) | |
tree | 01406b3c3f1d1037cffc5d012382e3e707fcfb80 /src | |
parent | 0512146d70a40971921a3cb59ce6783dde485550 (diff) |
Detect and prevent duplicate signals on network errors
Diffstat (limited to 'src')
-rw-r--r-- | src/musicbrainz/web/musicbrainzrecordingstask.cpp | 13 | ||||
-rw-r--r-- | src/network/jsonwebtask.cpp | 13 | ||||
-rw-r--r-- | src/network/webtask.cpp | 92 | ||||
-rw-r--r-- | src/network/webtask.h | 2 |
4 files changed, 70 insertions, 50 deletions
diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp index fd191c996d..0cad9148db 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp +++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp @@ -118,22 +118,15 @@ bool MusicBrainzRecordingsTask::doStart( return false; } + // It is not necessary to connect the QNetworkReply::errorOccurred + // signal (since Qt 5.15). Network errors are also received trough + // 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..0782b51cc7 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -268,22 +268,15 @@ bool JsonWebTask::doStart( return false; } + // It is not necessary to connect the QNetworkReply::errorOccurred + // signal (since Qt 5.15). Network errors are also received trough + // 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; } diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index 534c582b22..25d256a043 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -92,7 +92,10 @@ 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() @@ -107,7 +110,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 +129,20 @@ 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" + << "Unhandled network error:" << requestUrl << errorCode << errorString @@ -168,8 +187,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, @@ -180,12 +204,12 @@ void WebTask::slotStart(int timeoutMillis) { kLogger.debug() << "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,7 +264,7 @@ QUrl WebTask::abort() { killTimer(m_timeoutTimerId); m_timeoutTimerId = kInvalidTimerId; } - m_status = Status::Aborted; + m_status = Status::Aborting; kLogger.debug() << "Aborting..."; QUrl url = doAbort(); @@ -256,14 +280,13 @@ 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() << "Timed out"; @@ -280,26 +303,6 @@ 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() @@ -315,6 +318,35 @@ 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() 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, }; |