2.3 Musicbrainz fixes #10875

daschuer · 2022-09-07T22:28:19Z

This includes a couple of fixes regarding non fatal errors and missing GUI feedback after client timeout.
This also improves the error message that is shown in some cases.

Fixes #10883

In case of no pNetworkAccessManager change to Failed state.

… musicbrainz

uklotzde · 2022-09-07T22:57:52Z

src/network/webtask.cpp

+        m_state = State::Failed;
+        onNetworkError(
+                QNetworkReply::NetworkSessionFailedError,
+                tr("Request URL issue, network request has not been started"),


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.

Ok, I will change the message accordingly.

uklotzde · 2022-09-07T23:01:13Z

src/network/webtask.cpp

-        break;
-    case QNetworkReply::TimeoutError:
-        // Network or server-side timeout
+    case QNetworkReply::OperationCanceledError: // Client-side timeout


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

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.

#10884

Writing code is easier than fruitless arguing and explaining.

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

uklotzde · 2022-09-07T23:04:18Z

src/network/webtask.cpp

@@ -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);


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

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.

uklotzde · 2022-09-07T23:09:33Z

src/network/webtask.h

@@ -182,6 +182,11 @@ class WebTask : public NetworkTask {
            const QString& errorString,
            const WebResponseWithContent& responseWithContent);

+  protected:
+    virtual void doNetworkError(


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?

What is you suggestion to improve this situation.

O have renamed onNetworkError() to doNetworkError()

uklotzde · 2022-09-07T23:22:47Z

Sure, there might be undiscovered bugs. But I cannot follow most of these changes, especially in the base classes, and some decisions appear to be inconsistent as I outlined.

uklotzde · 2022-09-07T23:27:28Z

Others may take over. Time to say goodbye, finally.

uklotzde · 2022-09-08T00:21:11Z

Btw, my aoide web tasks worked flawlessly until now. With this PR they are triggering unexpected errors when aborted.

daschuer · 2022-09-08T07:16:11Z

Btw, my aoide web tasks worked flawlessly until now. With this PR they are triggering unexpected errors when aborted.

Please share some details, that we have at least a chance to fix this.

uklotzde · 2022-09-08T08:02:52Z

Btw, my aoide web tasks worked flawlessly until now. With this PR they are triggering unexpected errors when aborted.

Please share some details, that we have at least a chance to fix this.

Example: Abort a pending search and start a new search.

There is nothing to fix, because it works as expected. I already pointed out that it is wrong to remove the distinction between client-side abort and server-side timeout and evidence proves me right.

Please fix your downstream code instead of subverting upstream code.

daschuer · 2022-09-08T08:30:06Z

I cannot confirm that abborting a request and starting a new one is broken now. Instead this PR fixes the stalled GUI in case of a client timout.

Did you actually tried this code or do you share an assumption from the code review. Let's discuss along the code what is the real issue. Or are you prepared to overrule this PR with your one, like the previous one?

uklotzde · 2022-09-08T09:41:23Z

Did you actually tried this code or do you share an assumption from the code review. Let's discuss along the code what is the real issue. Or are you prepared to overrule this PR with your one, like the previous one?

I tried it after expressing my concerns on this PR and and the results proved me right.

uklotzde · 2022-09-08T09:42:36Z

I have battle tested this stuff probably more than anyone else. If there are issues than point them out and provide instructions to reproduce them.

fatihemreyildiz · 2022-09-09T07:33:11Z

I reported a bug earlier #10796. The bug is, if one of the MusicBrainz URL has an empty XML, whole task was failing even though there are other releases found.

I have tested it and now it is resolved. The result is:

…mmon pattern

daschuer · 2022-09-09T17:27:48Z

@uklotzde

I tried it after expressing my concerns on this PR and and the results proved me right.

Can you please explain step by step how to reproduce the issue? Maybe you have seen the bug recently fixed here:
#10878

ywwg · 2022-10-01T19:24:26Z

I do not love the design of these classes and I agree that there is some weirdness with the "on" and "do" functions and various overrides, but I don't think daniel's changes are harmful to the design of the code.

Btw, my aoide web tasks worked flawlessly until now. With this PR they are triggering unexpected errors when aborted.

@uklotzde if you want us to fix this new issue then you need to provide instructions to reproduce, and/or logs so that we can track it down. It is not fair to say that a PR is broken but you're unwilling to say exactly how. Daniel said he tried to reproduce your issue based on your description and was unable to do so. We have one user who has said this PR explicitly fixes a bug, and that bug has steps to reproduce. On that basis alone I am inclined to approve this PR so that we fix a known bug.

I think the main problem here is the complete lack of testing on this code. Writing tests for network code is tedious and difficult, and often requires mocks or even artificial environments to properly test, but I think we would all be well served by taking some time to establish tests for this code that covers the cases mentioned. Then at least we can talk about regressions in terms of concrete results or writing new tests.

I would propose that for this PR we write a single test that exercises the specific bug being fixed here. Yes, that may require a bunch of effort to establish a foundation. Then, after this is merged, if Uwe can write a test for client side cancellation that fails or is flaky, we can fix that in a subsequent PR.

… function usable in tests

daschuer · 2022-10-21T11:40:46Z

Now the tests are in place. This was a lot of work!
@ywwg Can we merge this now?

ywwg · 2022-10-24T20:28:43Z

@fatihemreyildiz can you reconfirm that this branch fixes your issue? If so, ok to merge

fatihemreyildiz · 2022-10-24T21:29:24Z

Hey @ywwg, I have tested again and this PR fixes issue #10796 👍 . Here is a screenshot I just took:

However while I was testing it, I guess I discovered an issue. I don't know if it is only happening to me. When I tried to import metadata, sometimes the Tag Fetcher Dialog was populating with Operation Canceled error.

This is happened when I looked for metadata populated from track property and abort the task, after aborting I changed the songs via Next & Previous buttons on track properties, and pressed on Import Metadata button again. Could you please try it too?

The steps to reproduce is:

Populate Properties of a song
Press Import Metadata From Musicbrainz
Close the Tag Fetcher Dialog (After fetching is success or while fetching, that doesn't matter)
Change the songs via buttons on Properties
The task is starting without the menu after each track change.

…gnals

daschuer · 2022-10-30T13:45:22Z

I was able to reproduce and fix the stray() "Operation canceled" signal when using the next/previous buttons.
@fatihemreyildiz can you confirm that?

fatihemreyildiz · 2022-10-31T09:06:23Z

Hey @daschuer , I've tried it a lot. Couldn't receive the Operation Canceled error. As far as I tested, it is fixed.

Also, when I fetch metadata from track properties and close only the DlgTagFetcher and press Next/Prev buttons the task is starting on the background. I can observe it on the console.
Edit: I've also tried it on various 2.3 branches and I can still observe this issue. On upstream/main there is no such an issue. I think this is a bug needs to be reported. What do you think?

daschuer · 2022-10-31T21:05:09Z

Thank you for testing. No need for a new bug. I will add the fix here.

… has been closed

daschuer · 2022-10-31T22:53:53Z

@fatihemreyildiz Done.
Please test again. I have also fixed an issue that happens because the track is received twice after next. One Time form DlgTagFetcher and again from DlgTrackInfo
Thank you.

fatihemreyildiz · 2022-11-01T06:50:43Z

I have tested it couple of times, and It seems like the bug is resolved 👍
Thanks.

daschuer · 2022-11-01T07:24:28Z

Cool, thank you for the immediate help!

@ywwg this is ready for merge now. Please have a look.

daschuer and others added 8 commits September 8, 2022 00:23

Make XML errors non fatal, because only one responds might be empty.

04bbef6

Make use of structured binding declaration

2d78c63

Remove wrong setting of Inital state.

48d0378

In case of no pNetworkAccessManager change to Failed state.

fix feedback in case of client side timeout

a75d93f

Use emitNetworkError in case the URL is invalid

ad3172d

display the Http Status code instead of always -1

404d308

Continue in case of non fatal errors during fetching of recordings

ef94bb0

Improve error messages in case we have no error message and code from…

41abb4d

… musicbrainz

daschuer added the bug label Sep 7, 2022

daschuer added this to the 2.3.4 milestone Sep 7, 2022

uklotzde suggested changes Sep 7, 2022

View reviewed changes

Do not mention the URi in case of sub class errors

7ba6ae4

Rename doNetworkError() to onNetworkError() to not conflict with a co…

498e828

…mmon pattern

This was referenced Sep 9, 2022

If MusicBrainz Returns an empty XML (404) whole task fails and no results displayed #10796

Closed

Stalled MusicBrainz GUI after client timout #10883

Closed

uklotzde mentioned this pull request Sep 9, 2022

NetworkTask: Distinguish implicit abort by timeout from explicit abort #10884

Closed

daschuer mentioned this pull request Sep 12, 2022

Fix: Rate limit exceeds for Musicbrainztask. #10822

Closed

daschuer added 2 commits October 15, 2022 00:29

Add class MockNetworkAccessManager

86f4c07

Reformat MockNetworkAccessManager using clang-format

1e34da3

daschuer added 5 commits October 15, 2022 00:29

Improve MockNetworkManager

a29fa94

Replace rlvalue reference by a normal reference parameter to make the…

9f69b96

… function usable in tests

Implement MockNetworkReply::abort()

ba501bc

Improve comment

7c388fa

Make WebTask::isBusy() public

8b1fffc

github-actions bot added build code quality labels Oct 21, 2022

daschuer force-pushed the musicbrainz_fixes branch 2 times, most recently from 11a486b to b66b76a Compare October 21, 2022 19:19

Add MusicBrainzRecordingsTaskTest

10326bc

daschuer force-pushed the musicbrainz_fixes branch from b66b76a to 10326bc Compare October 21, 2022 21:36

daschuer requested a review from ywwg October 22, 2022 05:35

TagFetcher: add a terminate() function to discard stray canceled() si…

e53c558

…gnals

daschuer added 4 commits October 31, 2022 23:49

Clear dialog in case of a null track

a4d8b70

Don't request meta data from musikbrains after the Musivbrainz dialog…

eb217e9

… has been closed

Avoid a double track update after next or previous

ec09754

Don't clear result if already cleared.

bb21444

github-actions bot added the library label Oct 31, 2022

ywwg merged commit 76fbfb2 into mixxxdj:2.3 Nov 3, 2022

daschuer deleted the musicbrainz_fixes branch November 16, 2022 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.3 Musicbrainz fixes #10875

2.3 Musicbrainz fixes #10875

daschuer commented Sep 7, 2022 •

edited by ywwg

Loading

uklotzde Sep 7, 2022

daschuer Sep 8, 2022

uklotzde Sep 7, 2022

daschuer Sep 8, 2022

uklotzde Sep 9, 2022

uklotzde Sep 7, 2022 •

edited

Loading

daschuer Sep 8, 2022

uklotzde Sep 7, 2022

daschuer Sep 8, 2022

daschuer Sep 9, 2022

uklotzde commented Sep 7, 2022

uklotzde commented Sep 7, 2022

uklotzde commented Sep 8, 2022

daschuer commented Sep 8, 2022 •

edited

Loading

uklotzde commented Sep 8, 2022

daschuer commented Sep 8, 2022

uklotzde commented Sep 8, 2022

uklotzde commented Sep 8, 2022

fatihemreyildiz commented Sep 9, 2022

daschuer commented Sep 9, 2022

ywwg commented Oct 1, 2022

daschuer commented Oct 21, 2022

ywwg commented Oct 24, 2022

fatihemreyildiz commented Oct 24, 2022

daschuer commented Oct 30, 2022

fatihemreyildiz commented Oct 31, 2022

daschuer commented Oct 31, 2022

daschuer commented Oct 31, 2022

fatihemreyildiz commented Nov 1, 2022

daschuer commented Nov 1, 2022

2.3 Musicbrainz fixes #10875

2.3 Musicbrainz fixes #10875

Conversation

daschuer commented Sep 7, 2022 • edited by ywwg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Sep 7, 2022

uklotzde commented Sep 7, 2022

uklotzde commented Sep 8, 2022

daschuer commented Sep 8, 2022 • edited Loading

uklotzde commented Sep 8, 2022

daschuer commented Sep 8, 2022

uklotzde commented Sep 8, 2022

uklotzde commented Sep 8, 2022

fatihemreyildiz commented Sep 9, 2022

daschuer commented Sep 9, 2022

ywwg commented Oct 1, 2022

daschuer commented Oct 21, 2022

ywwg commented Oct 24, 2022

fatihemreyildiz commented Oct 24, 2022

daschuer commented Oct 30, 2022

fatihemreyildiz commented Oct 31, 2022

daschuer commented Oct 31, 2022

daschuer commented Oct 31, 2022

fatihemreyildiz commented Nov 1, 2022

daschuer commented Nov 1, 2022

daschuer commented Sep 7, 2022 •

edited by ywwg

Loading

uklotzde Sep 7, 2022 •

edited

Loading

daschuer commented Sep 8, 2022 •

edited

Loading