-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
check thread whether joinable before join #2019
Conversation
Signed-off-by: uupks <uupks0325@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This is clearly bug, that could raise |
@clalancette could you take a look at this? |
There was a problem hiding this 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
There was a problem hiding this 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.
The one failure on Windows is a known flake, so I'm going to go ahead with this. Thanks for the contribution. |
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
So the way to deal with this would be:
|
@shuhaowu that could work, but straightforward way is to backport this to humble? |
@Mergifyio backport humble |
✅ Backports have been created
|
Signed-off-by: uupks <uupks0325@gmail.com> (cherry picked from commit b9b1468)
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.