Skip to content

check thread whether joinable before join#2019

Merged
clalancette merged 1 commit intoros2:rollingfrom
uupks:rolling
Oct 13, 2022
Merged

check thread whether joinable before join#2019
clalancette merged 1 commit intoros2:rollingfrom
uupks:rolling

Conversation

@uupks
Copy link
Copy Markdown
Contributor

@uupks uupks commented Oct 4, 2022

If rclcpp is inited with rclcpp::SignalHandlerOptions::None, signal_handler_thread will not be created.

Checking thread whether is joinable before join is a solution to this little bug.

Signed-off-by: uupks <uupks0325@gmail.com>
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya fujitatomoya added the bug Something isn't working label Oct 7, 2022
@fujitatomoya
Copy link
Copy Markdown
Collaborator

This is clearly bug, that could raise std::system_error with Invalid argument.
Test would be ideal.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@clalancette could you take a look at this?

Copy link
Copy Markdown
Collaborator

@alsora alsora left a comment

Choose a reason for hiding this comment

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

Looks correct to me.
+1 on adding a unit test

Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks good to me, fixes an obvious bug.

While adding a test would be nice, it is not at all clear to me how to do that. We don't have any existing tests for the SignalHandler class, and they would be tricky to write (particularly in a portable way). So I'm fine with this going in as-is; I'll run CI on it next.

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Oct 12, 2022

CI:

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

@clalancette
Copy link
Copy Markdown
Contributor

The one failure on Windows is a known flake, so I'm going to go ahead with this. Thanks for the contribution.

@clalancette clalancette merged commit b9b1468 into ros2:rolling Oct 13, 2022
@shuhaowu
Copy link
Copy Markdown

Is there a way to workaround this problem on Humble? The best way I've found is to override the signal handler with a custom one then call rclcpp::shutdown, but this hits this comment:

    // TODO(wjwwood): what happens if someone overrides our signal handler then calls uninstall?
    //   I think we need to assert that we're the current signal handler, and mitigate if not.

So the way to deal with this would be:

  1. Register signal handler in rclcpp::init
  2. Register your own signal handler which overrides the one in rclcpp::init
  3. Let the custom signal handler handle the signal
  4. Call rclcpp::shutdown, which will override the custom signal handler but it's OK at this point.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@shuhaowu that could work, but straightforward way is to backport this to humble?

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@Mergifyio backport humble

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Aug 16, 2023

backport humble

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Aug 16, 2023
Signed-off-by: uupks <uupks0325@gmail.com>
(cherry picked from commit b9b1468)
clalancette pushed a commit that referenced this pull request Sep 6, 2023
Signed-off-by: uupks <uupks0325@gmail.com>
(cherry picked from commit b9b1468)

Co-authored-by: uupks <uupks0325@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants