From c234d886e40555b0c0bef45ac3eb80f874f65813 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 7 Mar 2020 09:53:35 +0100 Subject: [PATCH 01/20] Allow restarting of web tasks --- src/network/webtask.cpp | 24 +++++++++++++++--------- src/network/webtask.h | 3 ++- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index c79b5438429..4f97bcc1048 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -163,11 +163,9 @@ void WebTask::slotStart(int timeoutMillis) { << "No network access"; return; } - VERIFY_OR_DEBUG_ASSERT(!m_abort) { - kLogger.warning() - << "Task has already been aborted"; - return; - } + // The task might be restarted after it has been aborted + // or finished. + m_abort = false; kLogger.debug() << "Starting..."; @@ -176,13 +174,13 @@ void WebTask::slotStart(int timeoutMillis) { << "Start aborted"; return; } - // Task can only be started once - m_networkAccessManager = nullptr; if (m_abort) { onAborted(); return; } + + DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); if (timeoutMillis > 0) { m_timeoutTimerId = startTimer(timeoutMillis); DEBUG_ASSERT(m_timeoutTimerId != kInvalidTimerId); @@ -192,8 +190,13 @@ void WebTask::slotStart(int timeoutMillis) { void WebTask::slotAbort() { DEBUG_ASSERT(thread() == QThread::currentThread()); if (m_abort) { + DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); return; } + if (m_timeoutTimerId != kInvalidTimerId) { + killTimer(m_timeoutTimerId); + m_timeoutTimerId = kInvalidTimerId; + } m_abort = true; kLogger.debug() << "Aborting..."; @@ -208,8 +211,6 @@ void WebTask::timerEvent(QTimerEvent* event) { // ignore return; } - killTimer(timerId); - m_timeoutTimerId = kInvalidTimerId; kLogger.info() << "Timed out"; slotAbort(); @@ -224,6 +225,11 @@ QPair WebTask::receiveNetworkReply() { } networkReply->deleteLater(); + if (m_timeoutTimerId != kInvalidTimerId) { + killTimer(m_timeoutTimerId); + m_timeoutTimerId = kInvalidTimerId; + } + if (m_abort) { onAborted(); return qMakePair(nullptr, statusCode); diff --git a/src/network/webtask.h b/src/network/webtask.h index 42c448a0ff7..03abcc603fd 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -151,7 +151,8 @@ class WebTask : public QObject { // All member variables must only be accessed from // the event loop thread!! - QPointer m_networkAccessManager; + const QPointer m_networkAccessManager; + int m_timeoutTimerId; bool m_abort; }; From 0fff63c68b5f015d2ae2ce88eee2fe0c4a0f8afd Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 9 Mar 2020 09:33:16 +0100 Subject: [PATCH 02/20] Add comment about abort while being started --- src/network/webtask.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index 4f97bcc1048..c1328ab3989 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -175,6 +175,7 @@ void WebTask::slotStart(int timeoutMillis) { return; } + // The task could be aborted immediately while being started. if (m_abort) { onAborted(); return; From c28fcd654c546584a5f5934d1142d43357c27f46 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Mar 2020 17:17:48 +0100 Subject: [PATCH 03/20] Prevent web network tasks from becoming parented --- src/musicbrainz/tagfetcher.cpp | 26 +++++++++---------- src/musicbrainz/tagfetcher.h | 6 ++--- src/musicbrainz/web/acoustidlookuptask.cpp | 6 ++--- src/musicbrainz/web/acoustidlookuptask.h | 3 +-- .../web/musicbrainzrecordingstask.cpp | 6 ++--- .../web/musicbrainzrecordingstask.h | 3 +-- src/network/jsonwebtask.cpp | 5 ++-- src/network/jsonwebtask.h | 3 +-- src/network/webtask.cpp | 6 ++--- src/network/webtask.h | 15 +++++++++-- 10 files changed, 39 insertions(+), 40 deletions(-) diff --git a/src/musicbrainz/tagfetcher.cpp b/src/musicbrainz/tagfetcher.cpp index 093c940dfdd..18e3ef75ccf 100644 --- a/src/musicbrainz/tagfetcher.cpp +++ b/src/musicbrainz/tagfetcher.cpp @@ -38,7 +38,7 @@ void TagFetcher::startFetch( void TagFetcher::abortAcoustIdTask() { if (m_pAcoustIdTask) { - disconnect(m_pAcoustIdTask.get()); + disconnect(m_pAcoustIdTask); m_pAcoustIdTask->deleteBeforeFinished(); m_pAcoustIdTask = nullptr; } @@ -46,7 +46,7 @@ void TagFetcher::abortAcoustIdTask() { void TagFetcher::abortMusicBrainzTask() { if (m_pMusicBrainzTask) { - disconnect(m_pMusicBrainzTask.get()); + disconnect(m_pMusicBrainzTask); m_pMusicBrainzTask->deleteBeforeFinished(); m_pMusicBrainzTask = nullptr; } @@ -78,20 +78,19 @@ void TagFetcher::slotFingerprintReady() { emit fetchProgress(tr("Identifying track through Acoustid")); DEBUG_ASSERT(!m_pAcoustIdTask); - m_pAcoustIdTask = make_parented( + m_pAcoustIdTask = new mixxx::AcoustIdLookupTask( &m_network, fingerprint, - m_pTrack->getDurationInt(), - this); - connect(m_pAcoustIdTask.get(), + m_pTrack->getDurationInt()); + connect(m_pAcoustIdTask, &mixxx::AcoustIdLookupTask::succeeded, this, &TagFetcher::slotAcoustIdTaskSucceeded); - connect(m_pAcoustIdTask.get(), + connect(m_pAcoustIdTask, &mixxx::AcoustIdLookupTask::failed, this, &TagFetcher::slotAcoustIdTaskFailed); - connect(m_pAcoustIdTask.get(), + connect(m_pAcoustIdTask, &mixxx::AcoustIdLookupTask::networkError, this, &TagFetcher::slotAcoustIdTaskNetworkError); @@ -115,19 +114,18 @@ void TagFetcher::slotAcoustIdTaskSucceeded( emit fetchProgress(tr("Retrieving metadata from MusicBrainz")); DEBUG_ASSERT(!m_pMusicBrainzTask); - m_pMusicBrainzTask = make_parented( + m_pMusicBrainzTask = new mixxx::MusicBrainzRecordingsTask( &m_network, - std::move(recordingIds), - this); - connect(m_pMusicBrainzTask.get(), + std::move(recordingIds)); + connect(m_pMusicBrainzTask, &mixxx::MusicBrainzRecordingsTask::succeeded, this, &TagFetcher::slotMusicBrainzTaskSucceeded); - connect(m_pMusicBrainzTask.get(), + connect(m_pMusicBrainzTask, &mixxx::MusicBrainzRecordingsTask::failed, this, &TagFetcher::slotMusicBrainzTaskFailed); - connect(m_pMusicBrainzTask.get(), + connect(m_pMusicBrainzTask, &mixxx::MusicBrainzRecordingsTask::networkError, this, &TagFetcher::slotMusicBrainzTaskNetworkError); diff --git a/src/musicbrainz/tagfetcher.h b/src/musicbrainz/tagfetcher.h index d0f14ad1edc..e10e184d237 100644 --- a/src/musicbrainz/tagfetcher.h +++ b/src/musicbrainz/tagfetcher.h @@ -2,11 +2,11 @@ #include #include +#include #include "musicbrainz/web/acoustidlookuptask.h" #include "musicbrainz/web/musicbrainzrecordingstask.h" #include "track/track.h" -#include "util/parented_ptr.h" class TagFetcher : public QObject { Q_OBJECT @@ -71,9 +71,9 @@ class TagFetcher : public QObject { QFutureWatcher m_fingerprintWatcher; - parented_ptr m_pAcoustIdTask; + QPointer m_pAcoustIdTask; - parented_ptr m_pMusicBrainzTask; + QPointer m_pMusicBrainzTask; TrackPointer m_pTrack; }; diff --git a/src/musicbrainz/web/acoustidlookuptask.cpp b/src/musicbrainz/web/acoustidlookuptask.cpp index 57a11fe3aa2..6f8a2acdfd7 100644 --- a/src/musicbrainz/web/acoustidlookuptask.cpp +++ b/src/musicbrainz/web/acoustidlookuptask.cpp @@ -67,13 +67,11 @@ network::JsonWebRequest lookupRequest() { AcoustIdLookupTask::AcoustIdLookupTask( QNetworkAccessManager* networkAccessManager, const QString& fingerprint, - int duration, - QObject* parent) + int duration) : network::JsonWebTask( networkAccessManager, kBaseUrl, - lookupRequest(), - parent), + lookupRequest()), m_urlQuery(lookupUrlQuery(fingerprint, duration)) { } diff --git a/src/musicbrainz/web/acoustidlookuptask.h b/src/musicbrainz/web/acoustidlookuptask.h index e8d0a2cce67..34dc4574916 100644 --- a/src/musicbrainz/web/acoustidlookuptask.h +++ b/src/musicbrainz/web/acoustidlookuptask.h @@ -15,8 +15,7 @@ class AcoustIdLookupTask : public network::JsonWebTask { AcoustIdLookupTask( QNetworkAccessManager* networkAccessManager, const QString& fingerprint, - int duration, - QObject* parent = nullptr); + int duration); ~AcoustIdLookupTask() override = default; signals: diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp index 7c6d1d7df1b..ff70cfe4bfd 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp +++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp @@ -65,11 +65,9 @@ QNetworkRequest createNetworkRequest( MusicBrainzRecordingsTask::MusicBrainzRecordingsTask( QNetworkAccessManager* networkAccessManager, - QList&& recordingIds, - QObject* parent) + QList&& recordingIds) : network::WebTask( - networkAccessManager, - parent), + networkAccessManager), m_queuedRecordingIds(std::move(recordingIds)), m_parentTimeoutMillis(0) { musicbrainz::registerMetaTypesOnce(); diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.h b/src/musicbrainz/web/musicbrainzrecordingstask.h index a81f8923050..fab025596c9 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.h +++ b/src/musicbrainz/web/musicbrainzrecordingstask.h @@ -17,8 +17,7 @@ class MusicBrainzRecordingsTask : public network::WebTask { public: MusicBrainzRecordingsTask( QNetworkAccessManager* networkAccessManager, - QList&& recordingIds, - QObject* parent = nullptr); + QList&& recordingIds); ~MusicBrainzRecordingsTask() override = default; signals: diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index 08530f66494..57a8aa195a6 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -87,9 +87,8 @@ QNetworkRequest newRequest( JsonWebTask::JsonWebTask( QNetworkAccessManager* networkAccessManager, QUrl baseUrl, - JsonWebRequest request, - QObject* parent) - : WebTask(networkAccessManager, parent), + JsonWebRequest request) + : WebTask(networkAccessManager), m_baseUrl(std::move(baseUrl)), m_request(std::move(request)), m_pendingNetworkReply(nullptr) { diff --git a/src/network/jsonwebtask.h b/src/network/jsonwebtask.h index 1a7fb0927aa..b7d8a791383 100644 --- a/src/network/jsonwebtask.h +++ b/src/network/jsonwebtask.h @@ -53,8 +53,7 @@ class JsonWebTask : public WebTask { JsonWebTask( QNetworkAccessManager* networkAccessManager, QUrl baseUrl, - JsonWebRequest request, - QObject* parent = nullptr); + JsonWebRequest request); ~JsonWebTask() override; signals: diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index c1328ab3989..0cbf129c46f 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -59,10 +59,8 @@ bool readStatusCode( } WebTask::WebTask( - QNetworkAccessManager* networkAccessManager, - QObject* parent) - : QObject(parent), - m_networkAccessManager(networkAccessManager), + QNetworkAccessManager* networkAccessManager) + : m_networkAccessManager(networkAccessManager), m_timeoutTimerId(kInvalidTimerId), m_abort(false) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); diff --git a/src/network/webtask.h b/src/network/webtask.h index 03abcc603fd..358f4fed57f 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -81,13 +81,24 @@ struct CustomWebResponse : public WebResponse { QByteArray content; }; +// A transient task for performing a single HTTP network request +// asynchronously. +// +// The results are transmitted by emitting signals. Only a single +// receiver can be connected to each signal by using Qt::UniqueConnection. +// The receiver of the signal is responsible for destroying the task +// by invoking QObject::deleteLater(). If no receiver is connected to +// a signal the task will destroy itself. +// +// Instances of this class must not be parented due to their built-in +// self-destruction mechanism. All pointers to tasks should be wrapped +// into QPointer. Otherwise plain pointers might become dangling! class WebTask : public QObject { Q_OBJECT public: explicit WebTask( - QNetworkAccessManager* networkAccessManager, - QObject* parent = nullptr); + QNetworkAccessManager* networkAccessManager); ~WebTask() override; // timeoutMillis <= 0: No timeout (unlimited) From f9264c3f4921064ddd4823048bacfd6589b803e3 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Mar 2020 23:21:55 +0100 Subject: [PATCH 04/20] Handle QNetworkReply::error signals --- src/musicbrainz/web/musicbrainzrecordingstask.cpp | 6 ++++++ src/network/jsonwebtask.cpp | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp index ff70cfe4bfd..cf524d918cc 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp +++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp @@ -115,6 +115,12 @@ bool MusicBrainzRecordingsTask::doStart( &MusicBrainzRecordingsTask::slotNetworkReplyFinished, Qt::UniqueConnection); + connect(m_pendingNetworkReply, + QOverload::of(&QNetworkReply::error), + this, + &MusicBrainzRecordingsTask::slotNetworkReplyFinished, + Qt::UniqueConnection); + return true; } diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index 57a8aa195a6..ead3d296966 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -236,6 +236,12 @@ bool JsonWebTask::doStart( &JsonWebTask::slotNetworkReplyFinished, Qt::UniqueConnection); + connect(m_pendingNetworkReply, + QOverload::of(&QNetworkReply::error), + this, + &JsonWebTask::slotNetworkReplyFinished, + Qt::UniqueConnection); + return true; } From 785e7112ad450658905b18d22bbd5a19a48d6a0f Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Mar 2020 23:28:59 +0100 Subject: [PATCH 05/20] Fix disconnect/connect when loading new tracks for lookup --- src/library/dlgtagfetcher.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/library/dlgtagfetcher.cpp b/src/library/dlgtagfetcher.cpp index 0b6b682064a..629ecf68fd7 100644 --- a/src/library/dlgtagfetcher.cpp +++ b/src/library/dlgtagfetcher.cpp @@ -84,11 +84,11 @@ void DlgTagFetcher::init() { } void DlgTagFetcher::loadTrack(const TrackPointer& track) { - if (track == NULL) { + if (!track) { return; } results->clear(); - disconnect(track.get(), + disconnect(m_track.get(), &Track::changed, this, &DlgTagFetcher::slotTrackChanged); @@ -96,13 +96,14 @@ void DlgTagFetcher::loadTrack(const TrackPointer& track) { m_track = track; m_data = Data(); m_networkResult = NetworkResult::Ok; - m_tagFetcher.startFetch(m_track); - connect(track.get(), + connect(m_track.get(), &Track::changed, this, &DlgTagFetcher::slotTrackChanged); + m_tagFetcher.startFetch(m_track); + updateStack(); } From 6341398788beb2dc561f8ee1da5b749cb420ded3 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Mar 2020 23:29:52 +0100 Subject: [PATCH 06/20] Handle abort signals of AcoustId and MusicBrainz tasks --- src/musicbrainz/tagfetcher.cpp | 16 ++++++++++++++++ src/musicbrainz/tagfetcher.h | 2 ++ 2 files changed, 18 insertions(+) diff --git a/src/musicbrainz/tagfetcher.cpp b/src/musicbrainz/tagfetcher.cpp index 18e3ef75ccf..1435aa2fa50 100644 --- a/src/musicbrainz/tagfetcher.cpp +++ b/src/musicbrainz/tagfetcher.cpp @@ -90,6 +90,10 @@ void TagFetcher::slotFingerprintReady() { &mixxx::AcoustIdLookupTask::failed, this, &TagFetcher::slotAcoustIdTaskFailed); + connect(m_pAcoustIdTask, + &mixxx::AcoustIdLookupTask::aborted, + this, + &TagFetcher::slotAcoustIdTaskAborted); connect(m_pAcoustIdTask, &mixxx::AcoustIdLookupTask::networkError, this, @@ -125,6 +129,10 @@ void TagFetcher::slotAcoustIdTaskSucceeded( &mixxx::MusicBrainzRecordingsTask::failed, this, &TagFetcher::slotMusicBrainzTaskFailed); + connect(m_pMusicBrainzTask, + &mixxx::MusicBrainzRecordingsTask::aborted, + this, + &TagFetcher::slotMusicBrainzTaskAborted); connect(m_pMusicBrainzTask, &mixxx::MusicBrainzRecordingsTask::networkError, this, @@ -143,6 +151,10 @@ void TagFetcher::slotAcoustIdTaskFailed( -1); } +void TagFetcher::slotAcoustIdTaskAborted() { + abortAcoustIdTask(); +} + void TagFetcher::slotAcoustIdTaskNetworkError( QUrl requestUrl, QNetworkReply::NetworkError errorCode, @@ -158,6 +170,10 @@ void TagFetcher::slotAcoustIdTaskNetworkError( errorCode); } +void TagFetcher::slotMusicBrainzTaskAborted() { + abortMusicBrainzTask(); +} + void TagFetcher::slotMusicBrainzTaskNetworkError( QUrl requestUrl, QNetworkReply::NetworkError errorCode, diff --git a/src/musicbrainz/tagfetcher.h b/src/musicbrainz/tagfetcher.h index e10e184d237..622cebe11cf 100644 --- a/src/musicbrainz/tagfetcher.h +++ b/src/musicbrainz/tagfetcher.h @@ -45,6 +45,7 @@ class TagFetcher : public QObject { QList recordingIds); void slotAcoustIdTaskFailed( mixxx::network::JsonWebResponse response); + void slotAcoustIdTaskAborted(); void slotAcoustIdTaskNetworkError( QUrl requestUrl, QNetworkReply::NetworkError errorCode, @@ -57,6 +58,7 @@ class TagFetcher : public QObject { mixxx::network::WebResponse response, int errorCode, QString errorMessage); + void slotMusicBrainzTaskAborted(); void slotMusicBrainzTaskNetworkError( QUrl requestUrl, QNetworkReply::NetworkError errorCode, From 303c0bc75cd1d5cb14e2a24d0c0bcc5d0b9ba16a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Mar 2020 23:30:34 +0100 Subject: [PATCH 07/20] Signal failure to parse XML reponse --- src/musicbrainz/web/musicbrainzrecordingstask.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp index cf524d918cc..4c4f7a7a4f8 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp +++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp @@ -179,7 +179,12 @@ void MusicBrainzRecordingsTask::slotNetworkReplyFinished() { if (!recordingsResult.second) { kLogger.warning() << "Failed to parse XML response"; - slotAbort(); + emitFailed( + network::WebResponse( + networkReply->url(), + statusCode), + -1, + "Failed to parse XML response"); return; } From db450a11e37b6f48e0f0fab64839f6b15cefda92 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Mar 2020 09:53:27 +0100 Subject: [PATCH 08/20] Yet another backport for Xenial --- src/network/jsonwebtask.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index ead3d296966..af9b084c0aa 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -11,6 +11,9 @@ #include // std::once_flag #include "util/assert.h" +#if QT_VERSION < QT_VERSION_CHECK(5, 7, 0) +#include "util/compatibility.h" +#endif #include "util/counter.h" #include "util/logger.h" @@ -237,7 +240,11 @@ bool JsonWebTask::doStart( Qt::UniqueConnection); connect(m_pendingNetworkReply, +#if QT_VERSION >= QT_VERSION_CHECK(5, 7, 0) + qOverload(&QNetworkReply::error), +#else QOverload::of(&QNetworkReply::error), +#endif this, &JsonWebTask::slotNetworkReplyFinished, Qt::UniqueConnection); From 38f625de0034c3662805395c66d10544946fbf88 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Mar 2020 13:07:36 +0100 Subject: [PATCH 09/20] Handle client-side timeouts --- .../web/musicbrainzrecordingstask.cpp | 23 ++++- .../web/musicbrainzrecordingstask.h | 3 +- src/network/jsonwebtask.cpp | 24 +++++- src/network/jsonwebtask.h | 3 +- src/network/webtask.cpp | 85 +++++++++++++++---- src/network/webtask.h | 28 ++++-- 6 files changed, 139 insertions(+), 27 deletions(-) diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp index 4c4f7a7a4f8..cccdaf673c5 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp +++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp @@ -124,10 +124,29 @@ bool MusicBrainzRecordingsTask::doStart( return true; } -void MusicBrainzRecordingsTask::doAbort() { +QUrl MusicBrainzRecordingsTask::doAbort() { + QUrl requestUrl; if (m_pendingNetworkReply) { - m_pendingNetworkReply->abort(); + requestUrl = abortPendingNetworkReply(m_pendingNetworkReply); + if (requestUrl.isValid()) { + // Already finished + m_pendingNetworkReply->deleteLater(); + m_pendingNetworkReply = nullptr; + } } + return requestUrl; +} + +QUrl MusicBrainzRecordingsTask::doTimeOut() { + DEBUG_ASSERT(thread() == QThread::currentThread()); + 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() { diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.h b/src/musicbrainz/web/musicbrainzrecordingstask.h index fab025596c9..614315d7584 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.h +++ b/src/musicbrainz/web/musicbrainzrecordingstask.h @@ -35,7 +35,8 @@ class MusicBrainzRecordingsTask : public network::WebTask { bool doStart( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) override; - void doAbort() override; + QUrl doAbort() override; + QUrl doTimeOut() override; void emitSucceeded( QList&& trackReleases); diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index af9b084c0aa..8ccdcffddcb 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -252,10 +252,30 @@ bool JsonWebTask::doStart( return true; } -void JsonWebTask::doAbort() { +QUrl JsonWebTask::doAbort() { + DEBUG_ASSERT(thread() == QThread::currentThread()); + 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(thread() == QThread::currentThread()); + QUrl requestUrl; if (m_pendingNetworkReply) { - m_pendingNetworkReply->abort(); + requestUrl = timeOutPendingNetworkReply(m_pendingNetworkReply); + // Don't wait until finished + m_pendingNetworkReply->deleteLater(); + m_pendingNetworkReply = nullptr; } + return requestUrl; } void JsonWebTask::slotNetworkReplyFinished() { diff --git a/src/network/jsonwebtask.h b/src/network/jsonwebtask.h index b7d8a791383..879feb1cf6f 100644 --- a/src/network/jsonwebtask.h +++ b/src/network/jsonwebtask.h @@ -86,7 +86,8 @@ class JsonWebTask : public WebTask { bool doStart( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) override; - void doAbort() override; + QUrl doAbort() override; + QUrl doTimeOut() override; // All member variables must only be accessed from // the event loop thread!! diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index 0cbf129c46f..eb2e43cf6a3 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -62,7 +62,7 @@ WebTask::WebTask( QNetworkAccessManager* networkAccessManager) : m_networkAccessManager(networkAccessManager), m_timeoutTimerId(kInvalidTimerId), - m_abort(false) { + m_status(Status::Idle) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); DEBUG_ASSERT(m_networkAccessManager); s_instanceCounter.increment(1); @@ -72,15 +72,38 @@ WebTask::~WebTask() { s_instanceCounter.increment(-1); } -void WebTask::onAborted() { - DEBUG_ASSERT(m_abort); +void WebTask::onAborted( + QUrl requestUrl) { + DEBUG_ASSERT(m_status == Status::Aborted); const auto signal = QMetaMethod::fromSignal( &WebTask::aborted); if (isSignalConnected(signal)) { emit aborted(); } else { kLogger.info() - << "Request aborted"; + << "Request aborted" + << requestUrl; + deleteAfterFinished(); + } +} + +void WebTask::onTimedOut( + QUrl requestUrl) { + DEBUG_ASSERT(m_status == Status::TimedOut); + const auto signal = QMetaMethod::fromSignal( + &WebTask::networkError); + if (isSignalConnected(signal)) { + emit networkError( + std::move(requestUrl), + QNetworkReply::TimeoutError, + QStringLiteral("Client-side timeout"), + QByteArray()); + } else { + kLogger.warning() + << "Aborting request after client-side timeout" + << requestUrl; + // Do not abort the request before deleting it, + // i.e. pretend it already has been finished. deleteAfterFinished(); } } @@ -156,14 +179,12 @@ void WebTask::deleteAfterFinished() { void WebTask::slotStart(int timeoutMillis) { DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_status == Status::Idle); VERIFY_OR_DEBUG_ASSERT(m_networkAccessManager) { kLogger.warning() << "No network access"; return; } - // The task might be restarted after it has been aborted - // or finished. - m_abort = false; kLogger.debug() << "Starting..."; @@ -174,10 +195,11 @@ void WebTask::slotStart(int timeoutMillis) { } // The task could be aborted immediately while being started. - if (m_abort) { - onAborted(); + if (m_status == Status::Aborted) { + onAborted(doAbort()); return; } + m_status = Status::Pending; DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); if (timeoutMillis > 0) { @@ -186,9 +208,33 @@ void WebTask::slotStart(int timeoutMillis) { } } +QUrl WebTask::abortPendingNetworkReply( + QNetworkReply* pendingNetworkReply) { + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(pendingNetworkReply); + if (pendingNetworkReply->isRunning()) { + pendingNetworkReply->abort(); + // Suspend until finished + return QUrl(); + } + return pendingNetworkReply->request().url(); +} + +QUrl WebTask::timeOutPendingNetworkReply( + QNetworkReply* pendingNetworkReply) { + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(pendingNetworkReply); + if (pendingNetworkReply->isRunning()) { + //pendingNetworkReply->abort(); + // Don't suspend until finished, i.e. abort and then + // delete the pending network request instantly + } + return pendingNetworkReply->request().url(); +} + void WebTask::slotAbort() { DEBUG_ASSERT(thread() == QThread::currentThread()); - if (m_abort) { + if (m_status != Status::Pending) { DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); return; } @@ -196,10 +242,10 @@ void WebTask::slotAbort() { killTimer(m_timeoutTimerId); m_timeoutTimerId = kInvalidTimerId; } - m_abort = true; + m_status = Status::Aborted; kLogger.debug() << "Aborting..."; - doAbort(); + onAborted(doAbort()); } void WebTask::timerEvent(QTimerEvent* event) { @@ -210,13 +256,19 @@ void WebTask::timerEvent(QTimerEvent* event) { // ignore return; } - kLogger.info() + killTimer(m_timeoutTimerId); + m_timeoutTimerId = kInvalidTimerId; + if (m_status != Status::Aborted) { + m_status = Status::TimedOut; + } + kLogger.debug() << "Timed out"; - slotAbort(); + onTimedOut(doTimeOut()); } QPair WebTask::receiveNetworkReply() { DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_status != Status::Idle); auto* const networkReply = qobject_cast(sender()); HttpStatusCode statusCode = kHttpStatusCodeInvalid; VERIFY_OR_DEBUG_ASSERT(networkReply) { @@ -229,10 +281,11 @@ QPair WebTask::receiveNetworkReply() { m_timeoutTimerId = kInvalidTimerId; } - if (m_abort) { - onAborted(); + if (m_status == Status::Aborted) { + onAborted(networkReply->request().url()); return qMakePair(nullptr, statusCode); } + m_status = Status::Finished; if (networkReply->error() != QNetworkReply::NetworkError::NoError) { onNetworkError( diff --git a/src/network/webtask.h b/src/network/webtask.h index 358f4fed57f..e78f0c2be94 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -136,12 +136,28 @@ class WebTask : public QObject { QByteArray errorContent); protected: - void timerEvent(QTimerEvent* event) override; + void timerEvent(QTimerEvent* event) final; - // Handle an aborted request and ensure that the task eventually + enum class Status { + Idle, + Pending, + Aborted, + TimedOut, + Finished, + }; + + // Handle status changes and ensure that the task eventually // gets deleted. The default implementation simply deletes the // task. - virtual void onAborted(); + virtual void onAborted( + QUrl requestUrl); + virtual void onTimedOut( + QUrl requestUrl); + + QUrl abortPendingNetworkReply( + QNetworkReply* pendingNetworkReply); + QUrl timeOutPendingNetworkReply( + QNetworkReply* pendingNetworkReply); // Handle the abort and ensure that the task eventually // gets deleted. The default implementation logs a warning @@ -158,14 +174,16 @@ class WebTask : public QObject { virtual bool doStart( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) = 0; - virtual void doAbort() = 0; + // Handle status change requests and return the request URL + virtual QUrl doAbort() = 0; + virtual QUrl doTimeOut() = 0; // All member variables must only be accessed from // the event loop thread!! const QPointer m_networkAccessManager; int m_timeoutTimerId; - bool m_abort; + Status m_status; }; } // namespace network From 88b083a7665ef9830a669c7d3f6d8775424b1ead Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Mar 2020 15:48:24 +0100 Subject: [PATCH 10/20] Fix restarting of WebTask --- src/network/webtask.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index eb2e43cf6a3..10214930127 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -179,7 +179,7 @@ void WebTask::deleteAfterFinished() { void WebTask::slotStart(int timeoutMillis) { DEBUG_ASSERT(thread() == QThread::currentThread()); - DEBUG_ASSERT(m_status == Status::Idle); + DEBUG_ASSERT(m_status != Status::Pending); VERIFY_OR_DEBUG_ASSERT(m_networkAccessManager) { kLogger.warning() << "No network access"; @@ -188,6 +188,7 @@ void WebTask::slotStart(int timeoutMillis) { kLogger.debug() << "Starting..."; + m_status = Status::Idle; if (!doStart(m_networkAccessManager, timeoutMillis)) { kLogger.warning() << "Start aborted"; From e2740bac94ad34ac8da8fc4a8a6da6bded9e6668 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 24 Mar 2020 19:26:01 +0100 Subject: [PATCH 11/20] Prevent re-import of metadata from file tags after MusicBrainz lookup --- src/library/dlgtagfetcher.cpp | 7 ++++++- src/track/track.cpp | 17 ++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/library/dlgtagfetcher.cpp b/src/library/dlgtagfetcher.cpp index 629ecf68fd7..c7394dea4c8 100644 --- a/src/library/dlgtagfetcher.cpp +++ b/src/library/dlgtagfetcher.cpp @@ -177,7 +177,12 @@ void DlgTagFetcher::apply() { trackRelease.releaseGroupId); } #endif // __EXTRA_METADATA__ - m_track->importMetadata(std::move(trackMetadata)); + m_track->importMetadata( + std::move(trackMetadata), + // Prevent re-import of outdated metadata from file tags + // by explicitly setting the synchronization time stamp + // to the current time. + QDateTime::currentDateTimeUtc()); } void DlgTagFetcher::quit() { diff --git a/src/track/track.cpp b/src/track/track.cpp index 27e04c8282f..3252dc3faf6 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -135,9 +135,20 @@ void Track::importMetadata( // enter locking scope QMutexLocker lock(&m_qMutex); - bool modified = compareAndSet( - &m_record.refMetadataSynchronized(), - !metadataSynchronized.isNull()); + bool modified = false; + // Only set the metadata synchronized flag (column `header_parsed` + // in the database) from false to true, but never reset it back to + // false. Otherwise file tags would be re-imported and overwrite + // the metadata stored in the database, e.g. after retrieving metadata + // from MusicBrainz! + // TODO: In the future this flag should become a time stamp + // to detect updates of files and then decide based on time + // stamps if file tags need to be re-imported. + if (!metadataSynchronized.isNull()) { + modified |= compareAndSet( + &m_record.refMetadataSynchronized(), + true); + } bool modifiedReplayGain = false; if (m_record.getMetadata() != importedMetadata) { modifiedReplayGain = From f1d3976921fc96f9b9cd64630d5f58a3dedbc4ce Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 25 Mar 2020 09:55:13 +0100 Subject: [PATCH 12/20] Add documentation for pure virtual functions --- src/network/webtask.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/network/webtask.h b/src/network/webtask.h index e78f0c2be94..81f62bc46cc 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -171,10 +171,17 @@ class WebTask : public QObject { QPair receiveNetworkReply(); 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( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) = 0; - // Handle status change requests and return the request URL + + // 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 tha QUrl() must be + // returned. virtual QUrl doAbort() = 0; virtual QUrl doTimeOut() = 0; From 168381f4475dc7bda970387d7e6791d886742f1e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 25 Mar 2020 09:55:52 +0100 Subject: [PATCH 13/20] Simplify start procedure after rethinking the interactions --- src/network/webtask.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index 10214930127..b385e5461c1 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -190,16 +190,18 @@ void WebTask::slotStart(int timeoutMillis) { << "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); kLogger.warning() << "Start aborted"; return; } - - // The task could be aborted immediately while being started. - if (m_status == Status::Aborted) { - onAborted(doAbort()); - return; - } + // Still idle after the request has been started + // successfully, i.e. nothing happend yet in this + // thread. + DEBUG_ASSERT(m_status == Status::Idle); m_status = Status::Pending; DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); From 4da251ba2b5c2349a66399ad9f2a85f8539587a0 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 28 Mar 2020 11:47:47 +0100 Subject: [PATCH 14/20] Provide an abort() function for composite web tasks --- src/network/webtask.cpp | 14 ++++++++++---- src/network/webtask.h | 6 +++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index b385e5461c1..4348660e252 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -78,7 +78,7 @@ void WebTask::onAborted( const auto signal = QMetaMethod::fromSignal( &WebTask::aborted); if (isSignalConnected(signal)) { - emit aborted(); + emit aborted(requestUrl); } else { kLogger.info() << "Request aborted" @@ -235,11 +235,11 @@ QUrl WebTask::timeOutPendingNetworkReply( return pendingNetworkReply->request().url(); } -void WebTask::slotAbort() { +QUrl WebTask::abort() { DEBUG_ASSERT(thread() == QThread::currentThread()); if (m_status != Status::Pending) { DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId); - return; + return QUrl(); } if (m_timeoutTimerId != kInvalidTimerId) { killTimer(m_timeoutTimerId); @@ -248,7 +248,13 @@ void WebTask::slotAbort() { m_status = Status::Aborted; kLogger.debug() << "Aborting..."; - onAborted(doAbort()); + QUrl url = doAbort(); + onAborted(url); + return url; +} + +void WebTask::slotAbort() { + abort(); } void WebTask::timerEvent(QTimerEvent* event) { diff --git a/src/network/webtask.h b/src/network/webtask.h index 81f62bc46cc..f9b13c8f763 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -109,6 +109,9 @@ class WebTask : public QObject { // Cancel a pending request. void invokeAbort(); + // Cancel a pending request from the event loop thread. + QUrl abort(); + // Abort the pending request while suppressing any signals // and mark the task for deletion. void deleteBeforeFinished(); @@ -128,7 +131,8 @@ class WebTask : public QObject { // in memory as a dysfunctional zombie until its parent object // is finally deleted. If no receiver is connected the task // will be deleted implicitly. - void aborted(); + void aborted( + QUrl requestUrl); void networkError( QUrl requestUrl, QNetworkReply::NetworkError errorCode, From 260f7a0b9217c667728eb8b7f08d9becd150ae2d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 31 Mar 2020 12:53:54 +0200 Subject: [PATCH 15/20] Always finish WebTask with a signal --- src/network/webtask.cpp | 35 +++++++++++++++-------------------- src/network/webtask.h | 4 ++-- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index 4348660e252..a9a05cf9681 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -90,22 +90,11 @@ void WebTask::onAborted( void WebTask::onTimedOut( QUrl requestUrl) { DEBUG_ASSERT(m_status == Status::TimedOut); - const auto signal = QMetaMethod::fromSignal( - &WebTask::networkError); - if (isSignalConnected(signal)) { - emit networkError( - std::move(requestUrl), - QNetworkReply::TimeoutError, - QStringLiteral("Client-side timeout"), - QByteArray()); - } else { - kLogger.warning() - << "Aborting request after client-side timeout" - << requestUrl; - // Do not abort the request before deleting it, - // i.e. pretend it already has been finished. - deleteAfterFinished(); - } + onNetworkError( + requestUrl, + QNetworkReply::TimeoutError, + tr("Client-side network timeout"), + QByteArray()); } void WebTask::onNetworkError( @@ -181,8 +170,11 @@ void WebTask::slotStart(int timeoutMillis) { DEBUG_ASSERT(thread() == QThread::currentThread()); DEBUG_ASSERT(m_status != Status::Pending); VERIFY_OR_DEBUG_ASSERT(m_networkAccessManager) { - kLogger.warning() - << "No network access"; + onNetworkError( + QUrl(), + QNetworkReply::NetworkSessionFailedError, + tr("No network access"), + QByteArray()); return; } @@ -194,8 +186,11 @@ void WebTask::slotStart(int timeoutMillis) { // The callee is not supposed to abort a request // before it has beeen started successfully. DEBUG_ASSERT(m_status == Status::Idle); - kLogger.warning() - << "Start aborted"; + onNetworkError( + QUrl(), + QNetworkReply::OperationCanceledError, + tr("Start of network task has been aborted"), + QByteArray()); return; } // Still idle after the request has been started diff --git a/src/network/webtask.h b/src/network/webtask.h index f9b13c8f763..3995ca20399 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -151,8 +151,8 @@ class WebTask : public QObject { }; // Handle status changes and ensure that the task eventually - // gets deleted. The default implementation simply deletes the - // task. + // gets deleted. The default implementations emit a signal + // if connected or otherwise implicitly delete the task. virtual void onAborted( QUrl requestUrl); virtual void onTimedOut( From fff19142c4ac139368122f0bf997e7d21e5ccef4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 31 Mar 2020 17:05:58 +0200 Subject: [PATCH 16/20] Add debug output operators for requests and repsonses --- src/network/jsonwebtask.cpp | 8 ++++++++ src/network/jsonwebtask.h | 2 ++ src/network/webtask.cpp | 16 ++++++++++++++++ src/network/webtask.h | 6 ++++-- 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index 8ccdcffddcb..ca002487b02 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -87,6 +87,14 @@ QNetworkRequest newRequest( qRegisterMetaType("mixxx::network::JsonWebResponse"); } +QDebug operator<<(QDebug dbg, const JsonWebResponse& arg) { + return dbg + << "CustomWebResponse{" + << static_cast(arg) + << arg.content + << '}'; +} + JsonWebTask::JsonWebTask( QNetworkAccessManager* networkAccessManager, QUrl baseUrl, diff --git a/src/network/jsonwebtask.h b/src/network/jsonwebtask.h index 879feb1cf6f..d0491e05bc1 100644 --- a/src/network/jsonwebtask.h +++ b/src/network/jsonwebtask.h @@ -46,6 +46,8 @@ struct JsonWebResponse : public WebResponse { QJsonDocument content; }; +QDebug operator<<(QDebug dbg, const JsonWebResponse& arg); + class JsonWebTask : public WebTask { Q_OBJECT diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index a9a05cf9681..8565665a2c2 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -54,10 +54,26 @@ bool readStatusCode( qRegisterMetaType("mixxx::network::WebResponse"); } +QDebug operator<<(QDebug dbg, const WebResponse& arg) { + return dbg + << "WebResponse{" + << arg.replyUrl + << arg.statusCode + << '}'; +} + /*static*/ void CustomWebResponse::registerMetaType() { qRegisterMetaType("mixxx::network::CustomWebResponse"); } +QDebug operator<<(QDebug dbg, const CustomWebResponse& arg) { + return dbg + << "CustomWebResponse{" + << static_cast(arg) + << arg.content + << '}'; +} + WebTask::WebTask( QNetworkAccessManager* networkAccessManager) : m_networkAccessManager(networkAccessManager), diff --git a/src/network/webtask.h b/src/network/webtask.h index 3995ca20399..2c5c3babeaf 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -58,6 +58,8 @@ struct WebResponse { HttpStatusCode statusCode; }; +QDebug operator<<(QDebug dbg, const WebResponse& arg); + struct CustomWebResponse : public WebResponse { public: static void registerMetaType(); @@ -76,11 +78,11 @@ struct CustomWebResponse : public WebResponse { CustomWebResponse& operator=(const CustomWebResponse&) = default; CustomWebResponse& operator=(CustomWebResponse&&) = default; - QUrl replyUrl; - HttpStatusCode statusCode; QByteArray content; }; +QDebug operator<<(QDebug dbg, const CustomWebResponse& arg); + // A transient task for performing a single HTTP network request // asynchronously. // From bf40170df323a5864fa7bfe96e1bb44b92b28099 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 31 Mar 2020 17:06:52 +0200 Subject: [PATCH 17/20] Remove implicit auto-delete from web tasks --- src/musicbrainz/tagfetcher.cpp | 85 ++++++++++++------- src/musicbrainz/tagfetcher.h | 20 ++--- src/musicbrainz/web/acoustidlookuptask.cpp | 24 +++--- src/musicbrainz/web/acoustidlookuptask.h | 5 +- .../web/musicbrainzrecordingstask.cpp | 58 ++++++++----- .../web/musicbrainzrecordingstask.h | 9 +- src/network/jsonwebtask.cpp | 29 ++++--- src/network/jsonwebtask.h | 11 +-- src/network/webtask.cpp | 67 +++++++-------- src/network/webtask.h | 39 +++++---- 10 files changed, 190 insertions(+), 157 deletions(-) diff --git a/src/musicbrainz/tagfetcher.cpp b/src/musicbrainz/tagfetcher.cpp index 1435aa2fa50..3c80410a526 100644 --- a/src/musicbrainz/tagfetcher.cpp +++ b/src/musicbrainz/tagfetcher.cpp @@ -36,30 +36,17 @@ void TagFetcher::startFetch( &TagFetcher::slotFingerprintReady); } -void TagFetcher::abortAcoustIdTask() { +void TagFetcher::cancel() { + m_pTrack.reset(); + m_fingerprintWatcher.cancel(); if (m_pAcoustIdTask) { - disconnect(m_pAcoustIdTask); - m_pAcoustIdTask->deleteBeforeFinished(); - m_pAcoustIdTask = nullptr; + m_pAcoustIdTask->invokeAbort(); } -} - -void TagFetcher::abortMusicBrainzTask() { if (m_pMusicBrainzTask) { - disconnect(m_pMusicBrainzTask); - m_pMusicBrainzTask->deleteBeforeFinished(); - m_pMusicBrainzTask = nullptr; + m_pMusicBrainzTask->invokeAbort(); } } -void TagFetcher::cancel() { - // qDebug()<< "Cancel tagfetching"; - m_fingerprintWatcher.cancel(); - abortAcoustIdTask(); - abortMusicBrainzTask(); - m_pTrack.reset(); -} - void TagFetcher::slotFingerprintReady() { if (!m_pTrack || !m_fingerprintWatcher.isFinished()) { @@ -74,14 +61,13 @@ void TagFetcher::slotFingerprintReady() { return; } - abortAcoustIdTask(); - emit fetchProgress(tr("Identifying track through Acoustid")); DEBUG_ASSERT(!m_pAcoustIdTask); - m_pAcoustIdTask = new mixxx::AcoustIdLookupTask( + m_pAcoustIdTask = make_parented( &m_network, fingerprint, - m_pTrack->getDurationInt()); + m_pTrack->getDurationInt(), + this); connect(m_pAcoustIdTask, &mixxx::AcoustIdLookupTask::succeeded, this, @@ -104,7 +90,12 @@ void TagFetcher::slotFingerprintReady() { void TagFetcher::slotAcoustIdTaskSucceeded( QList recordingIds) { - abortAcoustIdTask(); + VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { + return; + } + m_pAcoustIdTask->deleteAfterFinished(); + m_pAcoustIdTask = nullptr; + if (!m_pTrack) { return; } @@ -118,9 +109,10 @@ void TagFetcher::slotAcoustIdTaskSucceeded( emit fetchProgress(tr("Retrieving metadata from MusicBrainz")); DEBUG_ASSERT(!m_pMusicBrainzTask); - m_pMusicBrainzTask = new mixxx::MusicBrainzRecordingsTask( + m_pMusicBrainzTask = make_parented( &m_network, - std::move(recordingIds)); + std::move(recordingIds), + this); connect(m_pMusicBrainzTask, &mixxx::MusicBrainzRecordingsTask::succeeded, this, @@ -143,7 +135,11 @@ void TagFetcher::slotAcoustIdTaskSucceeded( void TagFetcher::slotAcoustIdTaskFailed( mixxx::network::JsonWebResponse response) { - cancel(); + VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { + return; + } + m_pAcoustIdTask->deleteAfterFinished(); + m_pAcoustIdTask = nullptr; emit networkError( response.statusCode, "AcoustID", @@ -152,7 +148,11 @@ void TagFetcher::slotAcoustIdTaskFailed( } void TagFetcher::slotAcoustIdTaskAborted() { - abortAcoustIdTask(); + VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { + return; + } + m_pAcoustIdTask->deleteAfterFinished(); + m_pAcoustIdTask = nullptr; } void TagFetcher::slotAcoustIdTaskNetworkError( @@ -162,7 +162,11 @@ void TagFetcher::slotAcoustIdTaskNetworkError( QByteArray errorContent) { Q_UNUSED(requestUrl); Q_UNUSED(errorContent); - cancel(); + VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { + return; + } + m_pAcoustIdTask->deleteAfterFinished(); + m_pAcoustIdTask = nullptr; emit networkError( mixxx::network::kHttpStatusCodeInvalid, "AcoustID", @@ -171,7 +175,11 @@ void TagFetcher::slotAcoustIdTaskNetworkError( } void TagFetcher::slotMusicBrainzTaskAborted() { - abortMusicBrainzTask(); + VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { + return; + } + m_pMusicBrainzTask->deleteAfterFinished(); + m_pMusicBrainzTask = nullptr; } void TagFetcher::slotMusicBrainzTaskNetworkError( @@ -181,7 +189,11 @@ void TagFetcher::slotMusicBrainzTaskNetworkError( QByteArray errorContent) { Q_UNUSED(requestUrl); Q_UNUSED(errorContent); - cancel(); + VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { + return; + } + m_pMusicBrainzTask->deleteAfterFinished(); + m_pMusicBrainzTask = nullptr; emit networkError( mixxx::network::kHttpStatusCodeInvalid, "MusicBrainz", @@ -193,7 +205,11 @@ void TagFetcher::slotMusicBrainzTaskFailed( mixxx::network::WebResponse response, int errorCode, QString errorMessage) { - cancel(); + VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { + return; + } + m_pMusicBrainzTask->deleteAfterFinished(); + m_pMusicBrainzTask = nullptr; emit networkError( response.statusCode, "MusicBrainz", @@ -204,7 +220,12 @@ void TagFetcher::slotMusicBrainzTaskFailed( void TagFetcher::slotMusicBrainzTaskSucceeded( QList guessedTrackReleases) { auto pOriginalTrack = std::move(m_pTrack); - abortMusicBrainzTask(); + DEBUG_ASSERT(!m_pTrack); + VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { + return; + } + m_pMusicBrainzTask->deleteAfterFinished(); + m_pMusicBrainzTask = nullptr; if (!pOriginalTrack) { // aborted return; diff --git a/src/musicbrainz/tagfetcher.h b/src/musicbrainz/tagfetcher.h index 622cebe11cf..3caf4ac7e32 100644 --- a/src/musicbrainz/tagfetcher.h +++ b/src/musicbrainz/tagfetcher.h @@ -2,23 +2,24 @@ #include #include -#include #include "musicbrainz/web/acoustidlookuptask.h" #include "musicbrainz/web/musicbrainzrecordingstask.h" #include "track/track.h" +#include "util/parented_ptr.h" class TagFetcher : public QObject { - Q_OBJECT + Q_OBJECT - // Implements retrieval of metadata in 3 subsequent stages: - // 1. Chromaprint -> AcoustID fingerprint - // 2. AcoustID -> MusicBrainz recording UUIDs - // 3. MusicBrainz -> MusicBrainz track releases + // Implements retrieval of metadata in 3 subsequent stages: + // 1. Chromaprint -> AcoustID fingerprint + // 2. AcoustID -> MusicBrainz recording UUIDs + // 3. MusicBrainz -> MusicBrainz track releases public: explicit TagFetcher( QObject* parent = nullptr); + ~TagFetcher() override = default; void startFetch( TrackPointer pTrack); @@ -66,16 +67,13 @@ class TagFetcher : public QObject { QByteArray errorContent); private: - void abortAcoustIdTask(); - void abortMusicBrainzTask(); - QNetworkAccessManager m_network; QFutureWatcher m_fingerprintWatcher; - QPointer m_pAcoustIdTask; + parented_ptr m_pAcoustIdTask; - QPointer m_pMusicBrainzTask; + parented_ptr m_pMusicBrainzTask; TrackPointer m_pTrack; }; diff --git a/src/musicbrainz/web/acoustidlookuptask.cpp b/src/musicbrainz/web/acoustidlookuptask.cpp index 6f8a2acdfd7..a9807f6b8bb 100644 --- a/src/musicbrainz/web/acoustidlookuptask.cpp +++ b/src/musicbrainz/web/acoustidlookuptask.cpp @@ -67,11 +67,13 @@ network::JsonWebRequest lookupRequest() { AcoustIdLookupTask::AcoustIdLookupTask( QNetworkAccessManager* networkAccessManager, const QString& fingerprint, - int duration) + int duration, + QObject* parent) : network::JsonWebTask( networkAccessManager, kBaseUrl, - lookupRequest()), + lookupRequest(), + parent), m_urlQuery(lookupUrlQuery(fingerprint, duration)) { } @@ -110,7 +112,7 @@ QNetworkReply* AcoustIdLookupTask::sendNetworkRequest( } void AcoustIdLookupTask::onFinished( - network::JsonWebResponse response) { + network::JsonWebResponse&& response) { if (!response.isStatusCodeSuccess()) { kLogger.warning() << "Request failed with HTTP status code" @@ -198,7 +200,7 @@ void AcoustIdLookupTask::onFinished( const auto recordingId = QUuid(recordingObject.value(QLatin1String("id")).toString()); VERIFY_OR_DEBUG_ASSERT(!recordingId.isNull()) { - continue; + continue; } recordingIds.append(recordingId); } @@ -209,15 +211,15 @@ void AcoustIdLookupTask::onFinished( void AcoustIdLookupTask::emitSucceeded( QList&& recordingIds) { - const auto signal = QMetaMethod::fromSignal( - &AcoustIdLookupTask::succeeded); - DEBUG_ASSERT(receivers(signal.name()) <= 1); // unique connection - if (isSignalConnected(signal)) { - emit succeeded( - std::move(recordingIds)); - } else { + VERIFY_OR_DEBUG_ASSERT( + isSignalFuncConnected(&AcoustIdLookupTask::succeeded)) { + kLogger.warning() + << "Unhandled succeeded signal"; deleteLater(); + return; } + emit succeeded( + std::move(recordingIds)); } } // namespace mixxx diff --git a/src/musicbrainz/web/acoustidlookuptask.h b/src/musicbrainz/web/acoustidlookuptask.h index 34dc4574916..20a1a504a52 100644 --- a/src/musicbrainz/web/acoustidlookuptask.h +++ b/src/musicbrainz/web/acoustidlookuptask.h @@ -15,7 +15,8 @@ class AcoustIdLookupTask : public network::JsonWebTask { AcoustIdLookupTask( QNetworkAccessManager* networkAccessManager, const QString& fingerprint, - int duration); + int duration, + QObject* parent = nullptr); ~AcoustIdLookupTask() override = default; signals: @@ -31,7 +32,7 @@ class AcoustIdLookupTask : public network::JsonWebTask { private: void onFinished( - network::JsonWebResponse response) override; + network::JsonWebResponse&& response) override; void emitSucceeded( QList&& recordingIds); diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp index cccdaf673c5..054338e68c0 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp +++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp @@ -45,7 +45,7 @@ QUrlQuery createUrlQuery() { } QNetworkRequest createNetworkRequest( - const QUuid& recordingId) { + const QUuid& recordingId) { DEBUG_ASSERT(kBaseUrl.isValid()); DEBUG_ASSERT(!recordingId.isNull()); QUrl url = kBaseUrl; @@ -65,14 +65,22 @@ QNetworkRequest createNetworkRequest( MusicBrainzRecordingsTask::MusicBrainzRecordingsTask( QNetworkAccessManager* networkAccessManager, - QList&& recordingIds) + QList&& recordingIds, + QObject* parent) : network::WebTask( - networkAccessManager), + networkAccessManager, + parent), m_queuedRecordingIds(std::move(recordingIds)), m_parentTimeoutMillis(0) { musicbrainz::registerMetaTypesOnce(); } +MusicBrainzRecordingsTask::~MusicBrainzRecordingsTask() { + VERIFY_OR_DEBUG_ASSERT(!m_pendingNetworkReply) { + m_pendingNetworkReply->deleteLater(); + } +} + bool MusicBrainzRecordingsTask::doStart( QNetworkAccessManager* networkAccessManager, int parentTimeoutMillis) { @@ -176,13 +184,13 @@ void MusicBrainzRecordingsTask::slotNetworkReplyFinished() { << "GET reply" << "statusCode:" << statusCode << "body:" << body; - const auto error = musicbrainz::Error(reader); + auto error = musicbrainz::Error(reader); emitFailed( network::WebResponse( networkReply->url(), statusCode), error.code, - error.message); + std::move(error.message)); return; } @@ -223,31 +231,35 @@ void MusicBrainzRecordingsTask::slotNetworkReplyFinished() { void MusicBrainzRecordingsTask::emitSucceeded( QList&& trackReleases) { - const auto signal = QMetaMethod::fromSignal( - &MusicBrainzRecordingsTask::succeeded); - DEBUG_ASSERT(receivers(signal.name()) <= 1); // unique connection - if (isSignalConnected(signal)) { - emit succeeded( - std::move(trackReleases)); - } else { + VERIFY_OR_DEBUG_ASSERT( + isSignalFuncConnected(&MusicBrainzRecordingsTask::succeeded)) { + kLogger.warning() + << "Unhandled succeeded signal"; deleteLater(); + return; } + emit succeeded( + std::move(trackReleases)); } + void MusicBrainzRecordingsTask::emitFailed( - network::WebResponse response, + network::WebResponse&& response, int errorCode, - QString errorMessage) { - const auto signal = QMetaMethod::fromSignal( - &MusicBrainzRecordingsTask::succeeded); - DEBUG_ASSERT(receivers(signal.name()) <= 1); // unique connection - if (isSignalConnected(signal)) { - emit failed( - response, - errorCode, - errorMessage); - } else { + QString&& errorMessage) { + VERIFY_OR_DEBUG_ASSERT( + isSignalFuncConnected(&MusicBrainzRecordingsTask::failed)) { + kLogger.warning() + << "Unhandled failed signal" + << response + << errorCode + << errorMessage; deleteLater(); + return; } + emit failed( + std::move(response), + errorCode, + std::move(errorMessage)); } } // namespace mixxx diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.h b/src/musicbrainz/web/musicbrainzrecordingstask.h index 614315d7584..e3f65e55a80 100644 --- a/src/musicbrainz/web/musicbrainzrecordingstask.h +++ b/src/musicbrainz/web/musicbrainzrecordingstask.h @@ -17,8 +17,9 @@ class MusicBrainzRecordingsTask : public network::WebTask { public: MusicBrainzRecordingsTask( QNetworkAccessManager* networkAccessManager, - QList&& recordingIds); - ~MusicBrainzRecordingsTask() override = default; + QList&& recordingIds, + QObject* parent = nullptr); + ~MusicBrainzRecordingsTask() override; signals: void succeeded( @@ -41,9 +42,9 @@ class MusicBrainzRecordingsTask : public network::WebTask { void emitSucceeded( QList&& trackReleases); void emitFailed( - network::WebResponse response, + network::WebResponse&& response, int errorCode, - QString errorMessage); + QString&& errorMessage); void continueWithNextRequest(); diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index ca002487b02..2d4b76309f3 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -97,10 +97,11 @@ QDebug operator<<(QDebug dbg, const JsonWebResponse& arg) { JsonWebTask::JsonWebTask( QNetworkAccessManager* networkAccessManager, - QUrl baseUrl, - JsonWebRequest request) - : WebTask(networkAccessManager), - m_baseUrl(std::move(baseUrl)), + const QUrl& baseUrl, + JsonWebRequest&& request, + QObject* parent) + : WebTask(networkAccessManager, parent), + m_baseUrl(baseUrl), m_request(std::move(request)), m_pendingNetworkReply(nullptr) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); @@ -114,7 +115,7 @@ JsonWebTask::~JsonWebTask() { } void JsonWebTask::onFinished( - JsonWebResponse response) { + JsonWebResponse&& response) { kLogger.info() << "Response received" << response.replyUrl @@ -124,7 +125,7 @@ void JsonWebTask::onFinished( } void JsonWebTask::onFinishedCustom( - CustomWebResponse response) { + CustomWebResponse&& response) { kLogger.info() << "Custom response received" << response.replyUrl @@ -320,16 +321,16 @@ void JsonWebTask::slotNetworkReplyFinished() { } void JsonWebTask::emitFailed( - network::JsonWebResponse response) { - const auto signal = QMetaMethod::fromSignal( - &JsonWebTask::failed); - DEBUG_ASSERT(receivers(signal.name()) <= 1); // unique connection - if (isSignalConnected(signal)) { - emit failed( - std::move(response)); - } else { + network::JsonWebResponse&& response) { + VERIFY_OR_DEBUG_ASSERT( + isSignalFuncConnected(&JsonWebTask::failed)) { + kLogger.warning() + << "Unhandled failed signal" + << response; deleteLater(); + return; } + emit failed(std::move(response)); } } // namespace network diff --git a/src/network/jsonwebtask.h b/src/network/jsonwebtask.h index d0491e05bc1..9c653d7c6bd 100644 --- a/src/network/jsonwebtask.h +++ b/src/network/jsonwebtask.h @@ -54,8 +54,9 @@ class JsonWebTask : public WebTask { public: JsonWebTask( QNetworkAccessManager* networkAccessManager, - QUrl baseUrl, - JsonWebRequest request); + const QUrl& baseUrl, + JsonWebRequest&& request, + QObject* parent = nullptr); ~JsonWebTask() override; signals: @@ -74,16 +75,16 @@ class JsonWebTask : public WebTask { const QJsonDocument& content); void emitFailed( - network::JsonWebResponse response); + network::JsonWebResponse&& response); private: // Handle the response and ensure that the task eventually // gets deleted. The default implementation discards the // response and deletes the task. virtual void onFinished( - JsonWebResponse response); + JsonWebResponse&& response); virtual void onFinishedCustom( - CustomWebResponse response); + CustomWebResponse&& response); bool doStart( QNetworkAccessManager* networkAccessManager, diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index 8565665a2c2..c9998b53764 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -1,6 +1,5 @@ #include "network/webtask.h" -#include #include #include #include // std::once_flag @@ -75,8 +74,10 @@ QDebug operator<<(QDebug dbg, const CustomWebResponse& arg) { } WebTask::WebTask( - QNetworkAccessManager* networkAccessManager) - : m_networkAccessManager(networkAccessManager), + QNetworkAccessManager* networkAccessManager, + QObject* parent) + : QObject(parent), + m_networkAccessManager(networkAccessManager), m_timeoutTimerId(kInvalidTimerId), m_status(Status::Idle) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); @@ -89,52 +90,51 @@ WebTask::~WebTask() { } void WebTask::onAborted( - QUrl requestUrl) { + QUrl&& requestUrl) { DEBUG_ASSERT(m_status == Status::Aborted); - const auto signal = QMetaMethod::fromSignal( - &WebTask::aborted); - if (isSignalConnected(signal)) { - emit aborted(requestUrl); - } else { - kLogger.info() - << "Request aborted" + VERIFY_OR_DEBUG_ASSERT( + isSignalFuncConnected(&WebTask::aborted)) { + kLogger.warning() + << "Unhandled abort signal" << requestUrl; - deleteAfterFinished(); + deleteLater(); + return; } + emit aborted( + std::move(requestUrl)); } void WebTask::onTimedOut( - QUrl requestUrl) { + QUrl&& requestUrl) { DEBUG_ASSERT(m_status == Status::TimedOut); onNetworkError( - requestUrl, + std::move(requestUrl), QNetworkReply::TimeoutError, tr("Client-side network timeout"), QByteArray()); } void WebTask::onNetworkError( - QUrl requestUrl, + QUrl&& requestUrl, QNetworkReply::NetworkError errorCode, - QString errorString, - QByteArray errorContent) { - const auto signal = QMetaMethod::fromSignal( - &WebTask::networkError); - if (isSignalConnected(signal)) { - emit networkError( - std::move(requestUrl), - errorCode, - std::move(errorString), - std::move(errorContent)); - } else { + QString&& errorString, + QByteArray&& errorContent) { + VERIFY_OR_DEBUG_ASSERT( + isSignalFuncConnected(&WebTask::networkError)) { kLogger.warning() - << "Network error" + << "Unhandled network error signal" << requestUrl << errorCode << errorString << errorContent; - deleteAfterFinished(); + deleteLater(); + return; } + emit networkError( + std::move(requestUrl), + errorCode, + std::move(errorString), + std::move(errorContent)); } void WebTask::invokeStart(int timeoutMillis) { @@ -165,15 +165,6 @@ void WebTask::invokeAbort() { ); } -void WebTask::deleteBeforeFinished() { - // Might be called from any thread so we must not - // access any member variables! - // Do not disconnect any connections, because otherwise - // the destroyed() signal is not received! - invokeAbort(); - deleteLater(); -} - void WebTask::deleteAfterFinished() { // Might be called from any thread so we must not // access any member variables! @@ -260,7 +251,7 @@ QUrl WebTask::abort() { kLogger.debug() << "Aborting..."; QUrl url = doAbort(); - onAborted(url); + onAborted(QUrl(url)); return url; } diff --git a/src/network/webtask.h b/src/network/webtask.h index 2c5c3babeaf..09ce1413796 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -100,7 +101,8 @@ class WebTask : public QObject { public: explicit WebTask( - QNetworkAccessManager* networkAccessManager); + QNetworkAccessManager* networkAccessManager, + QObject* parent = nullptr); ~WebTask() override; // timeoutMillis <= 0: No timeout (unlimited) @@ -114,10 +116,6 @@ class WebTask : public QObject { // Cancel a pending request from the event loop thread. QUrl abort(); - // Abort the pending request while suppressing any signals - // and mark the task for deletion. - void deleteBeforeFinished(); - // Disconnect from all signals after receiving a reply // and mark the task for deletion. void deleteAfterFinished(); @@ -142,6 +140,13 @@ class WebTask : public QObject { QByteArray errorContent); protected: + template + bool isSignalFuncConnected( + S signalFunc) { + const QMetaMethod signal = QMetaMethod::fromSignal(signalFunc); + return isSignalConnected(signal); + } + void timerEvent(QTimerEvent* event) final; enum class Status { @@ -152,29 +157,29 @@ class WebTask : public QObject { Finished, }; + QUrl abortPendingNetworkReply( + QNetworkReply* pendingNetworkReply); + QUrl timeOutPendingNetworkReply( + QNetworkReply* pendingNetworkReply); + + QPair 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); + QUrl&& requestUrl); virtual void onTimedOut( - QUrl requestUrl); - - QUrl abortPendingNetworkReply( - QNetworkReply* pendingNetworkReply); - QUrl timeOutPendingNetworkReply( - QNetworkReply* pendingNetworkReply); + QUrl&& requestUrl); // Handle the abort and ensure that the task eventually // gets deleted. The default implementation logs a warning // and deletes the task. virtual void onNetworkError( - QUrl requestUrl, + QUrl&& requestUrl, QNetworkReply::NetworkError errorCode, - QString errorString, - QByteArray errorContent); - - QPair receiveNetworkReply(); + QString&& errorString, + QByteArray&& errorContent); private: // Try to compose and send the actual network request. If From 21ee89d3e545082ba8ddba2ca550e5ae035c6a7f Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 1 Apr 2020 00:35:12 +0200 Subject: [PATCH 18/20] Don't pass parent to object in a different thread --- src/musicbrainz/tagfetcher.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/musicbrainz/tagfetcher.cpp b/src/musicbrainz/tagfetcher.cpp index 3c80410a526..a79dd6a0939 100644 --- a/src/musicbrainz/tagfetcher.cpp +++ b/src/musicbrainz/tagfetcher.cpp @@ -25,8 +25,8 @@ void TagFetcher::startFetch( m_pTrack = pTrack; emit fetchProgress(tr("Fingerprinting track")); - const auto fingerprintTask = QtConcurrent::run([this, pTrack] { - return ChromaPrinter(this).getFingerprint(pTrack); + const auto fingerprintTask = QtConcurrent::run([pTrack] { + return ChromaPrinter().getFingerprint(pTrack); }); m_fingerprintWatcher.setFuture(fingerprintTask); connect( From 1e22f8c36cc580a316f7a73a1b99f91568e2a9fa Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 1 Apr 2020 13:10:26 +0200 Subject: [PATCH 19/20] Cancel all tasks in TagFetcher after last signal --- src/musicbrainz/tagfetcher.cpp | 124 +++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 54 deletions(-) diff --git a/src/musicbrainz/tagfetcher.cpp b/src/musicbrainz/tagfetcher.cpp index a79dd6a0939..f447930ffed 100644 --- a/src/musicbrainz/tagfetcher.cpp +++ b/src/musicbrainz/tagfetcher.cpp @@ -20,6 +20,7 @@ TagFetcher::TagFetcher(QObject* parent) void TagFetcher::startFetch( TrackPointer pTrack) { + DEBUG_ASSERT(thread() == QThread::currentThread()); cancel(); m_pTrack = pTrack; @@ -29,6 +30,7 @@ void TagFetcher::startFetch( return ChromaPrinter().getFingerprint(pTrack); }); m_fingerprintWatcher.setFuture(fingerprintTask); + DEBUG_ASSERT(!m_pAcoustIdTask); connect( &m_fingerprintWatcher, &QFutureWatcher::finished, @@ -37,22 +39,30 @@ void TagFetcher::startFetch( } void TagFetcher::cancel() { + DEBUG_ASSERT(thread() == QThread::currentThread()); m_pTrack.reset(); + m_fingerprintWatcher.disconnect(this); m_fingerprintWatcher.cancel(); if (m_pAcoustIdTask) { - m_pAcoustIdTask->invokeAbort(); + m_pAcoustIdTask->disconnect(this); + m_pAcoustIdTask->deleteLater(); + m_pAcoustIdTask = nullptr; } if (m_pMusicBrainzTask) { - m_pMusicBrainzTask->invokeAbort(); + m_pMusicBrainzTask->disconnect(this); + m_pMusicBrainzTask->deleteLater(); + m_pMusicBrainzTask = nullptr; } } void TagFetcher::slotFingerprintReady() { + DEBUG_ASSERT(thread() == QThread::currentThread()); if (!m_pTrack || !m_fingerprintWatcher.isFinished()) { return; } + DEBUG_ASSERT(m_fingerprintWatcher.isFinished()); const QString fingerprint = m_fingerprintWatcher.result(); if (fingerprint.isEmpty()) { emit resultAvailable( @@ -90,19 +100,15 @@ void TagFetcher::slotFingerprintReady() { void TagFetcher::slotAcoustIdTaskSucceeded( QList recordingIds) { - VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { - return; - } - m_pAcoustIdTask->deleteAfterFinished(); - m_pAcoustIdTask = nullptr; - - if (!m_pTrack) { - return; - } + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pAcoustIdTask.get() == + qobject_cast(sender())); if (recordingIds.isEmpty()) { + auto pTrack = std::move(m_pTrack); + cancel(); emit resultAvailable( - m_pTrack, + std::move(pTrack), QList()); return; } @@ -135,11 +141,12 @@ void TagFetcher::slotAcoustIdTaskSucceeded( void TagFetcher::slotAcoustIdTaskFailed( mixxx::network::JsonWebResponse response) { - VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { - return; - } - m_pAcoustIdTask->deleteAfterFinished(); - m_pAcoustIdTask = nullptr; + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pAcoustIdTask.get() == + qobject_cast(sender())); + + cancel(); + emit networkError( response.statusCode, "AcoustID", @@ -148,11 +155,16 @@ void TagFetcher::slotAcoustIdTaskFailed( } void TagFetcher::slotAcoustIdTaskAborted() { - VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { - return; - } - m_pAcoustIdTask->deleteAfterFinished(); - m_pAcoustIdTask = nullptr; + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pAcoustIdTask.get() == + qobject_cast(sender())); + + auto pTrack = std::move(m_pTrack); + cancel(); + + emit resultAvailable( + std::move(pTrack), + QList{}); } void TagFetcher::slotAcoustIdTaskNetworkError( @@ -162,11 +174,12 @@ void TagFetcher::slotAcoustIdTaskNetworkError( QByteArray errorContent) { Q_UNUSED(requestUrl); Q_UNUSED(errorContent); - VERIFY_OR_DEBUG_ASSERT(m_pAcoustIdTask) { - return; - } - m_pAcoustIdTask->deleteAfterFinished(); - m_pAcoustIdTask = nullptr; + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pAcoustIdTask.get() == + qobject_cast(sender())); + + cancel(); + emit networkError( mixxx::network::kHttpStatusCodeInvalid, "AcoustID", @@ -175,11 +188,16 @@ void TagFetcher::slotAcoustIdTaskNetworkError( } void TagFetcher::slotMusicBrainzTaskAborted() { - VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { - return; - } - m_pMusicBrainzTask->deleteAfterFinished(); - m_pMusicBrainzTask = nullptr; + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pMusicBrainzTask.get() == + qobject_cast(sender())); + + auto pTrack = std::move(m_pTrack); + cancel(); + + emit resultAvailable( + std::move(pTrack), + QList{}); } void TagFetcher::slotMusicBrainzTaskNetworkError( @@ -189,11 +207,12 @@ void TagFetcher::slotMusicBrainzTaskNetworkError( QByteArray errorContent) { Q_UNUSED(requestUrl); Q_UNUSED(errorContent); - VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { - return; - } - m_pMusicBrainzTask->deleteAfterFinished(); - m_pMusicBrainzTask = nullptr; + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pMusicBrainzTask.get() == + qobject_cast(sender())); + + cancel(); + emit networkError( mixxx::network::kHttpStatusCodeInvalid, "MusicBrainz", @@ -205,11 +224,12 @@ void TagFetcher::slotMusicBrainzTaskFailed( mixxx::network::WebResponse response, int errorCode, QString errorMessage) { - VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { - return; - } - m_pMusicBrainzTask->deleteAfterFinished(); - m_pMusicBrainzTask = nullptr; + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pMusicBrainzTask.get() == + qobject_cast(sender())); + + cancel(); + emit networkError( response.statusCode, "MusicBrainz", @@ -219,18 +239,14 @@ void TagFetcher::slotMusicBrainzTaskFailed( void TagFetcher::slotMusicBrainzTaskSucceeded( QList guessedTrackReleases) { - auto pOriginalTrack = std::move(m_pTrack); - DEBUG_ASSERT(!m_pTrack); - VERIFY_OR_DEBUG_ASSERT(m_pMusicBrainzTask) { - return; - } - m_pMusicBrainzTask->deleteAfterFinished(); - m_pMusicBrainzTask = nullptr; - if (!pOriginalTrack) { - // aborted - return; - } + DEBUG_ASSERT(thread() == QThread::currentThread()); + DEBUG_ASSERT(m_pMusicBrainzTask.get() == + qobject_cast(sender())); + + auto pTrack = std::move(m_pTrack); + cancel(); + emit resultAvailable( - pOriginalTrack, + std::move(pTrack), std::move(guessedTrackReleases)); } From b8bc7ba817b0811cc5e992bf2cd5069da8cb7a36 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 1 Apr 2020 20:39:12 +0200 Subject: [PATCH 20/20] Delete obsolete method deleteAfterFinished() --- src/network/jsonwebtask.cpp | 4 ++-- src/network/webtask.cpp | 8 -------- src/network/webtask.h | 4 ---- 3 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp index 2d4b76309f3..8e8802c1ea4 100644 --- a/src/network/jsonwebtask.cpp +++ b/src/network/jsonwebtask.cpp @@ -121,7 +121,7 @@ void JsonWebTask::onFinished( << response.replyUrl << response.statusCode << response.content; - deleteAfterFinished(); + deleteLater(); } void JsonWebTask::onFinishedCustom( @@ -131,7 +131,7 @@ void JsonWebTask::onFinishedCustom( << response.replyUrl << response.statusCode << response.content; - deleteAfterFinished(); + deleteLater(); } QNetworkReply* JsonWebTask::sendNetworkRequest( diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp index c9998b53764..fc7f1b3f356 100644 --- a/src/network/webtask.cpp +++ b/src/network/webtask.cpp @@ -165,14 +165,6 @@ void WebTask::invokeAbort() { ); } -void WebTask::deleteAfterFinished() { - // Might be called from any thread so we must not - // access any member variables! - // Do not disconnect any connections, because otherwise - // the destroyed() signal is not received! - deleteLater(); -} - void WebTask::slotStart(int timeoutMillis) { DEBUG_ASSERT(thread() == QThread::currentThread()); DEBUG_ASSERT(m_status != Status::Pending); diff --git a/src/network/webtask.h b/src/network/webtask.h index 09ce1413796..3fc9beb91db 100644 --- a/src/network/webtask.h +++ b/src/network/webtask.h @@ -116,10 +116,6 @@ class WebTask : public QObject { // Cancel a pending request from the event loop thread. QUrl abort(); - // Disconnect from all signals after receiving a reply - // and mark the task for deletion. - void deleteAfterFinished(); - public slots: void slotStart( int timeoutMillis);