refactor: Use std::conditional_variable in signal handler code#2999
refactor: Use std::conditional_variable in signal handler code#2999jmachowinski wants to merge 1 commit intoros2:rollingfrom
Conversation
jmachowinski
force-pushed
the
sync_with_conditional
branch
2 times, most recently
from
1c2a717 to
096cc95
Compare
jmachowinski
force-pushed
the
sync_with_conditional
branch
2 times, most recently
from
52646ea to
399f671
Compare
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
@jmachowinski thanks for the fix, i will start the CI.
Note: not only the replacement of custom semaphore notification code for handling signals (SIGINT/SIGTERM) with std::condition_variable and related modern C++ synchronization primitives, but also having dedicated fields (atomic field) for SIGTERM and SIGINT allows the system to reliably record if each signal was received, even if they arrive close together or simultaneously.
|
Pulls: #2999
|
|
Pulls: #2999
|
|
From triage meeting: ci.ros2.org was down, but seems like it could be merged maybe? |
|
@Mergifyio rebase |
❌ Pull request can't be updated with latest base branch changesDetailsMergify needs the author permission to update the base branch of the pull request. |
|
@jmachowinski can you rebase and restart CI? |
Code cleanup to use std::conditional_variable in the signal handler code instead of a custom home brew version. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
jmachowinski
force-pushed
the
sync_with_conditional
branch
from
399f671 to
edee4fc
Compare
|
Pulls: #2999 Started by user Janosch Machowinski Running as SYSTEM
Triggering a new build of ci_windows Triggering a new build of ci_linux-rhel Triggering a new build of ci_linux Finished: SUCCESS |
| RCLCPP_ERROR(get_logger(), "sem_post failed in notify_signal_handler()"); | ||
| } | ||
| #endif | ||
| signal_conditional_.notify_one(); |
There was a problem hiding this comment.
Are you sure this is safe to do from a signal handler? I believe the custom code was used explicitly because there was no standard way to do this safely.
There was a problem hiding this comment.
There was a problem hiding this comment.
wjwwood
left a comment
wjwwood
left a comment
There was a problem hiding this comment.
I don't think this is safe to be honest, see my other comments.
3 participants
Code cleanup to use std::conditional_variable in the signal handler code instead of a custom home brew version.
Description
Reworked version of
#2998