Skip to content
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

use signal safe synchronization with platform specific semaphores #607

Merged
merged 8 commits into from
Dec 13, 2018
Merged

use signal safe synchronization with platform specific semaphores #607

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
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 !

rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/utilities.cpp Outdated Show resolved Hide resolved
@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.

rclcpp/src/rclcpp/signal_handler.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/signal_handler.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/signal_handler.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/signal_handler.cpp Show resolved Hide resolved
rclcpp/src/rclcpp/signal_handler.cpp Show resolved Hide resolved
@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 and others added 3 commits December 11, 2018 19:07
@wjwwood
use signal safe synchronization with platform specific semaphores
1e8c1df 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
addressed feedback and refactored into separate files
e73e040 
Signed-off-by: William Woodall <william@osrfoundation.org>
@jacobperron @wjwwood
Apply suggestions from code review
74d018d 
Co-Authored-By: wjwwood <william+github@osrfoundation.org>
@wjwwood
include what you use (cpplint)
83c567c 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
avoid redundant use of SignalHandler::
f2bf339 
Signed-off-by: William Woodall <william@osrfoundation.org>
@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

rclcpp/src/rclcpp/signal_handler.hpp Outdated Show resolved Hide resolved
@jacobperron @wjwwood
Update rclcpp/src/rclcpp/signal_handler.hpp
e7ec34b 
Co-Authored-By: wjwwood <william+github@osrfoundation.org>
@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.

rclcpp/src/rclcpp/signal_handler.cpp Show resolved Hide resolved
rclcpp/src/rclcpp/signal_handler.cpp Show resolved Hide resolved
@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 <william@osrfoundation.org>
@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 <william@osrfoundation.org>
@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

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 <william@osrfoundation.org>

* addressed feedback and refactored into separate files

Signed-off-by: William Woodall <william@osrfoundation.org>

* Apply suggestions from code review

Co-Authored-By: wjwwood <william+github@osrfoundation.org>

* include what you use (cpplint)

Signed-off-by: William Woodall <william@osrfoundation.org>

* avoid redundant use of SignalHandler::

Signed-off-by: William Woodall <william@osrfoundation.org>

* Update rclcpp/src/rclcpp/signal_handler.hpp

Co-Authored-By: wjwwood <william+github@osrfoundation.org>

* fix Windows build

Signed-off-by: William Woodall <william@osrfoundation.org>

* actually fix Windows

Signed-off-by: William Woodall <william@osrfoundation.org>
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 <ivanpauno@ekumenlabs.com>
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.

None yet
5 participants
@wjwwood @sloretz @nuclearsandwich @jacobperron @hidmic