Merge pull request #1111 from adamcik/feature/make-core-more-robust

Make core more robust

docs/changelog.rst

@@ -13,6 +13,9 @@ Core API
 - Calling :meth:`mopidy.core.library.LibraryController.search`` with ``kwargs``
   as the query is no longer supported (PR: :issue:`1090`)
 
+- Update core controllers to handle backend exceptions in all calls that rely
+  on multiple backends. (Issue: :issue:`667`)
+
 Models
 ------
 

mopidy/core/library.py

@@ -5,7 +5,6 @@ import logging
 import operator
 import urlparse
 
-import pykka
 
 from mopidy.utils import deprecation
 
@@ -70,15 +69,31 @@ class LibraryController(object):
         .. versionadded:: 0.18
         """
         if uri is None:
-            backends = self.backends.with_library_browse.values()
-            unique_dirs = {b.library.root_directory.get() for b in backends}
-            return sorted(unique_dirs, key=operator.attrgetter('name'))
+            return self._roots()
+        return self._browse(uri)
 
+    def _roots(self):
+        directories = set()
+        backends = self.backends.with_library_browse.values()
+        futures = {b: b.library.root_directory for b in backends}
+        for backend, future in futures.items():
+            try:
+                directories.add(future.get())
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
+        return sorted(directories, key=operator.attrgetter('name'))
+
+    def _browse(self, uri):
         scheme = urlparse.urlparse(uri).scheme
         backend = self.backends.with_library_browse.get(scheme)
-        if not backend:
-            return []
-        return backend.library.browse(uri).get()
+        try:
+            if backend:
+                return backend.library.browse(uri).get()
+        except Exception:
+            logger.exception('%s backend caused an exception.',
+                             backend.actor_ref.actor_class.__name__)
+        return []
 
     def get_distinct(self, field, query=None):
         """
@@ -96,11 +111,15 @@ class LibraryController(object):
 
         .. versionadded:: 1.0
         """
-        futures = [b.library.get_distinct(field, query)
-                   for b in self.backends.with_library.values()]
         result = set()
-        for r in pykka.get_all(futures):
-            result.update(r)
+        futures = {b: b.library.get_distinct(field, query)
+                   for b in self.backends.with_library.values()}
+        for backend, future in futures.items():
+            try:
+                result.update(future.get())
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
         return result
 
     def get_images(self, uris):
@@ -118,15 +137,19 @@ class LibraryController(object):
 
         .. versionadded:: 1.0
         """
-        futures = [
-            backend.library.get_images(backend_uris)
+        futures = {
+            backend: backend.library.get_images(backend_uris)
             for (backend, backend_uris)
-            in self._get_backends_to_uris(uris).items() if backend_uris]
+            in self._get_backends_to_uris(uris).items() if backend_uris}
 
         results = {uri: tuple() for uri in uris}
-        for r in pykka.get_all(futures):
-            for uri, images in r.items():
-                results[uri] += tuple(images)
+        for backend, future in futures.items():
+            try:
+                for uri, images in future.get().items():
+                    results[uri] += tuple(images)
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
         return results
 
     def find_exact(self, query=None, uris=None, **kwargs):
@@ -171,19 +194,19 @@ class LibraryController(object):
             uris = [uri]
 
         futures = {}
-        result = {}
-        backends = self._get_backends_to_uris(uris)
+        result = {u: [] for u in uris}
 
         # TODO: lookup(uris) to backend APIs
-        for backend, backend_uris in backends.items():
-            for u in backend_uris or []:
-                futures[u] = backend.library.lookup(u)
+        for backend, backend_uris in self._get_backends_to_uris(uris).items():
+            for u in backend_uris:
+                futures[(backend, u)] = backend.library.lookup(u)
 
-        for u in uris:
-            if u in futures:
-                result[u] = futures[u].get()
-            else:
-                result[u] = []
+        for (backend, u), future in futures.items():
+            try:
+                result[u] = future.get()
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
 
         if uri:
             return result[uri]
@@ -196,14 +219,23 @@ class LibraryController(object):
         :param uri: directory or track URI
         :type uri: string
         """
-        if uri is not None:
-            backend = self._get_backend(uri)
-            if backend:
-                backend.library.refresh(uri).get()
-        else:
-            futures = [b.library.refresh(uri)
-                       for b in self.backends.with_library.values()]
-            pykka.get_all(futures)
+        futures = {}
+        backends = {}
+        uri_scheme = urlparse.urlparse(uri).scheme if uri else None
+
+        for backend_scheme, backend in self.backends.with_playlists.items():
+            backends.setdefault(backend, set()).add(backend_scheme)
+
+        for backend, backend_schemes in backends.items():
+            if uri_scheme is None or uri_scheme in backend_schemes:
+                futures[backend] = backend.library.refresh(uri)
+
+        for backend, future in futures.items():
+            try:
+                future.get()
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
 
     def search(self, query=None, uris=None, exact=False, **kwargs):
         """
@@ -273,6 +305,15 @@ class LibraryController(object):
                 logger.warning(
                     '%s does not implement library.search() with "exact" '
                     'support. Please upgrade it.', backend_name)
+            except LookupError:
+                # Some of our tests check for this to catch bad queries. This
+                # is silly and should be replaced with query validation before
+                # passing it to the backends.
+                raise
+            except Exception:
+                logger.exception('%s backend caused an exception.',
+                                 backend.actor_ref.actor_class.__name__)
+
         return [r for r in results if r]
 
 

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`.
@@ -203,16 +207,27 @@ class PlaylistsController(object):
         :param uri_scheme: limit to the backend matching the URI scheme
         :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)
+        futures = {}
+        backends = {}
+        playlists_loaded = False
+
+        for backend_scheme, backend in self.backends.with_playlists.items():
+            backends.setdefault(backend, set()).add(backend_scheme)
+
+        for backend, backend_schemes in backends.items():
+            if uri_scheme is None or uri_scheme in backend_schemes:
+                futures[backend] = backend.playlists.refresh()
+
+        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')
 
     def save(self, playlist):
         """

tests/core/test_library.py

@@ -190,8 +190,8 @@ class CoreLibraryTest(BaseCoreLibraryTest):
     def test_refresh_without_uri_calls_all_backends(self):
         self.core.library.refresh()
 
-        self.library1.refresh.assert_called_once_with(None)
-        self.library2.refresh.assert_called_twice_with(None)
+        self.library1.refresh.return_value.get.assert_called_once_with()
+        self.library2.refresh.return_value.get.assert_called_once_with()
 
     def test_search_combines_results_from_all_backends(self):
         track1 = Track(uri='dummy1:a')
@@ -419,3 +419,70 @@ class LegacyFindExactToSearchLibraryTest(unittest.TestCase):
         self.backend.library.search.return_value.get.side_effect = TypeError
         self.core.library.search(query={'any': ['a']}, exact=True)
         # We are just testing that this doesn't fail.
+
+
+@mock.patch('mopidy.core.library.logger')
+class BackendFailuresCoreLibraryTest(unittest.TestCase):
+
+    def setUp(self):  # noqa: N802
+        dummy_root = Ref.directory(uri='dummy:directory', name='dummy')
+
+        self.library = mock.Mock(spec=backend.LibraryProvider)
+        self.library.root_directory.get.return_value = dummy_root
+
+        self.backend = mock.Mock()
+        self.backend.actor_ref.actor_class.__name__ = 'DummyBackend'
+        self.backend.uri_schemes.get.return_value = ['dummy']
+        self.backend.library = self.library
+
+        self.core = core.Core(mixer=None, backends=[self.backend])
+
+    def test_browse_backend_get_root_exception_gets_ignored(self, logger):
+        # Might happen if root_directory is a property for some weird reason.
+        self.library.root_directory.get.side_effect = Exception
+        self.assertEqual([], self.core.library.browse(None))
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_browse_backend_browse_uri_exception_gets_ignored(self, logger):
+        self.library.browse.return_value.get.side_effect = Exception
+        self.assertEqual([], self.core.library.browse('dummy:directory'))
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_get_distinct_backend_exception_gets_ignored(self, logger):
+        self.library.get_distinct.return_value.get.side_effect = Exception
+        self.assertEqual(set(), self.core.library.get_distinct('artist'))
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_get_images_backend_exception_get_ignored(self, logger):
+        self.library.get_images.return_value.get.side_effect = Exception
+        self.assertEqual(
+            {'dummy:/1': tuple()}, self.core.library.get_images(['dummy:/1']))
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_lookup_backend_exceptiosn_gets_ignores(self, logger):
+        self.library.lookup.return_value.get.side_effect = Exception
+        self.assertEqual(
+            {'dummy:/1': []}, self.core.library.lookup(uris=['dummy:/1']))
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_refresh_backend_exception_gets_ignored(self, logger):
+        self.library.refresh.return_value.get.side_effect = Exception
+        self.core.library.refresh()
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_refresh_uri_backend_exception_gets_ignored(self, logger):
+        self.library.refresh.return_value.get.side_effect = Exception
+        self.core.library.refresh('dummy:/1')
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_search_backend_exception_gets_ignored(self, logger):
+        self.library.search.return_value.get.side_effect = Exception
+        self.assertEqual([], self.core.library.search(query={'any': ['foo']}))
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    def test_search_backend_lookup_error_gets_through(self, logger):
+        # TODO: is this behavior desired? Do we need to continue handling
+        # LookupError case specially.
+        self.library.search.return_value.get.side_effect = LookupError
+        with self.assertRaises(LookupError):
+            self.core.library.search(query={'any': ['foo']})

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')