From 54dcc508847b5624aa8364fda16c5f76bde00c5c Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Tue, 3 Sep 2024 11:11:28 +0800 Subject: [PATCH 1/3] Fix the race condition while calling rcl_shutdown Signed-off-by: Barry Xu --- rclpy/src/rclpy/context.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/rclpy/src/rclpy/context.cpp b/rclpy/src/rclpy/context.cpp index 6d7219a79..9a0a0f5a5 100644 --- a/rclpy/src/rclpy/context.cpp +++ b/rclpy/src/rclpy/context.cpp @@ -150,12 +150,10 @@ Context::ok() void Context::shutdown() { - { - std::lock_guard 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 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); } rcl_ret_t ret = rcl_shutdown(rcl_context_.get()); From 40329f28090b70be17422b782d20e02b304e8e09 Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Tue, 3 Sep 2024 14:14:31 +0800 Subject: [PATCH 2/3] Avoid calling rcl_shutdown() multiple times on the same context Signed-off-by: Barry Xu --- rclpy/src/rclpy/context.cpp | 9 ++++----- rclpy/test/test_init_shutdown.py | 9 --------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/rclpy/src/rclpy/context.cpp b/rclpy/src/rclpy/context.cpp index 9a0a0f5a5..2fa5146b6 100644 --- a/rclpy/src/rclpy/context.cpp +++ b/rclpy/src/rclpy/context.cpp @@ -154,11 +154,10 @@ Context::shutdown() auto iter = std::find(g_contexts.begin(), g_contexts.end(), rcl_context_.get()); 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"); + rcl_ret_t ret = rcl_shutdown(rcl_context_.get()); + if (RCL_RET_OK != ret) { + throw RCLError("failed to shutdown"); + } } } diff --git a/rclpy/test/test_init_shutdown.py b/rclpy/test/test_init_shutdown.py index 43a514448..ecce3862e 100644 --- a/rclpy/test/test_init_shutdown.py +++ b/rclpy/test/test_init_shutdown.py @@ -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): - rclpy.shutdown(context=context) - - def test_create_node_without_init(): context = rclpy.context.Context() with pytest.raises(NotInitializedException): From 0eb7ee57731d0b2dbf44288493290e5049b55fc0 Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Mon, 9 Sep 2024 10:29:32 +0800 Subject: [PATCH 3/3] Multiple calls to Context::shutdown will throw an exception Signed-off-by: Barry Xu --- rclpy/src/rclpy/context.cpp | 4 ++++ rclpy/src/rclpy/context.hpp | 1 + rclpy/src/rclpy/exceptions.hpp | 5 +++++ rclpy/test/test_init_shutdown.py | 9 +++++++++ 4 files changed, 19 insertions(+) diff --git a/rclpy/src/rclpy/context.cpp b/rclpy/src/rclpy/context.cpp index 2fa5146b6..079ab02cb 100644 --- a/rclpy/src/rclpy/context.cpp +++ b/rclpy/src/rclpy/context.cpp @@ -151,6 +151,9 @@ void Context::shutdown() { std::lock_guard guard{g_contexts_mutex}; + if (already_shutdown_) { + throw CONTEXT_ALREADY_SHUTDOWN("Context already shutdown."); + } auto iter = std::find(g_contexts.begin(), g_contexts.end(), rcl_context_.get()); if (iter != g_contexts.end()) { g_contexts.erase(iter); @@ -158,6 +161,7 @@ Context::shutdown() if (RCL_RET_OK != ret) { throw RCLError("failed to shutdown"); } + already_shutdown_ = true; } } diff --git a/rclpy/src/rclpy/context.hpp b/rclpy/src/rclpy/context.hpp index cdeb14122..30259e3e4 100644 --- a/rclpy/src/rclpy/context.hpp +++ b/rclpy/src/rclpy/context.hpp @@ -101,6 +101,7 @@ class Context : public Destroyable, public std::enable_shared_from_this private: std::shared_ptr rcl_context_; + bool already_shutdown_{false}; }; /// Define a pybind11 wrapper for an rclpy::Context diff --git a/rclpy/src/rclpy/exceptions.hpp b/rclpy/src/rclpy/exceptions.hpp index 521146fd0..9146b794d 100644 --- a/rclpy/src/rclpy/exceptions.hpp +++ b/rclpy/src/rclpy/exceptions.hpp @@ -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_ diff --git a/rclpy/test/test_init_shutdown.py b/rclpy/test/test_init_shutdown.py index ecce3862e..43a514448 100644 --- a/rclpy/test/test_init_shutdown.py +++ b/rclpy/test/test_init_shutdown.py @@ -74,6 +74,15 @@ 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): + rclpy.shutdown(context=context) + + def test_create_node_without_init(): context = rclpy.context.Context() with pytest.raises(NotInitializedException):