sync signal with future #2998
sync signal with future #2998
Conversation
Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR attempts to replace platform-specific semaphore implementations (Windows, macOS, POSIX) with standard C++ std::future/std::promise for signal synchronization. While this simplification is well-intentioned for maintainability, the implementation has a critical flaw: promises and futures are single-use synchronization primitives that cannot handle multiple signals, whereas semaphores support multiple post/wait cycles.
Key Changes:
- Replaced platform-specific semaphore code with
std::promise<void>andstd::future<void> - Removed Windows (
CreateSemaphore/WaitForSingleObject), macOS (dispatch_semaphore_t), and POSIX (sem_t) specific implementations - Added
<future>and<memory>includes
Critical Issue: After the first signal sets the promise value, the future remains ready permanently. Subsequent wait_for_signal() calls return immediately instead of blocking, creating a busy-wait loop. The previous semaphore implementation correctly handled multiple signal events.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| rclcpp/src/rclcpp/signal_handler.hpp | Updated member variables from platform-specific semaphore types to std::unique_ptr<std::promise<void>> and std::unique_ptr<std::future<void>>; added <future> and <memory> includes |
| rclcpp/src/rclcpp/signal_handler.cpp | Replaced semaphore-based setup_wait_for_signal(), teardown_wait_for_signal(), wait_for_signal(), and notify_signal_handler() implementations with promise/future-based equivalents that have single-use semantics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Tomoya Fujita <[email protected]>
|
@jmachowinski can you take a look? |
|
Pulls: #2998
|
fujitatomoya
changed the title
|
CI fails with |
|
|
I played around with the code and see multiple problems:
I created #2999 as a poc, note it's not tested etc |
|
Pulls: #2998
|
|
@jmachowinski thanks for #2999, i am fine to replace this with it. |
Description
This replaces platform specific signal synchronization into
std::future.This should be better for maintenance and code quality depending on standard library.
Is this user-facing behavior change?
No
Did you use Generative AI?
Yes, GitHub Copilot(Claude Sonnet 4)
Additional Information