From 265951bf0082bfa52073381a5fd9b5ab4c429bef Mon Sep 17 00:00:00 2001 From: Trygve Aaberge Date: Thu, 14 Aug 2014 01:55:51 +0200 Subject: [PATCH 1/5] network: disable_recv before telling actor to close connection As of d62ad96, when the connection can't receive more data from the client, it tells the actor to stop the connection and calls disable_recv(). The actor operates in it's own thread and when it stops the connection, disable_recv is being called again from a different thread. Since the actor is told to stop the connection before disable_recv is called, the two calls to disable_recv may happen simultaneously. This causes a race condition issue where both threads can reach past the check that recv_id is not None before either of them set it to None. If one of them set it to None before the other one tries to use it, an error is raised. This commit calls disable_recv before telling the actor to stop the connection. Since disable_recv is a blocking call, this ensures that recv_id is being set to None before the actor thread begins to stop the connection. Fixes #781 --- docs/changelog.rst | 3 +++ mopidy/utils/network.py | 2 +- tests/utils/network/test_connection.py | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index ec881a92..e242b989 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -21,6 +21,9 @@ Bug fix release. - Configuration: :option:`mopidy --config` now supports directories. +- Network: Fix a race condition where two threads could try to free the same + data simultaneously. (Fixes: :issue:`781`) + v0.19.3 (2014-08-03) ==================== diff --git a/mopidy/utils/network.py b/mopidy/utils/network.py index 11469b47..4ea25026 100644 --- a/mopidy/utils/network.py +++ b/mopidy/utils/network.py @@ -268,8 +268,8 @@ class Connection(object): return True if not data: - self.actor_ref.tell({'close': True}) self.disable_recv() + self.actor_ref.tell({'close': True}) return True try: diff --git a/tests/utils/network/test_connection.py b/tests/utils/network/test_connection.py index 7a25643f..7435353a 100644 --- a/tests/utils/network/test_connection.py +++ b/tests/utils/network/test_connection.py @@ -418,8 +418,8 @@ class ConnectionTest(unittest.TestCase): self.assertTrue(network.Connection.recv_callback( self.mock, sentinel.fd, gobject.IO_IN)) - self.mock.actor_ref.tell.assert_called_once_with({'close': True}) self.mock.disable_recv.assert_called_once_with() + self.mock.actor_ref.tell.assert_called_once_with({'close': True}) def test_recv_callback_recoverable_error(self): self.mock.sock = Mock(spec=socket.SocketType) From 15597b3c604a7b0e0c03a2da79d36f8f51617e64 Mon Sep 17 00:00:00 2001 From: Trygve Aaberge Date: Thu, 14 Aug 2014 12:43:42 +0200 Subject: [PATCH 2/5] tests: Test call order in test_recv_callback_gets_no_data --- tests/utils/network/test_connection.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/utils/network/test_connection.py b/tests/utils/network/test_connection.py index 7435353a..c3200689 100644 --- a/tests/utils/network/test_connection.py +++ b/tests/utils/network/test_connection.py @@ -7,7 +7,7 @@ import unittest import gobject -from mock import Mock, patch, sentinel +from mock import Mock, call, patch, sentinel import pykka @@ -418,8 +418,11 @@ class ConnectionTest(unittest.TestCase): self.assertTrue(network.Connection.recv_callback( self.mock, sentinel.fd, gobject.IO_IN)) - self.mock.disable_recv.assert_called_once_with() - self.mock.actor_ref.tell.assert_called_once_with({'close': True}) + self.assertEqual(self.mock.mock_calls, [ + call.sock.recv(any_int), + call.disable_recv(), + call.actor_ref.tell({'close': True}), + ]) def test_recv_callback_recoverable_error(self): self.mock.sock = Mock(spec=socket.SocketType) From 148451422453d198eb450aa7bd32e55404ac3a73 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 16 Aug 2014 23:04:01 +0200 Subject: [PATCH 3/5] main: Log uncaught exceptions (cherry picked from commit 027b7a53fe9212853062da352c464c4277ade510) --- mopidy/commands.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mopidy/commands.py b/mopidy/commands.py index 2a0b6f48..237ec86b 100644 --- a/mopidy/commands.py +++ b/mopidy/commands.py @@ -280,6 +280,8 @@ class RootCommand(Command): exit_status_code = 1 except KeyboardInterrupt: logger.info('Interrupted. Exiting...') + except Exception: + logger.exception('Uncaught exception') finally: loop.quit() self.stop_frontends(frontend_classes) From 0e60730704d4810fe40e3eaf85639dc5b004ee54 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 29 Aug 2014 13:48:01 +0200 Subject: [PATCH 4/5] backends: Update browse() signature and docs to match core implementation Fixes #833 --- docs/changelog.rst | 4 ++++ mopidy/backend/__init__.py | 4 ++-- mopidy/local/__init__.py | 6 +++--- mopidy/local/library.py | 4 ++-- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index e242b989..6ef9393e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -24,6 +24,10 @@ Bug fix release. - Network: Fix a race condition where two threads could try to free the same data simultaneously. (Fixes: :issue:`781`) +- Backend API: Update :meth:`mopidy.backend.LibraryProvider.browse` signature + and docs to match how the core use the backend's browse method. (Fixes: + :issue:`833`) + v0.19.3 (2014-08-03) ==================== diff --git a/mopidy/backend/__init__.py b/mopidy/backend/__init__.py index 317cf762..b8e37cb2 100644 --- a/mopidy/backend/__init__.py +++ b/mopidy/backend/__init__.py @@ -81,12 +81,12 @@ class LibraryProvider(object): def __init__(self, backend): self.backend = backend - def browse(self, path): + def browse(self, uri): """ See :meth:`mopidy.core.LibraryController.browse`. If you implement this method, make sure to also set - :attr:`root_directory_name`. + :attr:`root_directory`. *MAY be implemented by subclass.* """ diff --git a/mopidy/local/__init__.py b/mopidy/local/__init__.py index 8b4a8b1f..da4d8a55 100644 --- a/mopidy/local/__init__.py +++ b/mopidy/local/__init__.py @@ -64,11 +64,11 @@ class Library(object): def __init__(self, config): self._config = config - def browse(self, path): + def browse(self, uri): """ - Browse directories and tracks at the given path. + Browse directories and tracks at the given URI. - :param string path: path to browse or None for root. + :param string path: URI to browse. :rtype: List of :class:`~mopidy.models.Ref` tracks and directories. """ raise NotImplementedError diff --git a/mopidy/local/library.py b/mopidy/local/library.py index a626f566..65aa4268 100644 --- a/mopidy/local/library.py +++ b/mopidy/local/library.py @@ -18,10 +18,10 @@ class LocalLibraryProvider(backend.LibraryProvider): self._library = library self.refresh() - def browse(self, path): + def browse(self, uri): if not self._library: return [] - return self._library.browse(path) + return self._library.browse(uri) def refresh(self, uri=None): if not self._library: From 69c3e107a20fe9df0f6612cd72b7ba85b420c084 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 29 Aug 2014 14:02:08 +0200 Subject: [PATCH 5/5] local: Add ROOT_DIRECTORY_URI constant Related to #833 --- docs/changelog.rst | 4 ++++ mopidy/local/__init__.py | 12 ++++++++++++ mopidy/local/json.py | 7 +++---- mopidy/local/library.py | 6 +++--- 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 6ef9393e..bc7c0e73 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -28,6 +28,10 @@ Bug fix release. and docs to match how the core use the backend's browse method. (Fixes: :issue:`833`) +- Local library API: Add :attr:`mopidy.local.ROOT_DIRECTORY_URI` constant for + use by implementors of :method:`mopidy.local.Library.browse`. (Related to: + :issue:`833`) + v0.19.3 (2014-08-03) ==================== diff --git a/mopidy/local/__init__.py b/mopidy/local/__init__.py index da4d8a55..7f4cd03f 100644 --- a/mopidy/local/__init__.py +++ b/mopidy/local/__init__.py @@ -46,6 +46,15 @@ class Extension(ext.Extension): return LocalCommand() +ROOT_DIRECTORY_URI = 'local:directory' +""" +URI of the local backend's root directory. + +This constant should be used by libraries implementing the +:meth:`Library.browse` method. +""" + + class Library(object): """ Local library interface. @@ -68,6 +77,9 @@ class Library(object): """ Browse directories and tracks at the given URI. + The URI for the root directory is a constant available at + :attr:`ROOT_DIRECTORY_URI`. + :param string path: URI to browse. :rtype: List of :class:`~mopidy.models.Ref` tracks and directories. """ diff --git a/mopidy/local/json.py b/mopidy/local/json.py index 927f8898..1bb55389 100644 --- a/mopidy/local/json.py +++ b/mopidy/local/json.py @@ -61,8 +61,7 @@ class _BrowseCache(object): splitpath_re = re.compile(r'([^/]+)') def __init__(self, uris): - # TODO: local.ROOT_DIRECTORY_URI - self._cache = {'local:directory': collections.OrderedDict()} + self._cache = {local.ROOT_DIRECTORY_URI: collections.OrderedDict()} for track_uri in uris: path = translator.local_track_uri_to_path(track_uri, b'/') @@ -97,10 +96,10 @@ class _BrowseCache(object): else: # Loop completed, so final child needs to be added to root. if child: - self._cache['local:directory'][child.uri] = child + self._cache[local.ROOT_DIRECTORY_URI][child.uri] = child # If no parent was set we belong in the root. if not parent_uri: - parent_uri = 'local:directory' + parent_uri = local.ROOT_DIRECTORY_URI self._cache[parent_uri][track_uri] = track_ref diff --git a/mopidy/local/library.py b/mopidy/local/library.py index 65aa4268..cf312883 100644 --- a/mopidy/local/library.py +++ b/mopidy/local/library.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals import logging -from mopidy import backend, models +from mopidy import backend, local, models logger = logging.getLogger(__name__) @@ -10,8 +10,8 @@ logger = logging.getLogger(__name__) class LocalLibraryProvider(backend.LibraryProvider): """Proxy library that delegates work to our active local library.""" - root_directory = models.Ref.directory(uri=b'local:directory', - name='Local media') + root_directory = models.Ref.directory( + uri=local.ROOT_DIRECTORY_URI, name='Local media') def __init__(self, backend, library): super(LocalLibraryProvider, self).__init__(backend)