diff --git a/rclcpp/include/rclcpp/context.hpp b/rclcpp/include/rclcpp/context.hpp index 1e84912018..56cb47e078 100644 --- a/rclcpp/include/rclcpp/context.hpp +++ b/rclcpp/include/rclcpp/context.hpp @@ -15,6 +15,7 @@ #ifndef RCLCPP__CONTEXT_HPP_ #define RCLCPP__CONTEXT_HPP_ +#include #include #include #include @@ -378,6 +379,7 @@ class Context : public std::enable_shared_from_this std::shared_ptr rcl_context_; rclcpp::InitOptions init_options_; std::string shutdown_reason_; + std::atomic is_shutting_down_{false}; // Keep shared ownership of the global logging mutex. std::shared_ptr logging_mutex_; diff --git a/rclcpp/src/rclcpp/context.cpp b/rclcpp/src/rclcpp/context.cpp index 1a7e381b44..357119cea4 100644 --- a/rclcpp/src/rclcpp/context.cpp +++ b/rclcpp/src/rclcpp/context.cpp @@ -201,6 +201,8 @@ Context::init( throw rclcpp::ContextAlreadyInitialized(); } this->clean_up(); + // allow shutdown() to be called again after re-initialization + is_shutting_down_.store(false); rcl_context_t * context = new rcl_context_t; if (!context) { throw std::runtime_error("failed to allocate memory for rcl context"); @@ -319,6 +321,12 @@ Context::shutdown_reason() const bool Context::shutdown(const std::string & reason) { + // Prevent double-shutdown: the signal handler thread and the main thread + // may both call shutdown() during Ctrl-C. Use an atomic flag to ensure + // only the first call proceeds; subsequent calls return immediately. + if (is_shutting_down_.exchange(true)) { + return false; + } // prevent races std::lock_guard init_lock(init_mutex_); // ensure validity diff --git a/rclcpp/test/rclcpp/test_context.cpp b/rclcpp/test/rclcpp/test_context.cpp index c8779371fe..0374a2d151 100644 --- a/rclcpp/test/rclcpp/test_context.cpp +++ b/rclcpp/test/rclcpp/test_context.cpp @@ -13,6 +13,10 @@ // limitations under the License. #include +#include +#include +#include + #include "rclcpp/context.hpp" #include "rclcpp/rclcpp.hpp" @@ -215,6 +219,75 @@ TEST(TestContext, check_on_shutdown_callback_order_after_del) { EXPECT_TRUE(result[0] == 1 && result[1] == 3 && result[2] == 4 && result[3] == 0); } +// This test checks that a reentrant call to shutdown(), e.g. issued from a +// pre_shutdown callback, returns false instead of running the entire shutdown +// sequence again (which used to recurse into the pre_shutdown callbacks and +// call rcl_shutdown() twice). +TEST(TestContext, reentrant_shutdown_from_pre_shutdown_callback) { + auto context = std::make_shared(); + context->init(0, nullptr); + + size_t pre_shutdown_calls = 0; + size_t on_shutdown_calls = 0; + bool reentrant_shutdown_result = true; + + context->add_pre_shutdown_callback( + [&context, &pre_shutdown_calls, &reentrant_shutdown_result]() { + pre_shutdown_calls++; + reentrant_shutdown_result = context->shutdown("reentrant shutdown"); + }); + context->add_on_shutdown_callback( + [&on_shutdown_calls]() { + on_shutdown_calls++; + }); + + EXPECT_TRUE(context->shutdown("for test")); + EXPECT_FALSE(reentrant_shutdown_result); + EXPECT_EQ(pre_shutdown_calls, 1u); + EXPECT_EQ(on_shutdown_calls, 1u); + EXPECT_FALSE(context->is_valid()); + + // shutdown() must work again after re-initialization + context->init(0, nullptr); + EXPECT_TRUE(context->is_valid()); + EXPECT_TRUE(context->shutdown("for test again")); + EXPECT_FALSE(context->is_valid()); +} + +// This test checks that concurrent calls to shutdown() from multiple threads +// result in exactly one thread running the shutdown sequence, and the other +// threads returning false without error. +TEST(TestContext, concurrent_shutdown) { + auto context = std::make_shared(); + context->init(0, nullptr); + + std::atomic pre_shutdown_calls{0}; + context->add_pre_shutdown_callback( + [&pre_shutdown_calls]() { + pre_shutdown_calls++; + }); + + constexpr size_t num_threads = 8; + std::atomic success_count{0}; + std::vector threads; + threads.reserve(num_threads); + for (size_t i = 0; i < num_threads; ++i) { + threads.emplace_back( + [&context, &success_count]() { + if (context->shutdown("concurrent shutdown")) { + success_count++; + } + }); + } + for (auto & thread : threads) { + thread.join(); + } + + EXPECT_EQ(success_count, 1u); + EXPECT_EQ(pre_shutdown_calls, 1u); + EXPECT_FALSE(context->is_valid()); +} + // This test checks that contexts will be properly destroyed when leaving a scope, after a // guard condition has been created. TEST(TestContext, check_context_destroyed) {