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.

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

Handle FreeDB server errors gracefully #304

Merged
merged 3 commits into from Oct 6, 2018

Conversation

Projects
None yet
2 participants
@anarcat
Contributor

anarcat commented Oct 5, 2018

In my repeated tests, I ended up upsetting freedb.org, which started
issuing 502 errors. Those errors are not correctly caught by the
program which just crashes with a backtrace. Instead, we handle those
like any other API error, which can already be generated by
perform_lookup (but not handled).

The visible result for the user is that the CD is simply not found on
FreeDB, an acceptable compromise, in my opinion.

Closes: #206

anarcat added some commits Oct 5, 2018

handle FreeDB server errors gracefully
In my repeated tests, I ended up upsetting freedb.org, which started
issuing 502 errors. Those errors are not correctly caught by the
program which just crashes with a backtrace. Instead, we handle those
like any other API error, which can already be generated by
perform_lookup (but not handled).

The visible result for the user is that the CD is simply not found on
FreeDB, an acceptable compromise, in my opinion.

Closes: #206
harmonize an error message with other warnings
... which are all capitalized.
@anarcat

This comment has been minimized.

Contributor

anarcat commented Oct 5, 2018

note that #206 was closed, but should be reopened before this is merged of course. :)

@JoeLametta

This comment has been minimized.

Collaborator

JoeLametta commented Oct 6, 2018

Thanks for reporting this issue. I've reopened #206.

Shouldn't we just handle the exception in program.py without adding additional code to freedb.py? (the perform_lookup function in whipper's freedb.py is intentionally almost identical to the one found in python-audio-tools).

@anarcat

This comment has been minimized.

Contributor

anarcat commented Oct 6, 2018

Shouldn't we just handle the exception in program.py without adding additional code to freedb.py? (the perform_lookup function in whipper's freedb.py is intentionally almost identical to the one found in python-audio-tools).

The reason I did it this way was to catch the ValueError that could occur elsewhere in the freedb.py code. But it's true we could just catch URLError there as well. The problem then is we depend upon freedb-specific code: if they decide to switch to (say) requests to do network requests, our error-handling goes out the window. Plus it means we now depend on urllib in program.py, which is not currently present.

I was trying to keep things separate...

@JoeLametta JoeLametta changed the title from handle FreeDB server errors gracefully to Handle FreeDB server errors gracefully Oct 6, 2018

@JoeLametta

This comment has been minimized.

Collaborator

JoeLametta commented Oct 6, 2018

The problem then is we depend upon freedb-specific code: if they decide to switch to (say) requests to do network requests, our error-handling goes out the window.

That's true, although we're not not relying on a blindly imported freedb module (the code is already quite customised, just see fdeeed9). Almost 3 years have passed since the last update to that file in the upstream repository.
Anyway I'm not opposing to the way you've implemented the fix, just trying to clarify the situation. I'll wait for your reply then will probably proceed to merge this pull request as is right now. 😉

@anarcat

This comment has been minimized.

Contributor

anarcat commented Oct 6, 2018

@JoeLametta JoeLametta merged commit 021e621 into whipper-team:master Oct 6, 2018

@JoeLametta

This comment has been minimized.

Collaborator

JoeLametta commented Oct 6, 2018

Besides, all this would ideally go away and be abstracted in an external libdiscid right?

Don't know: it seems libdiscid only handles the computation of the IDs not the actual network communications with FreeDB or MusicBrainz (in whipper the former it's handled by the customisedfreedb.py, the latter by python-musicbrainzngs).

@anarcat anarcat deleted the anarcat:freedb-catch branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment