From 66771dec68fed31a6720cb3e348cbc26ba62ecc4 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 3 Apr 2015 20:39:51 +0200 Subject: [PATCH] core: Update LibraryController to catch backend exceptions --- mopidy/core/library.py | 83 +++++++++++++++++++++++++------------- tests/core/test_library.py | 68 +++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 27 deletions(-) diff --git a/mopidy/core/library.py b/mopidy/core/library.py index c787e013..4281b865 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,9 +69,16 @@ class LibraryController(object): .. versionadded:: 0.18 """ if uri is None: + directories = set() 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')) + 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')) scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_library_browse.get(scheme) @@ -96,11 +102,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 +128,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 +185,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] @@ -199,11 +213,20 @@ class LibraryController(object): if uri is not None: backend = self._get_backend(uri) if backend: - backend.library.refresh(uri).get() + try: + backend.library.refresh(uri).get() + except Exception: + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) else: - futures = [b.library.refresh(uri) - for b in self.backends.with_library.values()] - pykka.get_all(futures) + futures = {b: b.library.refresh(uri) + for b in self.backends.with_library.values()} + 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 +296,12 @@ class LibraryController(object): logger.warning( '%s does not implement library.search() with "exact" ' 'support. Please upgrade it.', backend_name) + except LookupError: + 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/tests/core/test_library.py b/tests/core/test_library.py index 8d2195a2..ba6b859e 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -419,3 +419,71 @@ 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_through(self, logger): + # TODO: is this behavior desired? + self.library.browse.return_value.get.side_effect = Exception + with self.assertRaises(Exception): + self.core.library.browse('dummy:directory') + + 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']})