Repository: beetbox/beets
Pull request: Improve Deezer error handling
## Description

Fixes #4983 

## To Do

<!--
- If you believe one of below checkpoints is not required for the change you
  are submitting, cross it out and check the box nonetheless to let us know.
  For example: - [x] ~Changelog~
- Regarding the changelog, often it makes sense to add your entry only once
  reviewing is finished. That way you might prevent conflicts from other PR's in
  that file, as well as keep the chance high your description fits with the
  latest revision of your feature/fix.
- Regarding documentation, bugfixes often don't require additions to the docs.
- Please remove the descriptive sentences in braces from the enumeration below,
  which helps to unclutter your PR description.
-->

- [x] Documentation. 
- [x] Changelog. 
- [x] Tests. 


Problem statement:
Deezer plugin hangs and errors out instead of gracefully handling timeout errors
Deezer plugin often errors out with a timeout (after waiting for a long time). See the trace below:

```bash
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 382, in _make_request
    self._validate_conn(conn)
  File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 1012, in _validate_conn
    conn.connect()
  File "/usr/lib/python3/dist-packages/urllib3/connection.py", line 411, in connect
    self.sock = ssl_wrap_socket(
  File "/usr/lib/python3/dist-packages/urllib3/util/ssl_.py", line 449, in ssl_wrap_socket
    ssl_sock = _ssl_wrap_socket_impl(
  File "/usr/lib/python3/dist-packages/urllib3/util/ssl_.py", line 493, in _ssl_wrap_socket_impl
    return ssl_context.wrap_socket(sock, server_hostname=server_hostname)
  File "/usr/lib/python3.10/ssl.py", line 513, in wrap_socket
    return self.sslsocket_class._create(
  File "/usr/lib/python3.10/ssl.py", line 1071, in _create
    self.do_handshake()
  File "/usr/lib/python3.10/ssl.py", line 1342, in do_handshake
    self._sslobj.do_handshake()
TimeoutError: [Errno 110] Connection timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/arsaboo/.local/lib/python3.10/site-packages/requests/adapters.py", line 486, in send
    resp = conn.urlopen(
  File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 755, in urlopen
    retries = retries.increment(
  File "/usr/lib/python3/dist-packages/urllib3/util/retry.py", line 532, in increment
    raise six.reraise(type(error), error, _stacktrace)
  File "/usr/lib/python3/dist-packages/six.py", line 719, in reraise
    raise value
  File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 699, in urlopen
    httplib_response = self._make_request(
  File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 385, in _make_request
    self._raise_timeout(err=e, url=url, timeout_value=conn.timeout)
  File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 336, in _raise_timeout
    raise ReadTimeoutError(
urllib3.exceptions.ReadTimeoutError: HTTPSConnectionPool(host='api.deezer.com', port=443): Read timed out. (read timeout=None)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/arsaboo/.local/bin/beet", line 8, in <module>
    sys.exit(main())
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/ui/__init__.py", line 1862, in main
    _raw_main(args)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/ui/__init__.py", line 1849, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/ui/commands.py", line 1384, in import_func
    import_files(lib, paths, query)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/ui/commands.py", line 1315, in import_files
    session.run()
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/importer.py", line 360, in run
    pl.run_parallel(QUEUE_SIZE)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/util/pipeline.py", line 447, in run_parallel
    raise exc_info[1].with_traceback(exc_info[2])
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/util/pipeline.py", line 312, in run
    out = self.coro.send(msg)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/util/pipeline.py", line 195, in coro
    func(*(args + (task,)))
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/importer.py", line 1497, in lookup_candidates
    task.lookup_candidates()
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/importer.py", line 688, in lookup_candidates
    artist, album, prop = autotag.tag_album(
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/autotag/match.py", line 547, in tag_album
    for matched_candidate in hooks.album_candidates(
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/plugins.py", line 579, in decorated
    for v in generator(*args, **kwargs):
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/autotag/hooks.py", line 756, in album_candidates
    yield from plugins.candidates(items, artist, album, va_likely, extra_tags)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/plugins.py", line 390, in candidates
    yield from plugin.candidates(
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/plugins.py", line 759, in candidates
    albums = [self.album_for_id(album_id=r["id"]) for r in results]
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beets/plugins.py", line 759, in <listcomp>
    albums = [self.album_for_id(album_id=r["id"]) for r in results]
  File "/home/arsaboo/.local/lib/python3.10/site-packages/beetsplug/deezer.py", line 111, in album_for_id
    tracks_obj = requests.get(self.album_url + deezer_id + "/tracks").json()
  File "/home/arsaboo/.local/lib/python3.10/site-packages/requests/api.py", line 73, in get
    return request("get", url, params=params, **kwargs)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/requests/api.py", line 59, in request
    return session.request(method=method, url=url, **kwargs)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/requests/sessions.py", line 589, in request
    resp = self.send(prep, **send_kwargs)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/requests/sessions.py", line 703, in send
    r = adapter.send(request, **kwargs)
  File "/home/arsaboo/.local/lib/python3.10/site-packages/requests/adapters.py", line 532, in send
    raise ReadTimeout(e, request=request)
requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='api.deezer.com', port=443): Read timed out. (read timeout=None)
```

Hints from issue discussion:

Commit message: Update changelog.rst

Patch to review:
diff --git a/beetsplug/deezer.py b/beetsplug/deezer.py
index 4c70d841ec..55ee233588 100644
--- a/beetsplug/deezer.py
+++ b/beetsplug/deezer.py
@@ -75,13 +75,12 @@ def album_for_id(self, album_id):
         deezer_id = self._get_id("album", album_id, self.id_regex)
         if deezer_id is None:
             return None
-
-        album_data = requests.get(self.album_url + deezer_id).json()
-        if "error" in album_data:
-            self._log.debug(
-                f"Error fetching album {album_id}: "
-                f"{album_data['error']['message']}"
-            )
+        try:
+            album_data = requests.get(
+                self.album_url + deezer_id, timeout=10
+            ).json()
+        except requests.exceptions as e:
+            self._log.debug("Error fetching album {}\n Error: {}", deezer_id, e)
             return None
         contributors = album_data.get("contributors")
         if contributors is not None:
@@ -108,8 +107,14 @@ def album_for_id(self, album_id):
                 "by {} API: '{}'".format(self.data_source, release_date)
             )
 
-        tracks_obj = requests.get(self.album_url + deezer_id + "/tracks").json()
-        tracks_data = tracks_obj["data"]
+        try:
+            tracks_obj = requests.get(
+                self.album_url + deezer_id + "/tracks", timeout=10
+            ).json()
+            tracks_data = tracks_obj["data"]
+        except requests.exceptions.ReadTimeout:
+            self._log.debug("Timeout fetching album tracks for {}", deezer_id)
+            tracks_data = None
         if not tracks_data:
             return None
         while "next" in tracks_obj:
@@ -192,11 +197,13 @@ def track_for_id(self, track_id=None, track_data=None):
             deezer_id = self._get_id("track", track_id, self.id_regex)
             if deezer_id is None:
                 return None
-            track_data = requests.get(self.track_url + deezer_id).json()
-            if "error" in track_data:
+            try:
+                track_data = requests.get(
+                    self.track_url + deezer_id, timeout=10
+                ).json()
+            except requests.exception as e:
                 self._log.debug(
-                    f"Error fetching track {track_id}: "
-                    f"{track_data['error']['message']}"
+                    "Error fetching track {}\n Error: {}", deezer_id, e
                 )
                 return None
         track = self._get_track(track_data)
@@ -204,9 +211,18 @@ def track_for_id(self, track_id=None, track_data=None):
         # Get album's tracks to set `track.index` (position on the entire
         # release) and `track.medium_total` (total number of tracks on
         # the track's disc).
-        album_tracks_data = requests.get(
-            self.album_url + str(track_data["album"]["id"]) + "/tracks"
-        ).json()["data"]
+        try:
+            album_tracks_data = requests.get(
+                self.album_url + str(track_data["album"]["id"]) + "/tracks",
+                timeout=10,
+            ).json()["data"]
+        except requests.exceptions as e:
+            self._log.debug(
+                "Error fetching album tracks for {} \n Error: {}",
+                track_data["album"]["id"],
+                e,
+            )
+            return None
         medium_total = 0
         for i, track_data in enumerate(album_tracks_data, start=1):
             if track_data["disk_number"] == track.medium:
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 52c681dc06..a83bfbc15d 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -144,6 +144,8 @@ New features:
 
 Bug fixes:
 
+* :doc:`/plugins/deezer`: Improve Deezer plugin error handling and set requests timeout to 10 seconds.
+  :bug:`4983`
 * :doc:`/plugins/spotify`: Add a limit of 3 retries, instead of retrying endlessly when the API is not available.
 * Fix a crash when the Spotify API timeouts or does not return a `Retry-After` interval.
   :bug:`4942`


Code review comments:
diff_hunk: @@ -75,13 +75,12 @@ def album_for_id(self, album_id):
         deezer_id = self._get_id("album", album_id, self.id_regex)
         if deezer_id is None:
             return None
-
-        album_data = requests.get(self.album_url + deezer_id).json()
-        if "error" in album_data:
-            self._log.debug(
-                f"Error fetching album {album_id}: "
-                f"{album_data['error']['message']}"
-            )
+        try:
+            album_data = requests.get(
+                self.album_url + deezer_id, timeout=10
+            ).json()
+        except requests.exceptions as e:
path: beetsplug/deezer.py
text: @user1:
I believe `requests.exceptions` is a module, not an exception.

diff_hunk: @@ -75,13 +75,12 @@ def album_for_id(self, album_id):
         deezer_id = self._get_id("album", album_id, self.id_regex)
         if deezer_id is None:
             return None
-
-        album_data = requests.get(self.album_url + deezer_id).json()
path: beetsplug/deezer.py
text: @user1:
Please correct me if I’m wrong, but it seems like we lost this layer of error checking. That is, it seems like we should still check for the “error” key in the JSON response?

@author:
The try-except block should capture it. If there is an `error` key, it should be caught in `except` block. It is possible that we may have to capture other exceptions (once we know which ones), but that is easy. 

@user1:
Hmm; I don't think I quite see the idea here… what exception will be raised when there is an `error` key? (Is there some other code that checks for the existence of this key, since it doesn't seem to happen here?)

@author:
Ok...I see now what I was missing. I updated the code to handle the `error` key.