[PATCH][python] dbus.connection: Release signals lock before calling _clean_up_signal_match().
John (J5) Palmieri
johnp at redhat.com
Wed May 30 12:49:42 PDT 2007
Looks good.
On Mon, 2007-05-28 at 19:49 +0100, Simon McVittie wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> This prevents a deadlock when a signal match that's tracking name owner
> changes is removed, causing a match on NameOwnerChanged to be removed too.
> (Debian bug#426412)
>
> Also move more of the tree manipulation inside the lock, to be nice to
> anyone attempting a port to implementations without a GIL (mainly IronPython),
> and add a regression test for the above bug.
> - ---
> dbus/bus.py | 2 +-
> dbus/connection.py | 45 +++++++++++++++++++++++++--------------------
> test/test-client.py | 2 ++
> test/test-signals.py | 34 ++++++++++++++++++++++++----------
> 4 files changed, 52 insertions(+), 31 deletions(-)
>
> diff --git a/dbus/bus.py b/dbus/bus.py
> index 9313289..04180a4 100644
> - --- a/dbus/bus.py
> +++ b/dbus/bus.py
> @@ -147,7 +147,7 @@ class BusConnection(Connection):
> return match
>
> def _clean_up_signal_match(self, match):
> - - # The signals lock must be held.
> + # The signals lock is no longer held here (it was in <= 0.81.0)
> self.remove_match_string(str(match))
> watch = self._signal_sender_matches.pop(match, None)
> if watch is not None:
> diff --git a/dbus/connection.py b/dbus/connection.py
> index 1304058..78ed287 100644
> - --- a/dbus/connection.py
> +++ b/dbus/connection.py
> @@ -245,9 +245,7 @@ class Connection(_Connection):
> mapping member to list of SignalMatch objects."""
>
> self._signals_lock = thread.allocate_lock()
> - - """Lock used to protect signal data structures if doing two
> - - removals at the same time (everything else is atomic, thanks to
> - - the GIL)"""
> + """Lock used to protect signal data structures"""
>
> self.add_message_filter(self.__class__._signal_func)
>
> @@ -389,17 +387,18 @@ class Connection(_Connection):
>
> match = SignalMatch(self, bus_name, path, dbus_interface,
> signal_name, handler_function, **keywords)
> - - by_interface = self._signal_recipients_by_object_path.setdefault(path,
> - - {})
> - - by_member = by_interface.setdefault(dbus_interface, {})
> - - matches = by_member.setdefault(signal_name, [])
>
> - - # make sure nobody is currently manipulating the list
> self._signals_lock.acquire()
> try:
> + by_interface = self._signal_recipients_by_object_path.setdefault(
> + path, {})
> + by_member = by_interface.setdefault(dbus_interface, {})
> + matches = by_member.setdefault(signal_name, [])
> +
> matches.append(match)
> finally:
> self._signals_lock.release()
> +
> return match
>
> def _iter_easy_matches(self, path, dbus_interface, member):
> @@ -450,18 +449,21 @@ class Connection(_Connection):
> 'positional parameters',
> DeprecationWarning, stacklevel=2)
>
> - - by_interface = self._signal_recipients_by_object_path.get(path, None)
> - - if by_interface is None:
> - - return
> - - by_member = by_interface.get(dbus_interface, None)
> - - if by_member is None:
> - - return
> - - matches = by_member.get(signal_name, None)
> - - if matches is None:
> - - return
> + new = []
> + deletions = []
> self._signals_lock.acquire()
> try:
> - - new = []
> + by_interface = self._signal_recipients_by_object_path.get(path,
> + None)
> + if by_interface is None:
> + return
> + by_member = by_interface.get(dbus_interface, None)
> + if by_member is None:
> + return
> + matches = by_member.get(signal_name, None)
> + if matches is None:
> + return
> +
> for match in matches:
> if (handler_or_match is match
> or match.matches_removal_spec(bus_name,
> @@ -470,15 +472,18 @@ class Connection(_Connection):
> signal_name,
> handler_or_match,
> **keywords)):
> - - self._clean_up_signal_match(match)
> + deletions.append(match)
> else:
> new.append(match)
> by_member[signal_name] = new
> finally:
> self._signals_lock.release()
>
> + for match in deletions:
> + self._clean_up_signal_match(match)
> +
> def _clean_up_signal_match(self, match):
> - - # Called with the signals lock held
> + # Now called without the signals lock held (it was held in <= 0.81.0)
> pass
>
> def _signal_func(self, message):
> diff --git a/test/test-client.py b/test/test-client.py
> index 04706df..49c4261 100755
> - --- a/test/test-client.py
> +++ b/test/test-client.py
> @@ -68,6 +68,8 @@ class TestDBusBindings(unittest.TestCase):
> def setUp(self):
> self.bus = dbus.SessionBus()
> self.remote_object = self.bus.get_object(NAME, OBJECT)
> + self.remote_object_follow = self.bus.get_object(NAME, OBJECT,
> + follow_name_owner_changes=True)
> self.iface = dbus.Interface(self.remote_object, IFACE)
>
> def testGObject(self):
> diff --git a/test/test-signals.py b/test/test-signals.py
> index f496de9..797f70c 100644
> - --- a/test/test-signals.py
> +++ b/test/test-signals.py
> @@ -53,10 +53,12 @@ class TestSignals(unittest.TestCase):
> logger.info('setUp()')
> self.bus = dbus.SessionBus()
> self.remote_object = self.bus.get_object("org.freedesktop.DBus.TestSuitePythonService", "/org/freedesktop/DBus/TestSuitePythonObject")
> + self.remote_object_follow = self.bus.get_object("org.freedesktop.DBus.TestSuitePythonService", "/org/freedesktop/DBus/TestSuitePythonObject", follow_name_owner_changes=True)
> self.iface = dbus.Interface(self.remote_object, "org.freedesktop.DBus.TestSuiteInterface")
> + self.iface_follow = dbus.Interface(self.remote_object_follow, "org.freedesktop.DBus.TestSuiteInterface")
> self.in_test = None
>
> - - def signal_test_impl(self, name, test_removal=False):
> + def signal_test_impl(self, iface, name, test_removal=False):
> self.in_test = name
> # using append rather than assignment here to avoid scoping issues
> result = []
> @@ -79,11 +81,11 @@ class TestSignals(unittest.TestCase):
> main_loop.quit()
>
> logger.info('Testing %s', name)
> - - match = self.iface.connect_to_signal('SignalOneString', _signal_handler,
> - - sender_keyword='sender',
> - - path_keyword='path')
> + match = iface.connect_to_signal('SignalOneString', _signal_handler,
> + sender_keyword='sender',
> + path_keyword='path')
> logger.info('Waiting for signal...')
> - - self.iface.EmitSignal('SignalOneString', 0)
> + iface.EmitSignal('SignalOneString', 0)
> source_id = gobject.timeout_add(1000, _timeout_handler)
> main_loop.run()
> if not result:
> @@ -95,7 +97,7 @@ class TestSignals(unittest.TestCase):
> self.in_test = name + '+removed'
> logger.info('Testing %s', name)
> result = []
> - - self.iface.EmitSignal('SignalOneString', 0)
> + iface.EmitSignal('SignalOneString', 0)
> source_id = gobject.timeout_add(1000, _rm_timeout_handler)
> main_loop.run()
> if result:
> @@ -103,16 +105,28 @@ class TestSignals(unittest.TestCase):
> gobject.source_remove(source_id)
>
> def testSignal(self):
> - - self.signal_test_impl('Signal')
> + self.signal_test_impl(self.iface, 'Signal')
>
> def testRemoval(self):
> - - self.signal_test_impl('Removal', True)
> + self.signal_test_impl(self.iface, 'Removal', True)
>
> def testSignalAgain(self):
> - - self.signal_test_impl('SignalAgain')
> + self.signal_test_impl(self.iface, 'SignalAgain')
>
> def testRemovalAgain(self):
> - - self.signal_test_impl('RemovalAgain', True)
> + self.signal_test_impl(self.iface, 'RemovalAgain', True)
> +
> + def testSignalF(self):
> + self.signal_test_impl(self.iface_follow, 'Signal')
> +
> + def testRemovalF(self):
> + self.signal_test_impl(self.iface_follow, 'Removal', True)
> +
> + def testSignalAgainF(self):
> + self.signal_test_impl(self.iface_follow, 'SignalAgain')
> +
> + def testRemovalAgainF(self):
> + self.signal_test_impl(self.iface_follow, 'RemovalAgain', True)
>
> if __name__ == '__main__':
> main_loop = gobject.MainLoop()
> - --
> 1.5.2-rc3.GIT
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: OpenPGP key: http://www.pseudorandom.co.uk/2003/contact/ or pgp.net
>
> iD8DBQFGWyQtWSc8zVUw7HYRAiB+AKDMFyMNLP7UHR9LpKo+AOnj7QbryACfY7d0
> lw69AC7JqjIjNNNQKE8pUWw=
> =RQe1
> -----END PGP SIGNATURE-----
> _______________________________________________
> dbus mailing list
> dbus at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dbus
More information about the dbus
mailing list