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

Jump to bottom

Fix the race condition while calling rcl_shutdown #1353

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Fix the race condition while calling rcl_shutdown #1353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54dcc50
Fix the race condition while calling rcl_shutdown
Barry-Xu-2018 Sep 3, 2024
40329f2
Avoid calling rcl_shutdown() multiple times on the same context
Barry-Xu-2018 Sep 3, 2024
0eb7ee5
Multiple calls to Context::shutdown will throw an exception
Barry-Xu-2018 Sep 9, 2024
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
21 changes: 11 additions & 10 deletions rclpy/src/rclpy/context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,17 +150,18 @@ Context::ok()
void
Context::shutdown()
{
{
std::lock_guard<std::mutex> guard{g_contexts_mutex};
auto iter = std::find(g_contexts.begin(), g_contexts.end(), rcl_context_.get());
if (iter != g_contexts.end()) {
g_contexts.erase(iter);
}
std::lock_guard<std::mutex> guard{g_contexts_mutex};
if (already_shutdown_) {
throw CONTEXT_ALREADY_SHUTDOWN("Context already shutdown.");
}

rcl_ret_t ret = rcl_shutdown(rcl_context_.get());
if (RCL_RET_OK != ret) {
throw RCLError("failed to shutdown");
auto iter = std::find(g_contexts.begin(), g_contexts.end(), rcl_context_.get());
Copy link
Collaborator

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 the g_contexts during this shutdown call, that is the invalid operation? so we can generate the exception without introducing already_shutdown_?

Copy link
Contributor Author

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 the g_contexts during this shutdown call, that is the invalid operation?

Yes.
rc_context_ is added to g_contexts in constructor of Context. Before calling rcl_shutdown(), rc_context_ is removed from g_contexts.

so we can generate the exception without introducing already_shutdown_?

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().

Copy link
Collaborator

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 have already_shutdown_ flag internally...

if (iter != g_contexts.end()) {
g_contexts.erase(iter);
rcl_ret_t ret = rcl_shutdown(rcl_context_.get());
if (RCL_RET_OK != ret) {
throw RCLError("failed to shutdown");
}
already_shutdown_ = true;
}
}

Expand Down
1 change: 1 addition & 0 deletions rclpy/src/rclpy/context.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ class Context : public Destroyable, public std::enable_shared_from_this<Context>

private:
std::shared_ptr<rcl_context_t> rcl_context_;
bool already_shutdown_{false};
};

/// Define a pybind11 wrapper for an rclpy::Context
Expand Down
5 changes: 5 additions & 0 deletions rclpy/src/rclpy/exceptions.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ class InvalidHandle : public std::runtime_error
using std::runtime_error::runtime_error;
};

class CONTEXT_ALREADY_SHUTDOWN : public std::runtime_error
{
using std::runtime_error::runtime_error;
};

} // namespace rclpy

#endif // RCLPY__EXCEPTIONS_HPP_