From fee3bb166828572270ce4409de14010f8189cfd2 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Fri, 21 Nov 2025 15:30:07 +0900 Subject: [PATCH 1/3] use std::future for signal synchronization for cross-platform. Signed-off-by: Tomoya Fujita --- rclcpp/src/rclcpp/signal_handler.cpp | 95 ++++++---------------------- rclcpp/src/rclcpp/signal_handler.hpp | 21 ++---- 2 files changed, 24 insertions(+), 92 deletions(-) diff --git a/rclcpp/src/rclcpp/signal_handler.cpp b/rclcpp/src/rclcpp/signal_handler.cpp index cf26d06df4..07151ffb09 100644 --- a/rclcpp/src/rclcpp/signal_handler.cpp +++ b/rclcpp/src/rclcpp/signal_handler.cpp @@ -16,19 +16,11 @@ #include #include +#include #include #include #include -// includes for semaphore notification code -#if defined(_WIN32) -#include -#elif defined(__APPLE__) -#include -#else // posix -#include -#endif - #include "rclcpp/logging.hpp" #include "rclcpp/utilities.hpp" #include "rcutils/strerror.h" @@ -287,22 +279,8 @@ SignalHandler::deferred_signal_handler() void SignalHandler::setup_wait_for_signal() { -#if defined(_WIN32) - signal_handler_sem_ = CreateSemaphore( - NULL, // default security attributes - 0, // initial semaphore count - 1, // maximum semaphore count - NULL); // unnamed semaphore - if (NULL == signal_handler_sem_) { - throw std::runtime_error("CreateSemaphore() failed in setup_wait_for_signal()"); - } -#elif defined(__APPLE__) - signal_handler_sem_ = dispatch_semaphore_create(0); -#else // posix - if (-1 == sem_init(&signal_handler_sem_, 0, 0)) { - throw std::runtime_error(std::string("sem_init() failed: ") + strerror(errno)); - } -#endif + signal_promise_ = std::make_unique>(); + signal_future_ = std::make_unique>(signal_promise_->get_future()); wait_for_signal_is_setup_.store(true); } @@ -312,15 +290,8 @@ SignalHandler::teardown_wait_for_signal() noexcept if (!wait_for_signal_is_setup_.exchange(false)) { return; } -#if defined(_WIN32) - CloseHandle(signal_handler_sem_); -#elif defined(__APPLE__) - dispatch_release(signal_handler_sem_); -#else // posix - if (-1 == sem_destroy(&signal_handler_sem_)) { - RCLCPP_ERROR(get_logger(), "invalid semaphore in teardown_wait_for_signal()"); - } -#endif + signal_promise_.reset(); + signal_future_.reset(); } void @@ -330,37 +301,13 @@ SignalHandler::wait_for_signal() RCLCPP_ERROR(get_logger(), "called wait_for_signal() before setup_wait_for_signal()"); return; } -#if defined(_WIN32) - DWORD dw_wait_result = WaitForSingleObject(signal_handler_sem_, INFINITE); - switch (dw_wait_result) { - case WAIT_ABANDONED: - RCLCPP_ERROR( - get_logger(), "WaitForSingleObject() failed in wait_for_signal() with WAIT_ABANDONED: %s", - GetLastError()); - break; - case WAIT_OBJECT_0: - // successful - break; - case WAIT_TIMEOUT: - RCLCPP_ERROR(get_logger(), "WaitForSingleObject() timedout out in wait_for_signal()"); - break; - case WAIT_FAILED: - RCLCPP_ERROR( - get_logger(), "WaitForSingleObject() failed in wait_for_signal(): %s", GetLastError()); - break; - default: - RCLCPP_ERROR( - get_logger(), "WaitForSingleObject() gave unknown return in wait_for_signal(): %s", - GetLastError()); + + try { + // Wait for the future to be signaled + signal_future_->wait(); + } catch (const std::future_error& e) { + RCLCPP_ERROR(get_logger(), "future_error in wait_for_signal(): %s", e.what()); } -#elif defined(__APPLE__) - dispatch_semaphore_wait(signal_handler_sem_, DISPATCH_TIME_FOREVER); -#else // posix - int s; - do { - s = sem_wait(&signal_handler_sem_); - } while (-1 == s && EINTR == errno); -#endif } void @@ -369,18 +316,16 @@ SignalHandler::notify_signal_handler() noexcept if (!wait_for_signal_is_setup_.load()) { return; } -#if defined(_WIN32) - if (!ReleaseSemaphore(signal_handler_sem_, 1, NULL)) { - RCLCPP_ERROR( - get_logger(), "ReleaseSemaphore() failed in notify_signal_handler(): %s", GetLastError()); - } -#elif defined(__APPLE__) - dispatch_semaphore_signal(signal_handler_sem_); -#else // posix - if (-1 == sem_post(&signal_handler_sem_)) { - RCLCPP_ERROR(get_logger(), "sem_post failed in notify_signal_handler()"); + + try { + // Set the promise value to unblock the waiting future + if (signal_promise_) { + signal_promise_->set_value(); + } + } catch (const std::future_error&) { + // Promise may already be set - this is expected on subsequent signals + // No error logging needed as this is normal behavior } -#endif } rclcpp::SignalHandlerOptions diff --git a/rclcpp/src/rclcpp/signal_handler.hpp b/rclcpp/src/rclcpp/signal_handler.hpp index db608b0d10..7e3087cf2d 100644 --- a/rclcpp/src/rclcpp/signal_handler.hpp +++ b/rclcpp/src/rclcpp/signal_handler.hpp @@ -17,21 +17,14 @@ #include #include +#include +#include #include #include #include "rclcpp/logging.hpp" #include "rclcpp/utilities.hpp" -// includes for semaphore notification code -#if defined(_WIN32) -#include -#elif defined(__APPLE__) -#include -#else // posix -#include -#endif - // Determine if sigaction is available #if __APPLE__ || _POSIX_C_SOURCE >= 1 || _XOPEN_SOURCE || _POSIX_SOURCE #define RCLCPP_HAS_SIGACTION @@ -199,14 +192,8 @@ class SignalHandler final // Whether or not the semaphore for wait_for_signal is setup. std::atomic_bool wait_for_signal_is_setup_; - // Storage for the wait_for_signal semaphore. -#if defined(_WIN32) - HANDLE signal_handler_sem_; -#elif defined(__APPLE__) - dispatch_semaphore_t signal_handler_sem_; -#else // posix - sem_t signal_handler_sem_; -#endif + std::unique_ptr> signal_promise_; + std::unique_ptr> signal_future_; }; } // namespace rclcpp From 12d777293c376fe11c498e5445f9aea27a18f0f5 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Fri, 21 Nov 2025 15:36:20 +0900 Subject: [PATCH 2/3] address uncrustify warnings. Signed-off-by: Tomoya Fujita --- rclcpp/src/rclcpp/signal_handler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rclcpp/src/rclcpp/signal_handler.cpp b/rclcpp/src/rclcpp/signal_handler.cpp index 07151ffb09..90a519a845 100644 --- a/rclcpp/src/rclcpp/signal_handler.cpp +++ b/rclcpp/src/rclcpp/signal_handler.cpp @@ -305,7 +305,7 @@ SignalHandler::wait_for_signal() try { // Wait for the future to be signaled signal_future_->wait(); - } catch (const std::future_error& e) { + } catch (const std::future_error & e) { RCLCPP_ERROR(get_logger(), "future_error in wait_for_signal(): %s", e.what()); } } @@ -322,7 +322,7 @@ SignalHandler::notify_signal_handler() noexcept if (signal_promise_) { signal_promise_->set_value(); } - } catch (const std::future_error&) { + } catch (const std::future_error &) { // Promise may already be set - this is expected on subsequent signals // No error logging needed as this is normal behavior } From 5d54ee5baa9b6ddced0155612e86517582e8399a Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Fri, 21 Nov 2025 16:16:00 +0900 Subject: [PATCH 3/3] make sure promise/future is reset for next incoming signal. Signed-off-by: Tomoya Fujita --- rclcpp/src/rclcpp/signal_handler.cpp | 17 ++++++++++++++++- rclcpp/src/rclcpp/signal_handler.hpp | 1 + 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/signal_handler.cpp b/rclcpp/src/rclcpp/signal_handler.cpp index 90a519a845..21f7fbd83c 100644 --- a/rclcpp/src/rclcpp/signal_handler.cpp +++ b/rclcpp/src/rclcpp/signal_handler.cpp @@ -302,9 +302,23 @@ SignalHandler::wait_for_signal() return; } + std::shared_future future_to_wait; + { + std::lock_guard lock(signal_mutex_); + future_to_wait = signal_future_->share(); + } + try { // Wait for the future to be signaled - signal_future_->wait(); + future_to_wait.wait(); + + // After being signaled, create a new promise/future pair for the next signal + // This allows multiple signals to be handled properly + { + std::lock_guard lock(signal_mutex_); + signal_promise_ = std::make_unique>(); + signal_future_ = std::make_unique>(signal_promise_->get_future()); + } } catch (const std::future_error & e) { RCLCPP_ERROR(get_logger(), "future_error in wait_for_signal(): %s", e.what()); } @@ -318,6 +332,7 @@ SignalHandler::notify_signal_handler() noexcept } try { + std::lock_guard lock(signal_mutex_); // Set the promise value to unblock the waiting future if (signal_promise_) { signal_promise_->set_value(); diff --git a/rclcpp/src/rclcpp/signal_handler.hpp b/rclcpp/src/rclcpp/signal_handler.hpp index 7e3087cf2d..e383a62113 100644 --- a/rclcpp/src/rclcpp/signal_handler.hpp +++ b/rclcpp/src/rclcpp/signal_handler.hpp @@ -192,6 +192,7 @@ class SignalHandler final // Whether or not the semaphore for wait_for_signal is setup. std::atomic_bool wait_for_signal_is_setup_; + std::mutex signal_mutex_; std::unique_ptr> signal_promise_; std::unique_ptr> signal_future_; };