core: Make sure exceptions from backend's change_track is handled

Also adds TODOs for the rest of the backend calls in playback which all need to
assume backends can and will screw up.

mopidy/core/playback.py

@@ -135,6 +135,7 @@ class PlaybackController(object):
             return self._pending_position
         backend = self._get_backend(self.get_current_tl_track())
         if backend:
+            # TODO: Wrap backend call in error handling.
             return backend.playback.get_time_position().get()
         else:
             return 0
@@ -253,6 +254,7 @@ class PlaybackController(object):
         pending = self.core.tracklist.eot_track(self._current_tl_track)
         while pending:
             # TODO: Avoid infinite loops if all tracks are unplayable.
+            # TODO: Wrap backend call in error handling.
             backend = self._get_backend(pending)
             if backend and backend.playback.change_track(pending.track).get():
                 self._pending_tl_track = pending
@@ -299,6 +301,7 @@ class PlaybackController(object):
     def pause(self):
         """Pause playback."""
         backend = self._get_backend(self.get_current_tl_track())
+        # TODO: Wrap backend call in error handling.
         if not backend or backend.playback.pause().get():
             # TODO: switch to:
             # backend.track(pause)
@@ -366,10 +369,18 @@ class PlaybackController(object):
         if not backend:
             return False
 
+        # TODO: Wrap backend call in error handling.
         backend.playback.prepare_change()
-        if not backend.playback.change_track(pending_tl_track.track).get():
-            return False  # TODO: test for this path
 
+        try:
+            if not backend.playback.change_track(pending_tl_track.track).get():
+                return False
+        except Exception:
+            logger.exception('%s backend caused an exception.',
+                             backend.actor_ref.actor_class.__name__)
+            return False
+
+        # TODO: Wrap backend calls in error handling.
         if state == PlaybackState.PLAYING:
             try:
                 return backend.playback.play().get()
@@ -418,6 +429,7 @@ class PlaybackController(object):
         if self.get_state() != PlaybackState.PAUSED:
             return
         backend = self._get_backend(self.get_current_tl_track())
+        # TODO: Wrap backend call in error handling.
         if backend and backend.playback.resume().get():
             self.set_state(PlaybackState.PLAYING)
             # TODO: trigger via gst messages
@@ -475,6 +487,7 @@ class PlaybackController(object):
         backend = self._get_backend(self.get_current_tl_track())
         if not backend:
             return False
+        # TODO: Wrap backend call in error handling.
         return backend.playback.seek(time_position).get()
 
     def stop(self):
@@ -482,6 +495,7 @@ class PlaybackController(object):
         if self.get_state() != PlaybackState.STOPPED:
             self._last_position = self.get_time_position()
             backend = self._get_backend(self.get_current_tl_track())
+            # TODO: Wrap backend call in error handling.
             if not backend or backend.playback.stop().get():
                 self.set_state(PlaybackState.STOPPED)
 

tests/core/test_playback.py

@@ -13,6 +13,16 @@ from mopidy.models import Track
 from tests import dummy_audio
 
 
+class TestPlaybackProvider(backend.PlaybackProvider):
+    def translate_uri(self, uri):
+        if 'error' in uri:
+            raise Exception(uri)
+        elif 'unplayable' in uri:
+            return None
+        else:
+            return uri
+
+
 # TODO: Replace this with dummy_backend now that it uses a real
 # playbackprovider Since we rely on our DummyAudio to actually emit events we
 # need a "real" backend and not a mock so the right calls make it through to
@@ -22,7 +32,7 @@ class TestBackend(pykka.ThreadingActor, backend.Backend):
 
     def __init__(self, config, audio):
         super(TestBackend, self).__init__()
-        self.playback = backend.PlaybackProvider(audio=audio, backend=self)
+        self.playback = TestPlaybackProvider(audio=audio, backend=self)
 
 
 class BaseTest(unittest.TestCase):
@@ -196,6 +206,36 @@ class TestNextHandling(BaseTest):
 
         assert self.core.playback.get_current_tl_track() == tl_tracks[2]
 
+    def test_next_skips_over_change_track_error(self):
+        # Trigger an exception in translate_uri.
+        track = Track(uri='dummy:error', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play()
+        self.replay_events()
+
+        self.core.playback.next()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[2]
+
+    def test_next_skips_over_change_track_unplayable(self):
+        # Make translate_uri return None.
+        track = Track(uri='dummy:unplayable', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play()
+        self.replay_events()
+
+        self.core.playback.next()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[2]
+
 
 class TestPreviousHandling(BaseTest):
     # TODO Test previous() more
@@ -252,8 +292,38 @@ class TestPreviousHandling(BaseTest):
 
         assert self.core.playback.get_current_tl_track() == tl_tracks[0]
 
+    def test_previous_skips_over_change_track_error(self):
+        # Trigger an exception in translate_uri.
+        track = Track(uri='dummy:error', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
 
-class OnAboutToFinishTest(BaseTest):
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play(tl_tracks[2])
+        self.replay_events()
+
+        self.core.playback.previous()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[0]
+
+    def test_previous_skips_over_change_track_unplayable(self):
+        # Makes translate_uri return None.
+        track = Track(uri='dummy:unplayable', length=1234)
+        self.core.tracklist.add(tracks=[track], at_position=1)
+
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.core.playback.play(tl_tracks[2])
+        self.replay_events()
+
+        self.core.playback.previous()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[0]
+
+
+class TestOnAboutToFinish(BaseTest):
 
     def test_on_about_to_finish_keeps_finished_track_in_tracklist(self):
         tl_track = self.core.tracklist.get_tl_tracks()[0]