-
Notifications
You must be signed in to change notification settings - Fork 225
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.
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
Fix the race condition while calling rcl_shutdown #1353
base: rolling
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,15 +74,6 @@ def test_double_init(): | |
rclpy.shutdown(context=context) | ||
|
||
|
||
def test_double_shutdown(): | ||
context = rclpy.context.Context() | ||
rclpy.init(context=context) | ||
assert context.ok() | ||
rclpy.shutdown(context=context) | ||
with pytest.raises(RuntimeError): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to remove this test? I think we can keep this test, but expects There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your comments.
With the current fix, if the context is not in g_contexts, rcl_shutdown will not be called. According to the issue described in #1352, it is indeed possible for two threads to call rcl_shutdown on the same context. However, one call is from rclpy::shutdown() and the other from Context::shutdown(). In this situation, we do not want to see an exception being thrown. The test calls Context::shutdown() twice, and it is reasonable to expect an exception on the second call. Therefore, I am considering setting a flag variable in the Context class. If Context::shutdown() has been successfully called, this variable will be set. If Context::shutdown() is called again, it will check if the flag has been set and, if so, will throw an exception CONTEXT_ALREADY_SHUTDOWN (not from RCL). What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please review 0eb7ee5 |
||
rclpy.shutdown(context=context) | ||
|
||
|
||
def test_create_node_without_init(): | ||
context = rclpy.context.Context() | ||
with pytest.raises(NotInitializedException): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the
g_contexts
is the collection of valid context, and this means if it cannot find the context in theg_contexts
during this shutdown call, that is the invalid operation? so we can generate the exception without introducingalready_shutdown_
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
rc_context_
is added to g_contexts in constructor of Context. Before calling rcl_shutdown(),rc_context_
is removed from g_contexts.already_shutdown_
is only used for multiple calls to Context::shutdown() to throw the exception.About issue reported by #1352, it describes a possible scenario. There are two threads to call rcl_shutdown for the same context. One call is from rclpy::shutdown() and the other from Context::shutdown(). In this situation, we do not want to get an exception in Context::shutdown().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is exactly why we have
std::lock_guard<std::mutex> guard{g_contexts_mutex};
lock escalated to the this function scope? so that when rclpy::shutdown_contexts is under process, we will not find the iterator with this context, so generate the exception. i really do not understand to havealready_shutdown_
flag internally...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fujitatomoya
Let me first summarize the two scenarios we currently need to address:
rcl_shutdown
is not thread-safe when used in bothsignal_handler
andContext.__exit__
. #1352. There are two threads to call rcl_shutdown for the same context. One call is from rclpy::shutdown() and the other from Context::shutdown(). For this scenario, we don't want to get exception.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fujitatomoya Friendly ping.