diff --git a/docs/changelog.rst b/docs/changelog.rst index 60d011d7..b0574246 100644 --- a/docs/changelog.rst +++ b/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 ------ diff --git a/mopidy/core/library.py b/mopidy/core/library.py index ae6e63ad..6fc1ce38 100644 --- a/mopidy/core/library.py +++ b/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] diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index 2c997d84..62001517 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`. @@ -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): """ diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 8d2195a2..89f3b284 100644 --- a/tests/core/test_library.py +++ b/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']}) 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')