Skip to content

use signal safe synchronization with platform specific semaphores #607

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.

Sign up for GitHub

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

Jump to bottom
Merged
wjwwood merged 8 commits into from
Dec 13, 2018
Merged

use signal safe synchronization with platform specific semaphores #607

wjwwood merged 8 commits into from
Dec 13, 2018

Conversation

@wjwwood
Copy link
Member

@wjwwood wjwwood commented Dec 11, 2018

This is a follow up of this comment: #605 (comment)

This is API and ABI neutral so we can merge it at anytime, but my preference would be after #605 but in for Crystal.

@wjwwood wjwwood added enhancement New feature or request in review Waiting for review (Kanban column) labels Dec 11, 2018
@wjwwood wjwwood added this to the crystal milestone Dec 11, 2018
@wjwwood wjwwood self-assigned this Dec 11, 2018
@wjwwood wjwwood requested review from hidmic and sloretz December 11, 2018 06:06
hidmic
hidmic reviewed Dec 11, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Left some comments, thanks for writing this down @wjwwood !

@sloretz
Copy link
Contributor

sloretz commented Dec 11, 2018 •
edited
Loading

It doesn't block this PR, just noting ros2/build_farmer#152 is still present with this PR tested on linux with

colcon test --retest-until-fail 100 --event-handlers console_direct+ --packages-select test_communication --ctest-args -R test_action_client_server__Fibonacci__rclcpp__rmw_fastrtps_cpp\$

It hangs after a few runs.

Edit: oops, ros2/build_farmer#152 (comment)

@wjwwood
Copy link
Member Author

wjwwood commented Dec 11, 2018

It hangs after a few runs.

I'm looking into it.

@tfoote tfoote mentioned this pull request Dec 12, 2018
Closed
@wjwwood
Copy link
Member Author

wjwwood commented Dec 12, 2018

Ok, I tested this a bunch after @sloretz's comment and neither he nor I could get anything to hang on ctrl-c (sigint).

I also tried to address all the feedback, but I also had started refactoring the SignalHandler class into its own files and to clean it up to avoid so much #ifdef'ing in weird places. Sorry for the large diff in that case.

This is ready for re-review! Thanks :)

@jacobperron jacobperron self-requested a review December 12, 2018 02:29
jacobperron
jacobperron reviewed Dec 12, 2018
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Some minor comments/questions.

@wjwwood wjwwood changed the base branch from hidmic/defer-signal-handling to master December 12, 2018 03:03
jacobperron
jacobperron previously approved these changes Dec 12, 2018
View reviewed changes
@wjwwood wjwwood force-pushed the wjwwood/signal_safe_notify branch from 731170d to 74d018d Compare December 12, 2018 03:08
@wjwwood
Copy link
Member Author

wjwwood commented Dec 12, 2018

@jacobperron can you quickly re-review since I addressed more feedback, fixed cpplint, and rebased?

jacobperron
jacobperron approved these changes Dec 12, 2018
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@wjwwood
Copy link
Member Author

wjwwood commented Dec 12, 2018 •
edited by nuclearsandwich
Loading

CI:

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

hidmic
hidmic reviewed Dec 12, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

One additional observation. BTW I cannot reproduce test failures as described in #605 (comment) with this branch.

@wjwwood
Copy link
Member Author

wjwwood commented Dec 12, 2018

Waiting on Windows ci still.

@wjwwood
fix Windows build
dd6a771 
Signed-off-by: William Woodall <[email protected]>
@wjwwood
Copy link
Member Author

wjwwood commented Dec 12, 2018

I had a small bug in the Windows #ifdef, I just fixed it in dd6a771 and retriggered Windows CI only (since it's within an #if defined(_WIN32) block):

Build Status

I'm still looking for another approval if anyone has time.

@wjwwood
actually fix Windows
beaed30 
Signed-off-by: William Woodall <[email protected]>
@wjwwood
Copy link
Member Author

wjwwood commented Dec 12, 2018

Oops, I had to fix one more thing (actually it should have worked since errno is global, but still) in beaed30, but CI hasn't started yet, so it should still be valid to cover both changes.

hidmic
hidmic approved these changes Dec 13, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM too!

@nuclearsandwich
Copy link
Member

nuclearsandwich commented Dec 13, 2018

Build Status

The failing tests here are also failing in the most recent nightlies. Compared to them, this PR introduces no new failing tests. With the green status of other tests and the investigation into the new failures ongoing I would propose that we consider this PR merge-able.

@wjwwood
Copy link
Member Author

wjwwood commented Dec 13, 2018

Sounds good to me, merging.

@wjwwood wjwwood merged commit 2e58dde into master Dec 13, 2018
@wjwwood wjwwood deleted the wjwwood/signal_safe_notify branch December 13, 2018 05:12
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Dec 13, 2018
cho3 pushed a commit to cho3/rclcpp that referenced this pull request Jun 3, 2019
@wjwwood @christopherho-ApexAI
use signal safe synchronization with platform specific semaphores (ro…
7eb4b79 
…s2#607)

* use signal safe synchronization with platform specific semaphores

Signed-off-by: William Woodall <[email protected]>

* addressed feedback and refactored into separate files

Signed-off-by: William Woodall <[email protected]>

* Apply suggestions from code review

Co-Authored-By: wjwwood <[email protected]>

* include what you use (cpplint)

Signed-off-by: William Woodall <[email protected]>

* avoid redundant use of SignalHandler::

Signed-off-by: William Woodall <[email protected]>

* Update rclcpp/src/rclcpp/signal_handler.hpp

Co-Authored-By: wjwwood <[email protected]>

* fix Windows build

Signed-off-by: William Woodall <[email protected]>

* actually fix Windows

Signed-off-by: William Woodall <[email protected]>
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
@ivanpauno
Use keystore root as security root directory, and not contexts folder (…
6ca6545 
…ros2#607)

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@wjwwood wjwwood mentioned this pull request Dec 16, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobperron jacobperron jacobperron approved these changes

@hidmic hidmic hidmic approved these changes

@sloretz sloretz Awaiting requested review from sloretz

Assignees

@wjwwood wjwwood

Labels

enhancement New feature or request

Projects

None yet

Milestone

crystal

Development

Successfully merging this pull request may close these issues.

6 participants

@wjwwood @sloretz @nuclearsandwich @jacobperron @hidmic