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

Jump to bottom

2.3 Musicbrainz fixes #10875

Merged
merged 23 commits into from
Nov 3, 2022
Merged

2.3 Musicbrainz fixes #10875

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
04bbef6
Make XML errors non fatal, because only one responds might be empty.
daschuer Sep 7, 2022
2d78c63
Make use of structured binding declaration
daschuer Sep 7, 2022
48d0378
Remove wrong setting of Inital state.
fatihemreyildiz Aug 27, 2022
a75d93f
fix feedback in case of client side timeout
daschuer Aug 30, 2022
ad3172d
Use emitNetworkError in case the URL is invalid
daschuer Sep 3, 2022
404d308
display the Http Status code instead of always -1
daschuer Sep 7, 2022
ef94bb0
Continue in case of non fatal errors during fetching of recordings
daschuer Sep 7, 2022
41abb4d
Improve error messages in case we have no error message and code from…
daschuer Sep 7, 2022
7ba6ae4
Do not mention the URi in case of sub class errors
daschuer Sep 8, 2022
498e828
Rename doNetworkError() to onNetworkError() to not conflict with a co…
daschuer Sep 9, 2022
86f4c07
Add class MockNetworkAccessManager
daschuer Oct 12, 2022
1e34da3
Reformat MockNetworkAccessManager using clang-format
daschuer Oct 12, 2022
a29fa94
Improve MockNetworkManager
daschuer Oct 14, 2022
9f69b96
Replace rlvalue reference by a normal reference parameter to make the…
daschuer Oct 16, 2022
ba501bc
Implement MockNetworkReply::abort()
daschuer Oct 21, 2022
7c388fa
Improve comment
daschuer Oct 21, 2022
8b1fffc
Make WebTask::isBusy() public
daschuer Oct 21, 2022
10326bc
Add MusicBrainzRecordingsTaskTest
daschuer Oct 21, 2022
e53c558
TagFetcher: add a terminate() function to discard stray canceled() si…
daschuer Oct 30, 2022
a4d8b70
Clear dialog in case of a null track
daschuer Oct 31, 2022
eb217e9
Don't request meta data from musikbrains after the Musivbrainz dialog…
daschuer Oct 31, 2022
ec09754
Avoid a double track update after next or previous
daschuer Oct 31, 2022
bb21444
Don't clear result if already cleared.
daschuer Oct 31, 2022
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
6 changes: 2 additions & 4 deletions src/musicbrainz/tagfetcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ void TagFetcher::slotAcoustIdTaskNetworkError(
QNetworkReply::NetworkError errorCode,
const QString& errorString,
const mixxx::network::WebResponseWithContent& responseWithContent) {
Q_UNUSED(responseWithContent);
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
if (!onAcoustIdTaskTerminated()) {
return;
Expand All @@ -201,7 +200,7 @@ void TagFetcher::slotAcoustIdTaskNetworkError(
cancel();

emit networkError(
mixxx::network::kHttpStatusCodeInvalid,
responseWithContent.statusCode(),
QStringLiteral("AcoustID"),
errorString,
errorCode);
Expand Down Expand Up @@ -234,7 +233,6 @@ void TagFetcher::slotMusicBrainzTaskNetworkError(
QNetworkReply::NetworkError errorCode,
const QString& errorString,
const mixxx::network::WebResponseWithContent& responseWithContent) {
Q_UNUSED(responseWithContent);
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
if (!onMusicBrainzTaskTerminated()) {
return;
Expand All @@ -243,7 +241,7 @@ void TagFetcher::slotMusicBrainzTaskNetworkError(
cancel();

emit networkError(
mixxx::network::kHttpStatusCodeInvalid,
responseWithContent.statusCode(),
QStringLiteral("MusicBrainz"),
errorString,
errorCode);
Expand Down
58 changes: 35 additions & 23 deletions src/musicbrainz/web/musicbrainzrecordingstask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ QNetworkReply* MusicBrainzRecordingsTask::doStartNetworkRequest(
}

void MusicBrainzRecordingsTask::doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
QNetworkReply* pFinishedNetworkReply,
network::HttpStatusCode statusCode) {
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);

const QByteArray body = finishedNetworkReply->readAll();
const QByteArray body = pFinishedNetworkReply->readAll();
QXmlStreamReader reader(body);

// HTTP status of successful results:
Expand All @@ -130,39 +130,45 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
<< "statusCode:" << statusCode
<< "body:" << body;
auto error = musicbrainz::Error(reader);
emitFailed(
network::WebResponse(
finishedNetworkReply->url(),
finishedNetworkReply->request().url(),
statusCode),
error.code,
error.message);
if (error.code) {
emitFailed(
network::WebResponse(
pFinishedNetworkReply->url(),
pFinishedNetworkReply->request().url(),
statusCode),
error.code,
error.message);
return;
}
WebTask::doNetworkError(pFinishedNetworkReply, statusCode);
return;
}

auto recordingsResult = musicbrainz::parseRecordings(reader);
for (auto&& trackRelease : recordingsResult.first) {
const auto [trackReleases, success] = musicbrainz::parseRecordings(reader);
for (auto&& trackRelease : trackReleases) {
// In case of a response with status 301 (Moved Permanently)
// the actual recording id might differ from the requested id.
// To avoid requesting recording ids twice we need to remember
// all recording ids.
m_finishedRecordingIds.insert(trackRelease.recordingId);
m_trackReleases.insert(trackRelease.trackReleaseId, trackRelease);
}
if (!recordingsResult.second) {
kLogger.warning()
<< "Failed to parse XML response";
emitFailed(
network::WebResponse(
finishedNetworkReply->url(),
finishedNetworkReply->request().url(),
statusCode),
-1,
QStringLiteral("Failed to parse XML response"));
return;
}

if (m_queuedRecordingIds.isEmpty()) {
if (!success && m_trackReleases.isEmpty()) {
// this error is only fatal if we have no tracks at all
kLogger.warning()
<< "Failed to parse XML response";
emitFailed(
network::WebResponse(
pFinishedNetworkReply->url(),
pFinishedNetworkReply->request().url(),
statusCode),
-1,
QStringLiteral("Failed to parse XML response"));
return;
}

// Finished all recording ids
m_finishedRecordingIds.clear();
auto trackReleases = m_trackReleases.values();
Expand All @@ -184,6 +190,12 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
slotStart(m_parentTimeoutMillis, delayBeforeNextRequest.toIntegerMillis());
}

void MusicBrainzRecordingsTask::doNetworkError(
QNetworkReply* pFinishedNetworkReply,
network::HttpStatusCode statusCode) {
doNetworkReplyFinished(pFinishedNetworkReply, statusCode);
}

void MusicBrainzRecordingsTask::emitSucceeded(
const QList<musicbrainz::TrackRelease>& trackReleases) {
VERIFY_OR_DEBUG_ASSERT(
Expand Down
7 changes: 6 additions & 1 deletion src/musicbrainz/web/musicbrainzrecordingstask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,17 @@ class MusicBrainzRecordingsTask : public network::WebTask {
int errorCode,
const QString& errorMessage);

protected:
void doNetworkError(
QNetworkReply* finishedNetworkReply,
network::HttpStatusCode statusCode) override;

private:
QNetworkReply* doStartNetworkRequest(
QNetworkAccessManager* networkAccessManager,
int parentTimeoutMillis) override;
void doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
QNetworkReply* pFinishedNetworkReply,
network::HttpStatusCode statusCode) override;

void emitSucceeded(
Expand Down
77 changes: 35 additions & 42 deletions src/network/webtask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,32 +123,24 @@ void WebTask::onNetworkError(
QNetworkReply::NetworkError errorCode,
const QString& errorString,
const WebResponseWithContent& responseWithContent) {
DEBUG_ASSERT(m_state == State::Pending);
DEBUG_ASSERT(m_state == State::Failed || m_state == State::Pending);
Copy link
Contributor

@uklotzde uklotzde Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

How could the internal state be already Failed when a network reply arrives?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because this function is also called, before the request is issued and the state goes to the pending state.

mixxx/src/network/webtask.cpp

Line 200 in 41abb4d

onNetworkError(

Before, the state was artificially set to State::Pending, even though there was already a failure detected.
Probably just to trick the original assertion.
This is fixed now for a traceable usage of states.

DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);

DEBUG_ASSERT(errorCode != QNetworkReply::NoError);
switch (errorCode) {
case QNetworkReply::OperationCanceledError:
// Client-side abort or timeout
m_state = State::Aborted;
break;
case QNetworkReply::TimeoutError:
// Network or server-side timeout
case QNetworkReply::OperationCanceledError: // Client-side timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Client- and server-side abort should be distinguished. Why did you merge them? The state TimedOut is wrong when manually aborting a request.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are here in onNetworkError() this is only called if a start request fails.
The case you mention is handled in slotAbort().
The change was required, because sometime MusicBrainz does not respond at all.
In that case the client Timothy of 60 s takes place. This case is a network error as well, and leads to a proper feedback about this situation to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

#10884

Writing code is easier than fruitless arguing and explaining.

Override doNetworkReplyAborted() in the derived class and do whatever needs to be done.

case QNetworkReply::TimeoutError: // Network or server-side timeout
m_state = State::TimedOut;
break;
default:
m_state = State::Failed;
}
DEBUG_ASSERT(hasTerminated());

if (m_state == State::Aborted) {
emitAborted(responseWithContent.requestUrl());
} else {
emitNetworkError(
errorCode,
errorString,
responseWithContent);
}
emitNetworkError(
errorCode,
errorString,
responseWithContent);
}

void WebTask::emitNetworkError(
Expand Down Expand Up @@ -183,7 +175,6 @@ void WebTask::slotStart(int timeoutMillis, int delayMillis) {
// Reset state
DEBUG_ASSERT(!m_pendingNetworkReplyWeakPtr);
DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
m_state = State::Initial;
m_timeoutMillis = kNoTimeout;

if (delayMillis > 0) {
Expand All @@ -205,7 +196,7 @@ void WebTask::slotStart(int timeoutMillis, int delayMillis) {

auto* const pNetworkAccessManager = m_networkAccessManagerWeakPtr.data();
VERIFY_OR_DEBUG_ASSERT(pNetworkAccessManager) {
m_state = State::Pending;
m_state = State::Failed;
onNetworkError(
QNetworkReply::NetworkSessionFailedError,
tr("No network access"),
Expand All @@ -222,18 +213,12 @@ void WebTask::slotStart(int timeoutMillis, int delayMillis) {
m_pendingNetworkReplyWeakPtr = doStartNetworkRequest(
pNetworkAccessManager,
timeoutMillis);
// Still idle, because we are in the same thread.
// The derived class is not allowed to abort a request
// during the callback before it has beeen started
// successfully. Instead it should return nullptr
// to abort the task immediately.
DEBUG_ASSERT(m_state == State::Initial);
if (!m_pendingNetworkReplyWeakPtr) {
kLogger.debug()
<< this
<< "Network request has not been started";
m_state = State::Aborted;
emitAborted(/*request URL is unknown*/);
m_state = State::Failed;
onNetworkError(
QNetworkReply::NetworkSessionFailedError,
tr("Request URL issue, network request has not been started"),
Copy link
Contributor

Choose a reason for hiding this comment

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

How could you be sure that this only happens for invalid URLs? Derived classes may return nullptr for whatever obstacles they encounter during doStartNetworkRequest(). Making such implicit assumptions is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will change the message accordingly.

WebResponseWithContent{});
return;
}

Expand Down Expand Up @@ -263,7 +248,7 @@ void WebTask::slotAbort() {
if (m_state == State::Initial) {
kLogger.debug()
<< this
<< "Cannot abort idle task";
<< "Cannot abort task in Initial state";
} else {
DEBUG_ASSERT(hasTerminated());
kLogger.debug()
Expand Down Expand Up @@ -403,27 +388,35 @@ void WebTask::slotNetworkReplyFinished() {
m_timeoutTimerId = kInvalidTimerId;
}

const auto statusCode = readStatusCode(*pFinishedNetworkReply);
const HttpStatusCode statusCode = readStatusCode(*pFinishedNetworkReply);
if (pFinishedNetworkReply->error() != QNetworkReply::NetworkError::NoError) {
onNetworkError(
pFinishedNetworkReply->error(),
pFinishedNetworkReply->errorString(),
WebResponseWithContent{
WebResponse{
pFinishedNetworkReply->url(),
pFinishedNetworkReply->request().url(),
statusCode},
readContentType(*pFinishedNetworkReply),
readContentData(pFinishedNetworkReply).value_or(QByteArray{}),
});
DEBUG_ASSERT(hasTerminated());
m_state = State::Failed;
doNetworkError(pFinishedNetworkReply, statusCode);
return;
}

m_state = State::Finished;
doNetworkReplyFinished(pFinishedNetworkReply, statusCode);
}

void WebTask::doNetworkError(
QNetworkReply* pFinishedNetworkReply,
HttpStatusCode statusCode) {
onNetworkError(
pFinishedNetworkReply->error(),
pFinishedNetworkReply->errorString(),
WebResponseWithContent{
WebResponse{
pFinishedNetworkReply->url(),
pFinishedNetworkReply->request().url(),
statusCode},
readContentType(*pFinishedNetworkReply),
readContentData(pFinishedNetworkReply).value_or(QByteArray{}),
});
DEBUG_ASSERT(hasTerminated());
return;
}

} // namespace network

} // namespace mixxx
7 changes: 6 additions & 1 deletion src/network/webtask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ class WebTask : public NetworkTask {
const QString& errorString,
const WebResponseWithContent& responseWithContent);

protected:
virtual void doNetworkError(
Copy link
Contributor

Choose a reason for hiding this comment

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

The do prefix indicates the Template Method pattern with a pure virtual function that is only invoked by the base class and inaccessible for derived classes. Derived classes are supposed to implement those functions, but are not allowed to invoke them at arbitrary times.

In contrast this seems to be an ordinary, overridable virtual function. It is also unclear even clear why and when you should override it?

Copy link
Member Author

Choose a reason for hiding this comment

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

What is you suggestion to improve this situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

O have renamed onNetworkError() to doNetworkError()

QNetworkReply* pFinishedNetworkReply,
HttpStatusCode statusCode);

private:
QUrl abortPendingNetworkReply();

Expand All @@ -199,7 +204,7 @@ class WebTask : public NetworkTask {

/// Handle network response.
virtual void doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
QNetworkReply* pFinishedNetworkReply,
HttpStatusCode statusCode) = 0;

/// Handle the abort and ensure that the task eventually
Expand Down