From d85caa69175a95c7cb9d2045307a7412b0554284 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 28 May 2021 18:04:26 -0300 Subject: [PATCH 1/4] Call Context._logging_fini() in Context.try_shutdown() Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/context.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/rclpy/rclpy/context.py b/rclpy/rclpy/context.py index f7a45ff8b..f1fa3c72b 100644 --- a/rclpy/rclpy/context.py +++ b/rclpy/rclpy/context.py @@ -101,8 +101,8 @@ def shutdown(self): # imported locally to avoid loading extensions on module import with self.__context, self._lock: self.__context.shutdown() - self._call_on_shutdown_callbacks() - self._logging_fini() + self._call_on_shutdown_callbacks() + self._logging_fini() def try_shutdown(self): """Shutdown this context, if not already shutdown.""" @@ -111,6 +111,7 @@ def try_shutdown(self): if self.__context.ok(): self.__context.shutdown() self._call_on_shutdown_callbacks() + self._logging_fini() def _remove_callback(self, weak_method): self._callbacks.remove(weak_method) @@ -126,18 +127,18 @@ def on_shutdown(self, callback: Callable[[], None]): self._callbacks.append(weakref.WeakMethod(callback, self._remove_callback)) def _logging_fini(self): + # This function must be called with self._lock held. from rclpy.impl.implementation_singleton import rclpy_implementation global g_logging_ref_count - with self._lock: - if self._logging_initialized: - with g_logging_configure_lock: - g_logging_ref_count -= 1 - if g_logging_ref_count == 0: - rclpy_implementation.rclpy_logging_fini() - if g_logging_ref_count < 0: - raise RuntimeError( - 'Unexpected error: logger ref count should never be lower that zero') - self._logging_initialized = False + if self._logging_initialized: + with g_logging_configure_lock: + g_logging_ref_count -= 1 + if g_logging_ref_count == 0: + rclpy_implementation.rclpy_logging_fini() + if g_logging_ref_count < 0: + raise RuntimeError( + 'Unexpected error: logger ref count should never be lower that zero') + self._logging_initialized = False def get_domain_id(self): """Get domain id of context.""" From c9cba6a8a1e1c2d3098b2c4ef124582ca30fd8f9 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 31 May 2021 18:27:55 -0300 Subject: [PATCH 2/4] Possible deadlock Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/context.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rclpy/rclpy/context.py b/rclpy/rclpy/context.py index f1fa3c72b..761eeec24 100644 --- a/rclpy/rclpy/context.py +++ b/rclpy/rclpy/context.py @@ -120,10 +120,10 @@ def on_shutdown(self, callback: Callable[[], None]): """Add a callback to be called on shutdown.""" if not callable(callback): raise TypeError('callback should be a callable, got {}', type(callback)) - with self._callbacks_lock: - if not self.ok(): - callback() - else: + if not self.ok(): + callback() + else: + with self._callbacks_lock: self._callbacks.append(weakref.WeakMethod(callback, self._remove_callback)) def _logging_fini(self): From c2f71b575c43c2834cf7523fe5f38063c91958d0 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 31 May 2021 18:42:40 -0300 Subject: [PATCH 3/4] Avoid unnecessary race Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/context.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/rclpy/rclpy/context.py b/rclpy/rclpy/context.py index 761eeec24..dd45b1856 100644 --- a/rclpy/rclpy/context.py +++ b/rclpy/rclpy/context.py @@ -120,11 +120,12 @@ def on_shutdown(self, callback: Callable[[], None]): """Add a callback to be called on shutdown.""" if not callable(callback): raise TypeError('callback should be a callable, got {}', type(callback)) - if not self.ok(): - callback() - else: - with self._callbacks_lock: - self._callbacks.append(weakref.WeakMethod(callback, self._remove_callback)) + with self.__context, self._lock: + if not self.__context.ok(): + callback() + else: + with self._callbacks_lock: + self._callbacks.append(weakref.WeakMethod(callback, self._remove_callback)) def _logging_fini(self): # This function must be called with self._lock held. From d909a71e46d9c83deac6c2faff0e268a37c3bcd8 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 17 Aug 2021 13:57:36 -0300 Subject: [PATCH 4/4] Address peer review comment Signed-off-by: Ivan Santiago Paunovic --- rclpy/rclpy/context.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/rclpy/rclpy/context.py b/rclpy/rclpy/context.py index dd45b1856..e0e7449c4 100644 --- a/rclpy/rclpy/context.py +++ b/rclpy/rclpy/context.py @@ -36,7 +36,6 @@ class Context: def __init__(self): self._lock = threading.Lock() self._callbacks = [] - self._callbacks_lock = threading.Lock() self._logging_initialized = False @property @@ -89,12 +88,11 @@ def ok(self): return False def _call_on_shutdown_callbacks(self): - with self._callbacks_lock: - for weak_method in self._callbacks: - callback = weak_method() - if callback is not None: - callback() - self._callbacks = [] + for weak_method in self._callbacks: + callback = weak_method() + if callback is not None: + callback() + self._callbacks = [] def shutdown(self): """Shutdown this context.""" @@ -124,8 +122,7 @@ def on_shutdown(self, callback: Callable[[], None]): if not self.__context.ok(): callback() else: - with self._callbacks_lock: - self._callbacks.append(weakref.WeakMethod(callback, self._remove_callback)) + self._callbacks.append(weakref.WeakMethod(callback, self._remove_callback)) def _logging_fini(self): # This function must be called with self._lock held.