Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lp1865957: Fetch multiple tracks from MusicBrainz #2534

Merged
merged 25 commits into from
Apr 1, 2020
Merged

lp1865957: Fetch multiple tracks from MusicBrainz #2534

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c234d88
Allow restarting of web tasks
uklotzde Mar 7, 2020
a8e2ab4
Merge branch 'master' of [email protected]:mixxxdj/mixxx.git into restar…
uklotzde Mar 7, 2020
0fff63c
Add comment about abort while being started
uklotzde Mar 9, 2020
3ff18a1
Merge branch 'master' into restart_webtask
uklotzde Mar 16, 2020
c28fcd6
Prevent web network tasks from becoming parented
uklotzde Mar 23, 2020
f9264c3
Handle QNetworkReply::error signals
uklotzde Mar 23, 2020
785e711
Fix disconnect/connect when loading new tracks for lookup
uklotzde Mar 23, 2020
6341398
Handle abort signals of AcoustId and MusicBrainz tasks
uklotzde Mar 23, 2020
303c0bc
Signal failure to parse XML reponse
uklotzde Mar 23, 2020
db450a1
Yet another backport for Xenial
uklotzde Mar 24, 2020
38f625d
Handle client-side timeouts
uklotzde Mar 24, 2020
7eca5c8
Merge branch 'master' into restart_webtask
uklotzde Mar 24, 2020
88b083a
Fix restarting of WebTask
uklotzde Mar 24, 2020
e2740ba
Prevent re-import of metadata from file tags after MusicBrainz lookup
uklotzde Mar 24, 2020
f1d3976
Add documentation for pure virtual functions
uklotzde Mar 25, 2020
168381f
Simplify start procedure after rethinking the interactions
uklotzde Mar 25, 2020
54fc724
Merge branch 'master' into restart_webtask
uklotzde Mar 28, 2020
4da251b
Provide an abort() function for composite web tasks
uklotzde Mar 28, 2020
638d00c
Merge branch 'master' of [email protected]:mixxxdj/mixxx.git into restar…
uklotzde Mar 31, 2020
260f7a0
Always finish WebTask with a signal
uklotzde Mar 31, 2020
fff1914
Add debug output operators for requests and repsonses
uklotzde Mar 31, 2020
bf40170
Remove implicit auto-delete from web tasks
uklotzde Mar 31, 2020
21ee89d
Don't pass parent to object in a different thread
uklotzde Mar 31, 2020
1e22f8c
Cancel all tasks in TagFetcher after last signal
uklotzde Apr 1, 2020
b8bc7ba
Delete obsolete method deleteAfterFinished()
uklotzde Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/library/dlgtagfetcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,25 +84,26 @@ 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);

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();
}

Expand Down Expand Up @@ -176,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() {
Expand Down
42 changes: 28 additions & 14 deletions src/musicbrainz/tagfetcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,15 @@ void TagFetcher::startFetch(

void TagFetcher::abortAcoustIdTask() {
if (m_pAcoustIdTask) {
disconnect(m_pAcoustIdTask.get());
disconnect(m_pAcoustIdTask);
m_pAcoustIdTask->deleteBeforeFinished();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crash happens here. The operator-> returns the plain pointer here in the GUI thread. If the thread is suspended and the delete from the worker thread happens in between, than the pointer becomes dangling an the crash occurs.
https://github.com/uklotzde/mixxx/pull/11 fixes the issue.
I strongly recommend to remove all auto deletion code as done in the PR.
If you need fire and forget nature in your other branches, inhered from the classes and add the desired nature.
It is kind of unpredictable and hard to review to have bot concepts mixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The self-destruction code is now enclosed in VERFIY_AND_DEBUG_ASSERT to ensure that no unhandled signals slip through. It only serves as a fallback to prevent memory leaks.

I still don't understand how a crash could happen when using deleteLater(). This should ensure that objects remain alive until all signal handling has finished and control has been returned to the event loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I can explain:

DeleteLater sends a signal to the object that calls a slot that deletes the object from the thread the object lives in. This ensures that all signals that have been issued before and might be still in the queue are executed before deletion, which would lead to a crash in the best case.

Since Qt5 Qt uses it's QWeakPointer and QSharedPointer internally to maintain the object tree. This works with less locking than the Qt4 approach, that utilizes signals and slots.
A QPointer is essential a wrapper around Qt's weak pointer. It becomes a nullptr at any time once the last reference to the object was removed.

The code of question can be also written like this:

    if (m_pAcoustIdTask) {
        disconnect(m_pAcoustIdTask);
        m_pAcoustIdTask->deleteBeforeFinished();

Can be also written inlined as:

    if (m_pAcoustIdTask) {
        disconnect(m_pAcoustIdTask);
        mixxx::AcoustIdLookupTask pTask = m_pAcoustIdTask.data();
       QMetaObject::invokeMethod([] {pTask->slotAbort();  })

The crash happens if the object is deleted from the other thread just between the last two lines. m_pAcoustIdTask.data() returns the still valid pointer 0x120697a0 but it becomes dangling before pTask->slotAbort() is called.

The DeleteLater trick dos not help here, because the object is already gone.

m_pAcoustIdTask = nullptr;
}
}

void TagFetcher::abortMusicBrainzTask() {
if (m_pMusicBrainzTask) {
disconnect(m_pMusicBrainzTask.get());
disconnect(m_pMusicBrainzTask);
m_pMusicBrainzTask->deleteBeforeFinished();
m_pMusicBrainzTask = nullptr;
}
Expand Down Expand Up @@ -78,20 +78,23 @@ void TagFetcher::slotFingerprintReady() {

emit fetchProgress(tr("Identifying track through Acoustid"));
DEBUG_ASSERT(!m_pAcoustIdTask);
m_pAcoustIdTask = make_parented<mixxx::AcoustIdLookupTask>(
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::aborted,
this,
&TagFetcher::slotAcoustIdTaskAborted);
connect(m_pAcoustIdTask,
&mixxx::AcoustIdLookupTask::networkError,
this,
&TagFetcher::slotAcoustIdTaskNetworkError);
Expand All @@ -115,19 +118,22 @@ void TagFetcher::slotAcoustIdTaskSucceeded(

emit fetchProgress(tr("Retrieving metadata from MusicBrainz"));
DEBUG_ASSERT(!m_pMusicBrainzTask);
m_pMusicBrainzTask = make_parented<mixxx::MusicBrainzRecordingsTask>(
m_pMusicBrainzTask = new mixxx::MusicBrainzRecordingsTask(
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
&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::aborted,
this,
&TagFetcher::slotMusicBrainzTaskAborted);
connect(m_pMusicBrainzTask,
&mixxx::MusicBrainzRecordingsTask::networkError,
this,
&TagFetcher::slotMusicBrainzTaskNetworkError);
Expand All @@ -145,6 +151,10 @@ void TagFetcher::slotAcoustIdTaskFailed(
-1);
}

void TagFetcher::slotAcoustIdTaskAborted() {
abortAcoustIdTask();
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
}

void TagFetcher::slotAcoustIdTaskNetworkError(
QUrl requestUrl,
QNetworkReply::NetworkError errorCode,
Expand All @@ -160,6 +170,10 @@ void TagFetcher::slotAcoustIdTaskNetworkError(
errorCode);
}

void TagFetcher::slotMusicBrainzTaskAborted() {
abortMusicBrainzTask();
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
}

void TagFetcher::slotMusicBrainzTaskNetworkError(
QUrl requestUrl,
QNetworkReply::NetworkError errorCode,
Expand Down
8 changes: 5 additions & 3 deletions src/musicbrainz/tagfetcher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

#include <QFutureWatcher>
#include <QObject>
#include <QPointer>

#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
Expand Down Expand Up @@ -45,6 +45,7 @@ class TagFetcher : public QObject {
QList<QUuid> recordingIds);
void slotAcoustIdTaskFailed(
mixxx::network::JsonWebResponse response);
void slotAcoustIdTaskAborted();
void slotAcoustIdTaskNetworkError(
QUrl requestUrl,
QNetworkReply::NetworkError errorCode,
Expand All @@ -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,
Expand All @@ -71,9 +73,9 @@ class TagFetcher : public QObject {

QFutureWatcher<QString> m_fingerprintWatcher;

parented_ptr<mixxx::AcoustIdLookupTask> m_pAcoustIdTask;
QPointer<mixxx::AcoustIdLookupTask> m_pAcoustIdTask;

parented_ptr<mixxx::MusicBrainzRecordingsTask> m_pMusicBrainzTask;
QPointer<mixxx::MusicBrainzRecordingsTask> m_pMusicBrainzTask;

TrackPointer m_pTrack;
};
6 changes: 2 additions & 4 deletions src/musicbrainz/web/acoustidlookuptask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
}

Expand Down
3 changes: 1 addition & 2 deletions src/musicbrainz/web/acoustidlookuptask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
42 changes: 35 additions & 7 deletions src/musicbrainz/web/musicbrainzrecordingstask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,9 @@ QNetworkRequest createNetworkRequest(

MusicBrainzRecordingsTask::MusicBrainzRecordingsTask(
QNetworkAccessManager* networkAccessManager,
QList<QUuid>&& recordingIds,
QObject* parent)
QList<QUuid>&& recordingIds)
: network::WebTask(
networkAccessManager,
parent),
networkAccessManager),
m_queuedRecordingIds(std::move(recordingIds)),
m_parentTimeoutMillis(0) {
musicbrainz::registerMetaTypesOnce();
Expand Down Expand Up @@ -117,13 +115,38 @@ bool MusicBrainzRecordingsTask::doStart(
&MusicBrainzRecordingsTask::slotNetworkReplyFinished,
Qt::UniqueConnection);

connect(m_pendingNetworkReply,
QOverload<QNetworkReply::NetworkError>::of(&QNetworkReply::error),
this,
&MusicBrainzRecordingsTask::slotNetworkReplyFinished,
Qt::UniqueConnection);

return true;
}

void MusicBrainzRecordingsTask::doAbort() {
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() {
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 MusicBrainzRecordingsTask::slotNetworkReplyFinished() {
Expand Down Expand Up @@ -175,7 +198,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;
}

Expand Down
6 changes: 3 additions & 3 deletions src/musicbrainz/web/musicbrainzrecordingstask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ class MusicBrainzRecordingsTask : public network::WebTask {
public:
MusicBrainzRecordingsTask(
QNetworkAccessManager* networkAccessManager,
QList<QUuid>&& recordingIds,
QObject* parent = nullptr);
QList<QUuid>&& recordingIds);
~MusicBrainzRecordingsTask() override = default;

signals:
Expand All @@ -36,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<musicbrainz::TrackRelease>&& trackReleases);
Expand Down
42 changes: 37 additions & 5 deletions src/network/jsonwebtask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
#include <mutex> // 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"

Expand Down Expand Up @@ -87,9 +90,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) {
Expand Down Expand Up @@ -237,13 +239,43 @@ bool JsonWebTask::doStart(
&JsonWebTask::slotNetworkReplyFinished,
Qt::UniqueConnection);

connect(m_pendingNetworkReply,
#if QT_VERSION >= QT_VERSION_CHECK(5, 7, 0)
qOverload<QNetworkReply::NetworkError>(&QNetworkReply::error),
#else
QOverload<QNetworkReply::NetworkError>::of(&QNetworkReply::error),
#endif
this,
&JsonWebTask::slotNetworkReplyFinished,
Qt::UniqueConnection);

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() {
Expand Down
6 changes: 3 additions & 3 deletions src/network/jsonwebtask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ class JsonWebTask : public WebTask {
JsonWebTask(
QNetworkAccessManager* networkAccessManager,
QUrl baseUrl,
JsonWebRequest request,
QObject* parent = nullptr);
JsonWebRequest request);
~JsonWebTask() override;

signals:
Expand Down Expand Up @@ -87,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!!
Expand Down
Loading