Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call Context._logging_fini() in Context.try_shutdown() #800

Merged
merged 4 commits into from
Aug 18, 2021
Merged

Call Context._logging_fini() in Context.try_shutdown() #800

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d85caa6
Call Context._logging_fini() in Context.try_shutdown()
ivanpauno May 28, 2021
c9cba6a
Possible deadlock
ivanpauno May 31, 2021
c2f71b5
Avoid unnecessary race
ivanpauno May 31, 2021
d909a71
Address peer review comment
ivanpauno Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 20 additions & 21 deletions rclpy/rclpy/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -89,20 +88,19 @@ 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."""
# 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ok to do with the lock acquired?

I was thinking maybe leave this outside the lock, leave the lock usage in fini logging but make it an rlock?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling _call_on_shutdown_callbacks() was supposed to be ok, as try_shutdown() was already doing that.
But we were taking the locks in the opposite order in another place, so that doesn't look fine.
c9cba6a, c2f71b5 should fix it.

I was thinking maybe leave this outside the lock, leave the lock usage in fini logging but make it an rlock?

I prefer only using an rlock when it's actually needed and not because of an implementation detail.
g_logging_configure_lock is always locked after self._lock, so there's no deadlock.
Doing all the shutdown process atomically keeping self._lock held sounds like a good idea IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like before this PR on_shutdown and shutdown() did not call callbacks with self._lock acquired, while try_shutdown() did.

Calling callbacks with self._lock seems fine to me because the context is already shutdown when it's called. There doesn't seem to be a useful method on a shutdown context that a callback might want to call. However, it looks like that makes self._callbacks_lock redundant. Anywhere self._callbacks_lock is used self._lock is already held.

I would recommend either making try_shutdown() not call callbacks with self._lock, or removing self._callbacks_lock. I personally like the second option because it means one less lock to keep track of when reviewing PRs to Context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it looks like that makes self._callbacks_lock redundant. Anywhere self._callbacks_lock is used self._lock is already held.

That's true

I would recommend either making try_shutdown() not call callbacks with self._lock, or removing self._callbacks_lock. I personally like the second option because it means one less lock to keep track of when reviewing PRs to Context.

Yes, that makes sense to me.
There's not much benefit of having two locks here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d909a71

self._logging_fini()

def try_shutdown(self):
"""Shutdown this context, if not already shutdown."""
Expand All @@ -111,6 +109,7 @@ def try_shutdown(self):
if self.__context.ok():
self.__context.shutdown()
self._call_on_shutdown_callbacks()
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
self._logging_fini()

def _remove_callback(self, weak_method):
self._callbacks.remove(weak_method)
Expand All @@ -119,25 +118,25 @@ 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():
with self.__context, self._lock:
if not self.__context.ok():
callback()
else:
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."""
Expand Down