From 50f68064be1eb6f174d1ba7fb76cde3f8bb39be4 Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Fri, 3 Apr 2015 20:40:30 +0200
Subject: [PATCH] core: Update PlaylistsController to catch backend exceptions

---
 mopidy/core/playlists.py     | 38 +++++++++++++++++++++++++----------
 tests/core/test_playlists.py | 39 ++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py
index 2c997d84..500713a3 100644
--- a/mopidy/core/playlists.py
+++ b/mopidy/core/playlists.py
@@ -3,8 +3,6 @@ from __future__ import absolute_import, unicode_literals
 import logging
 import urlparse
 
-import pykka
-
 from mopidy.core import listener
 from mopidy.models import Playlist
 from mopidy.utils import deprecation
@@ -32,17 +30,21 @@ class PlaylistsController(object):
         .. versionadded:: 1.0
         """
         futures = {
-            b.actor_ref.actor_class.__name__: b.playlists.as_list()
-            for b in set(self.backends.with_playlists.values())}
+            backend: backend.playlists.as_list()
+            for backend in set(self.backends.with_playlists.values())}
 
         results = []
-        for backend_name, future in futures.items():
+        for backend, future in futures.items():
             try:
                 results.extend(future.get())
             except NotImplementedError:
+                backend_name = backend.actor_ref.actor_class.__name__
                 logger.warning(
                     '%s does not implement playlists.as_list(). '
                     'Please upgrade it.', backend_name)
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
 
         return results
 
@@ -191,6 +193,8 @@ class PlaylistsController(object):
         else:
             return None
 
+    # TODO: there is an inconsistency between library.refresh(uri) and this
+    # call, not sure how to sort this out.
     def refresh(self, uri_scheme=None):
         """
         Refresh the playlists in :attr:`playlists`.
@@ -204,15 +208,27 @@ class PlaylistsController(object):
         :type uri_scheme: string
         """
         if uri_scheme is None:
-            futures = [b.playlists.refresh()
-                       for b in self.backends.with_playlists.values()]
-            pykka.get_all(futures)
-            listener.CoreListener.send('playlists_loaded')
+            futures = {b: b.playlists.refresh()
+                       for b in self.backends.with_playlists.values()}
+            playlists_loaded = False
+            for backend, future in futures.items():
+                try:
+                    future.get()
+                    playlists_loaded = True
+                except Exception:
+                    logger.exception('%s backend caused an exception.',
+                                     backend.actor_ref.actor_class.__name__)
+            if playlists_loaded:
+                listener.CoreListener.send('playlists_loaded')
         else:
             backend = self.backends.with_playlists.get(uri_scheme, None)
             if backend:
-                backend.playlists.refresh().get()
-                listener.CoreListener.send('playlists_loaded')
+                try:
+                    backend.playlists.refresh().get()
+                    listener.CoreListener.send('playlists_loaded')
+                except Exception:
+                    logger.exception('%s backend caused an exception.',
+                                     backend.actor_ref.actor_class.__name__)
 
     def save(self, playlist):
         """
diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py
index 4ca3d6df..1ccc1815 100644
--- a/tests/core/test_playlists.py
+++ b/tests/core/test_playlists.py
@@ -279,3 +279,42 @@ class DeprecatedGetPlaylistsTest(BasePlaylistsTest):
         self.assertEqual(len(result[0].tracks), 0)
         self.assertEqual(result[1].name, 'B')
         self.assertEqual(len(result[1].tracks), 0)
+
+
+@mock.patch('mopidy.core.playlists.logger')
+class BackendFailuresCorePlaylistsTest(unittest.TestCase):
+
+    def setUp(self):  # noqa: N802
+        self.playlists = mock.Mock(spec=backend.PlaylistsProvider)
+
+        self.backend = mock.Mock()
+        self.backend.actor_ref.actor_class.__name__ = 'DummyBackend'
+        self.backend.uri_schemes.get.return_value = ['dummy']
+        self.backend.playlists = self.playlists
+
+        self.core = core.Core(mixer=None, backends=[self.backend])
+
+    def test_as_list_backend_exception_gets_ignored(self, logger):
+        self.playlists.as_list.get.side_effect = Exception
+        self.assertEqual([], self.core.playlists.as_list())
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_get_items_backend_exception_gets_through(self, logger):
+        # TODO: is this behavior desired?
+        self.playlists.get_items.return_value.get.side_effect = Exception
+        with self.assertRaises(Exception):
+            self.core.playlists.get_items('dummy:/1')
+
+    @mock.patch('mopidy.core.listener.CoreListener.send')
+    def test_refresh_backend_exception_gets_ignored(self, send, logger):
+        self.playlists.refresh.return_value.get.side_effect = Exception
+        self.core.playlists.refresh()
+        self.assertFalse(send.called)
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    @mock.patch('mopidy.core.listener.CoreListener.send')
+    def test_refresh_uri_backend_exception_gets_ignored(self, send, logger):
+        self.playlists.refresh.return_value.get.side_effect = Exception
+        self.core.playlists.refresh('dummy')
+        self.assertFalse(send.called)
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')