From 7ab26652925095ea22f485caadffb6351b00c687 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 21:20:24 +0100 Subject: [PATCH 01/10] mpd: Switch MpdSession to using on_event and re-use listener helper. --- mopidy/mpd/actor.py | 6 ++---- mopidy/mpd/session.py | 2 +- tests/mpd/protocol/test_idle.py | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 7439e73f..7770ef60 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -4,7 +4,7 @@ import logging import pykka -from mopidy import exceptions, zeroconf +from mopidy import exceptions, listener, zeroconf from mopidy.core import CoreListener from mopidy.internal import encoding, network, process from mopidy.mpd import session, uri_mapper @@ -57,9 +57,7 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): process.stop_actors_by_class(session.MpdSession) def send_idle(self, subsystem): - listeners = pykka.ActorRegistry.get_by_class(session.MpdSession) - for listener in listeners: - getattr(listener.proxy(), 'on_idle')(subsystem) + listener.send(session.MpdSession, subsystem) def playback_state_changed(self, old_state, new_state): self.send_idle('player') diff --git a/mopidy/mpd/session.py b/mopidy/mpd/session.py index 68550f3b..d484d986 100644 --- a/mopidy/mpd/session.py +++ b/mopidy/mpd/session.py @@ -41,7 +41,7 @@ class MpdSession(network.LineProtocol): self.send_lines(response) - def on_idle(self, subsystem): + def on_event(self, subsystem): self.dispatcher.handle_idle(subsystem) def decode(self, line): diff --git a/tests/mpd/protocol/test_idle.py b/tests/mpd/protocol/test_idle.py index 075da845..f93bf8aa 100644 --- a/tests/mpd/protocol/test_idle.py +++ b/tests/mpd/protocol/test_idle.py @@ -10,7 +10,7 @@ from tests.mpd import protocol class IdleHandlerTest(protocol.BaseTestCase): def idle_event(self, subsystem): - self.session.on_idle(subsystem) + self.session.on_event(subsystem) def assertEqualEvents(self, events): # noqa: N802 self.assertEqual(set(events), self.context.events) From 49b0580c394bf26c8bb4e6efb003fb5b355fc748 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:08:56 +0100 Subject: [PATCH 02/10] mpd: Fix call signature for core playlist_deleted event --- mopidy/mpd/actor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 7770ef60..b7e3ab0d 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -68,7 +68,7 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): def playlist_changed(self, playlist): self.send_idle('stored_playlist') - def playlist_deleted(self, playlist): + def playlist_deleted(self, uri): self.send_idle('stored_playlist') def options_changed(self): From 7d4da4ac8c910d1e900cd695902cff91b015513f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:10:27 +0100 Subject: [PATCH 03/10] mpd: Add integration test for core events and idle --- mopidy/mpd/actor.py | 3 +++ tests/mpd/test_actor.py | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 tests/mpd/test_actor.py diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index b7e3ab0d..af78c331 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -24,6 +24,9 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): self.zeroconf_name = config['mpd']['zeroconf'] self.zeroconf_service = None + self._setup_server(config, core) + + def _setup_server(self, config, core): try: network.Server( self.hostname, self.port, diff --git a/tests/mpd/test_actor.py b/tests/mpd/test_actor.py new file mode 100644 index 00000000..968fbfe7 --- /dev/null +++ b/tests/mpd/test_actor.py @@ -0,0 +1,44 @@ +from __future__ import absolute_import, unicode_literals + +import mock + +import pytest + +from mopidy.mpd import actor + +# NOTE: Should be kept in sync with all events from mopidy.core.listener + + +@pytest.mark.parametrize("event,expected", [ + (['track_playback_paused', 'tl_track', 'time_position'], None), + (['track_playback_resumed', 'tl_track', 'time_position'], None), + (['track_playback_started', 'tl_track'], None), + (['track_playback_ended', 'tl_track', 'time_position'], None), + (['playback_state_changed', 'old_state', 'new_state'], 'player'), + (['tracklist_changed'], 'playlist'), + (['playlists_loaded'], None), + (['playlist_changed', 'playlist'], 'stored_playlist'), + (['playlist_deleted', 'uri'], 'stored_playlist'), + (['options_changed'], 'options'), + (['volume_changed', 'volume'], 'mixer'), + (['mute_changed', 'mute'], 'output'), + (['seeked', 'time_position'], None), + (['stream_title_changed', 'title'], 'playlist'), +]) +def test_idle_hooked_up_correctly(event, expected): + config = {'mpd': {'hostname': 'foobar', + 'port': 1234, + 'zeroconf': None, + 'max_connections': None, + 'connection_timeout': None}} + + with mock.patch.object(actor.MpdFrontend, '_setup_server'): + frontend = actor.MpdFrontend(core=mock.Mock(), config=config) + + with mock.patch('mopidy.listener.send') as send_mock: + frontend.on_event(event[0], **{e: None for e in event[1:]}) + + if expected is None: + assert not send_mock.call_args + else: + send_mock.assert_called_once_with(mock.ANY, expected) From 19daa89e15efa64cc128e742f6f8d3426f1adb0b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:11:55 +0100 Subject: [PATCH 04/10] mpd: Add missing seeked event handling for idle --- mopidy/mpd/actor.py | 3 +++ tests/mpd/test_actor.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index af78c331..9b1cbc1b 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -85,3 +85,6 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): def stream_title_changed(self, title): self.send_idle('playlist') + + def seeked(self, time_position): + self.send_idle('player') diff --git a/tests/mpd/test_actor.py b/tests/mpd/test_actor.py index 968fbfe7..9975e1cc 100644 --- a/tests/mpd/test_actor.py +++ b/tests/mpd/test_actor.py @@ -22,7 +22,7 @@ from mopidy.mpd import actor (['options_changed'], 'options'), (['volume_changed', 'volume'], 'mixer'), (['mute_changed', 'mute'], 'output'), - (['seeked', 'time_position'], None), + (['seeked', 'time_position'], 'player'), (['stream_title_changed', 'title'], 'playlist'), ]) def test_idle_hooked_up_correctly(event, expected): From a086857dd71b605ed7b4ba3419f60a637c82f07e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:16:35 +0100 Subject: [PATCH 05/10] docs: Update changelog with MPD idle fixes --- docs/changelog.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 3bda9237..387a67bf 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -52,6 +52,11 @@ MPD frontend - Start ``songid`` counting at 1 instead of 0 to match the original MPD server. +- Idle events are now emitted on ``seekeded`` events. (Fixes: :issue:`1331`) + +- Event handler for ``playlist_deleted`` has been unbroken. Likely fixes + unreported / diagnosed crashes. + Zeroconf -------- From aa010e03e995d68913a071793f1847a1ac8cd356 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 23:38:55 +0100 Subject: [PATCH 06/10] listener: Try and protect actors against "bad" events --- mopidy/core/listener.py | 3 ++- mopidy/listener.py | 6 +++++- mopidy/mpd/dispatcher.py | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/mopidy/core/listener.py b/mopidy/core/listener.py index 8feb0324..b8ef734d 100644 --- a/mopidy/core/listener.py +++ b/mopidy/core/listener.py @@ -31,7 +31,8 @@ class CoreListener(listener.Listener): :type event: string :param kwargs: any other arguments to the specific event handlers """ - getattr(self, event)(**kwargs) + # Just delegate to parent, entry mostly for docs. + super(CoreListener, self).on_event(event, **kwargs) def track_playback_paused(self, tl_track, time_position): """ diff --git a/mopidy/listener.py b/mopidy/listener.py index 9bcab0e0..b4ea5ec3 100644 --- a/mopidy/listener.py +++ b/mopidy/listener.py @@ -41,4 +41,8 @@ class Listener(object): :type event: string :param kwargs: any other arguments to the specific event handlers """ - getattr(self, event)(**kwargs) + try: + getattr(self, event)(**kwargs) + except Exception: + # Ensure we don't crash the actor due to "bad" events. + logger.exception('Triggering event failed: %s', event) diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index 099a2f18..175d8b32 100644 --- a/mopidy/mpd/dispatcher.py +++ b/mopidy/mpd/dispatcher.py @@ -47,6 +47,7 @@ class MpdDispatcher(object): return self._call_next_filter(request, response, filter_chain) def handle_idle(self, subsystem): + # TODO: validate against mopidy/mpd/protocol/status.SUBSYSTEMS self.context.events.add(subsystem) subsystems = self.context.subscriptions.intersection( From 435ca5064aa5b6c690fe810cdb50f5c3d344b923 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:05:45 +0100 Subject: [PATCH 07/10] listener: Log kwargs in failed send calls --- mopidy/listener.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mopidy/listener.py b/mopidy/listener.py index b4ea5ec3..79c53570 100644 --- a/mopidy/listener.py +++ b/mopidy/listener.py @@ -45,4 +45,5 @@ class Listener(object): getattr(self, event)(**kwargs) except Exception: # Ensure we don't crash the actor due to "bad" events. - logger.exception('Triggering event failed: %s', event) + logger.exception( + 'Triggering event failed: %s(%s)', event, ', '.join(kwargs)) From 07328e7dd2d504279e64fea6e7c263314750374d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:08:33 +0100 Subject: [PATCH 08/10] mpd: Map playlists_loaded to idle event stored_playlist --- mopidy/mpd/actor.py | 3 +++ tests/mpd/test_actor.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 9b1cbc1b..ff2385c8 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -68,6 +68,9 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): def tracklist_changed(self): self.send_idle('playlist') + def playlists_loaded(self): + self.send_idle('stored_playlist') + def playlist_changed(self, playlist): self.send_idle('stored_playlist') diff --git a/tests/mpd/test_actor.py b/tests/mpd/test_actor.py index 9975e1cc..843e46d3 100644 --- a/tests/mpd/test_actor.py +++ b/tests/mpd/test_actor.py @@ -16,7 +16,7 @@ from mopidy.mpd import actor (['track_playback_ended', 'tl_track', 'time_position'], None), (['playback_state_changed', 'old_state', 'new_state'], 'player'), (['tracklist_changed'], 'playlist'), - (['playlists_loaded'], None), + (['playlists_loaded'], 'stored_playlist'), (['playlist_changed', 'playlist'], 'stored_playlist'), (['playlist_deleted', 'uri'], 'stored_playlist'), (['options_changed'], 'options'), From da84c0f59c67fa7c76b8a5d989fce8116c8e04bd Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:22:33 +0100 Subject: [PATCH 09/10] docs: Update changelog for MPD idle fixes --- docs/changelog.rst | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 387a67bf..cf6bc54d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -52,10 +52,17 @@ MPD frontend - Start ``songid`` counting at 1 instead of 0 to match the original MPD server. -- Idle events are now emitted on ``seekeded`` events. (Fixes: :issue:`1331`) +- Idle events are now emitted on ``seekeded`` events. This fix means that + clients relying on ``idle`` events now get notified about seeks. + (Fixes: :issue:`1331` :issue:`1347`) -- Event handler for ``playlist_deleted`` has been unbroken. Likely fixes - unreported / diagnosed crashes. +- Idle events are now emitted on ``playlists_loaded`` events. This fix means + that clients relying on ``idle`` events now get notified about playlist loads. + (Fixes: :issue:`1331` PR: :issue:`1347`) + +- Event handler for ``playlist_deleted`` has been unbroken. This unreported bug + would cause the MPD Frontend to crash preventing any further communication + via the MPD protocol. (PR: :issue:`1347`) Zeroconf -------- @@ -76,6 +83,9 @@ Cleanups - Removed warning if :file:`~/.config/mopidy/settings.py` exists. We stopped using this settings file in 0.14, released in April 2013. +- The ``on_event`` handler in our listener helper now catches exceptions. This + means that any errors in event handling won't crash the actor in question. + Gapless ------- From c2fc313151aeb7fef8abbebaf16be69ba8f5d7f1 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 21:26:43 +0100 Subject: [PATCH 10/10] mpd: Update event handling to warn about unknown events --- mopidy/mpd/actor.py | 57 +++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index ff2385c8..067d20c5 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -11,6 +11,23 @@ from mopidy.mpd import session, uri_mapper logger = logging.getLogger(__name__) +_CORE_EVENTS_TO_IDLE_SUBSYSTEMS = { + 'track_playback_paused': None, + 'track_playback_resumed': None, + 'track_playback_started': None, + 'track_playback_ended': None, + 'playback_state_changed': 'player', + 'tracklist_changed': 'playlist', + 'playlists_loaded': 'stored_playlist', + 'playlist_changed': 'stored_playlist', + 'playlist_deleted': 'stored_playlist', + 'options_changed': 'options', + 'volume_changed': 'mixer', + 'mute_changed': 'output', + 'seeked': 'player', + 'stream_title_changed': 'playlist', +} + class MpdFrontend(pykka.ThreadingActor, CoreListener): @@ -59,35 +76,13 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): process.stop_actors_by_class(session.MpdSession) + def on_event(self, event, **kwargs): + if event not in _CORE_EVENTS_TO_IDLE_SUBSYSTEMS: + logger.warning( + 'Got unexpected event: %s(%s)', event, ', '.join(kwargs)) + else: + self.send_idle(_CORE_EVENTS_TO_IDLE_SUBSYSTEMS[event]) + def send_idle(self, subsystem): - listener.send(session.MpdSession, subsystem) - - def playback_state_changed(self, old_state, new_state): - self.send_idle('player') - - def tracklist_changed(self): - self.send_idle('playlist') - - def playlists_loaded(self): - self.send_idle('stored_playlist') - - def playlist_changed(self, playlist): - self.send_idle('stored_playlist') - - def playlist_deleted(self, uri): - self.send_idle('stored_playlist') - - def options_changed(self): - self.send_idle('options') - - def volume_changed(self, volume): - self.send_idle('mixer') - - def mute_changed(self, mute): - self.send_idle('output') - - def stream_title_changed(self, title): - self.send_idle('playlist') - - def seeked(self, time_position): - self.send_idle('player') + if subsystem: + listener.send(session.MpdSession, subsystem)