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

Install SIGINT handler optionally and call old handler #10

Merged
merged 1 commit into from
Nov 18, 2021
Merged

Install SIGINT handler optionally and call old handler #10

merged 1 commit into from
Nov 18, 2021

Conversation

MichaelOrlov
Copy link
Member

@MichaelOrlov MichaelOrlov commented Nov 11, 2021 •
edited
Loading

Add option in constructor to skip installing internal SIGINT handler.
Add static method restore_buffer_mode_for_stdin() as alternative for signal handler.
Add call for old signal handler if it was installed before constructing KeyboardHandler.
Exiting from process in signal handler only if there was not valid old signal handler.

@MichaelOrlov
Install SIGINT handler optionally and call old handler
33f7c61 
Add option in constructor to skip installing internal SIGINT handler.
Add static method `restore_buffer_mode_for_stdin()` as alternative for
signal handler.
Add call for old signal handler if it was installed before constructing
KeyboardHandler.
Exiting from process in signal handler only if there are was not valid
old signal handler.

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov MichaelOrlov force-pushed the morlov/sigint_optional branch from 75eb9b9 to 33f7c61 Compare November 11, 2021 09:36
@MichaelOrlov MichaelOrlov marked this pull request as ready for review November 11, 2021 09:38
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner November 11, 2021 09:38
@MichaelOrlov MichaelOrlov requested review from emersonknapp, lucashan, lihui815 and a team and removed request for a team November 11, 2021 09:38
@MichaelOrlov
Copy link
Member Author

@ivanpauno Could you please review this PR?
I added two options as we discussed in #9.
One for optionally not installing internal signal handler with ability to use static method restore_buffer_mode_for_stdin() as alternative in external signal handler.
And another one relates to the properly handling old signal handler if internal signal handler has been installed. i.e. signal chaining.

ivanpauno
ivanpauno reviewed Nov 11, 2021
View reviewed changes
Copy link

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

I have one question about compatibility with the windows implementation, otherwise lgtm

keyboard_handler/include/keyboard_handler/keyboard_handler_unix_impl.hpp Show resolved Hide resolved
keyboard_handler/src/keyboard_handler_unix_impl.cpp Show resolved Hide resolved
@MichaelOrlov
Copy link
Member Author

MichaelOrlov commented Nov 12, 2021 •
edited
Loading

Running CI
Gist: https://gist.githubusercontent.com/MichaelOrlov/b65781bf846f8b6e89a3866f941e81a0/raw/b880071de33d28d2576948bff45ceb4d4ce5300c/ros2.repos
BUILD args: --packages-up-to keyboard_handler
TEST args: --packages-select keyboard_handler
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9336/

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

@MichaelOrlov
Copy link
Member Author

For some reason macOS build hasn't started on CI automatically from ci_launhcer run.
Running it one more time manually

  • macOS Build Status

@MichaelOrlov MichaelOrlov mentioned this pull request Nov 16, 2021
Merged
@MichaelOrlov
Copy link
Member Author

@emersonknapp Could you please review/approve this PR.
@ivanpauno has already reviewed it and I think it's good to go. The CI passing green.
I need formal approval from person who has rights to approve this PR to be able to merge it.
This PR is a blocker for ros2/rosbag2#905

@emersonknapp emersonknapp merged commit 1c62ac8 into main Nov 18, 2021
@MichaelOrlov MichaelOrlov deleted the morlov/sigint_optional branch November 19, 2021 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

@emersonknapp emersonknapp emersonknapp approved these changes

@lucashan lucashan Awaiting requested review from lucashan lucashan is a code owner automatically assigned from ros-tooling/reviewers

@lihui815 lihui815 Awaiting requested review from lihui815

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to call SIGINT handler which has already been installed in the system before KeybordHandler
3 participants
@MichaelOrlov @emersonknapp @ivanpauno