Skip to content

remove I/O from signal handler.#2986

Merged
ahcorde merged 1 commit intorollingfrom
fujitatomoya/signal-handler-deadlock
Nov 19, 2025
Merged

remove I/O from signal handler.#2986
ahcorde merged 1 commit intorollingfrom
fujitatomoya/signal-handler-deadlock

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya commented Nov 16, 2025 •
edited
Loading

Description

Fixes #2982

Is this user-facing behavior change?

Yes this fixes the possible deadlock via IO.

Did you use Generative AI?

No.

Additional Information

Note

backport required for all downstream distros.

@fujitatomoya
remove I/O from signal handler.
08de4eb 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya requested a review from Copilot November 16, 2025 21:33
@fujitatomoya fujitatomoya self-assigned this Nov 16, 2025
fujitatomoya
fujitatomoya commented Nov 16, 2025
View reviewed changes
Copilot AI reviewed Nov 16, 2025
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes I/O operations from signal handlers to fix a potential deadlock issue (issue #2982). The logging calls that were previously executed directly in the signal handler are now deferred to a separate thread.

  • Removed RCLCPP_INFO and RCLCPP_DEBUG calls from the signal handler functions
  • Added signal_number_ atomic variable to pass signal information to the deferred handler
  • Modified signal_handler_common() to accept and store the signal number parameter

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
rclcpp/src/rclcpp/signal_handler.hpp Added signal_number_ atomic member and updated signal_handler_common() signature to accept signal number
rclcpp/src/rclcpp/signal_handler.cpp Removed I/O operations from signal handlers, stored signal number in atomic variable, and moved logging to deferred handler thread

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 16, 2025 •
edited
Loading

Pulls: #2986
Gist: https://gist.githubusercontent.com/fujitatomoya/dc183cb45013b37e59e2f7784377177f/raw/4d26e86d31206a149f509e73b3f95f44fb29e65d/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17501

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya mentioned this pull request Nov 16, 2025
Closed
ahcorde
ahcorde requested changes Nov 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some signal handler. are failing

nugins99
nugins99 approved these changes Nov 17, 2025
View reviewed changes
Copy link
Copy Markdown

@nugins99 nugins99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Thanks!

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 18, 2025

@ahcorde yeah, i will take a look! thanks

@fujitatomoya fujitatomoya mentioned this pull request Nov 18, 2025
Merged
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 18, 2025 •
edited by ahcorde
Loading

Pulls: #2986, ros2/system_tests#576
Gist: https://gist.githubusercontent.com/fujitatomoya/2d25aacc1059cd394750fb9b88193f12/raw/9245a073dfdba8e93a15e000ecb0a0485a265fb1/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp test_rclcpp
TEST args: --packages-above rclcpp test_rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17517

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented Nov 18, 2025

Hm, this patch gets the job done, but It looks like using a std::future would make a cleaner implementation.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 18, 2025

@jmachowinski yeah cannot disagree, i will try that with another PR when i have time or maybe help-wanted since it seems pretty straight-forward change.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 18, 2025 •
edited
Loading

  • Linux-aarch64 Build Status (I do not think previous CI is related to this PR, but just in case.)
  • Linux-rhel Build Status

@ahcorde ahcorde merged commit 95cb964 into rolling Nov 19, 2025
8 of 9 checks passed
@ahcorde ahcorde deleted the fujitatomoya/signal-handler-deadlock branch November 19, 2025 10:37
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 21, 2025

@Mergifyio backport kilted jazzy humble

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 21, 2025 •
edited
Loading

backport kilted jazzy humble

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Nov 21, 2025
@fujitatomoya @mergify
remove I/O from signal handler. (#2986)
7767008 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 95cb964)
@mergify mergify bot mentioned this pull request Nov 21, 2025
Closed
mergify bot pushed a commit that referenced this pull request Nov 21, 2025
@fujitatomoya @mergify
remove I/O from signal handler. (#2986)
e06f685 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 95cb964)
@mergify mergify bot mentioned this pull request Nov 21, 2025
Closed
mergify bot pushed a commit that referenced this pull request Nov 21, 2025
@fujitatomoya @mergify
remove I/O from signal handler. (#2986)
376d404 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 95cb964)

# Conflicts:
#	rclcpp/src/rclcpp/signal_handler.hpp
@mergify mergify bot mentioned this pull request Nov 21, 2025
Closed
This was referenced Nov 21, 2025
Closed
Closed
Closed
fujitatomoya added a commit that referenced this pull request Nov 24, 2025
@fujitatomoya
remove I/O from signal handler.
58c4c40 
Backport of #2986

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya mentioned this pull request Nov 24, 2025
Merged
This was referenced Dec 4, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@ahcorde ahcorde ahcorde approved these changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll

+1 more reviewer

@nugins99 nugins99 nugins99 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fujitatomoya fujitatomoya

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deadlock when handling signals (e.g. SIGTERM)

5 participants

@fujitatomoya @jmachowinski @nugins99 @ahcorde