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

Gracefully handle SIGINT and SIGTERM signals for play and burst CLI #1557

Merged
merged 5 commits into from Apr 11, 2024
Merged

Gracefully handle SIGINT and SIGTERM signals for play and burst CLI #1557

merged 5 commits into from Apr 11, 2024

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Jan 31, 2024 •
edited

  • Depends from Fix for false negative tests in rosbag2_py #1592
  • Intercept signals to call Player::stop() instead of relying on the rclcpp::shutdown() in default signal handlers.
  • Also added static Player::Cancel() method.
  • Added test coverage for Cancel during playback. Note: adding unit test coverage for Cancel during burst is not feasible because it runs very fast without delays between messages and without a repetition option.

@MichaelOrlov MichaelOrlov force-pushed the morlov/gracefully_handle_signals_in_player branch 3 times, most recently from fd111a4 to 9c71281 Compare March 19, 2024 19:06
@MichaelOrlov MichaelOrlov marked this pull request as ready for review March 19, 2024 19:11
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner March 19, 2024 19:11
@MichaelOrlov MichaelOrlov requested review from emersonknapp and hidmic and removed request for a team March 19, 2024 19:11
@MichaelOrlov MichaelOrlov changed the title [WIP] Gracefully handle SIGINT and SIGTERM signals for play and burst CLI Gracefully handle SIGINT and SIGTERM signals for play and burst CLI Mar 19, 2024
@MichaelOrlov MichaelOrlov self-assigned this Mar 19, 2024
@MichaelOrlov MichaelOrlov force-pushed the morlov/gracefully_handle_signals_in_player branch from 9c71281 to 1e168a1 Compare March 21, 2024 16:04
@MichaelOrlov
Copy link
Contributor Author

@fujitatomoya @r7vme @sangteak601 I would use a review for this PR.

@sangteak601
Copy link

Looks good to me.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2024-03-21/36814/1

@MichaelOrlov
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/6988fe3cba817e9ea4882a6957d37d28/raw/73ff5df4c6165b9d480abd7caeff0fbb350484b4/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_py rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_py rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13536

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

@fujitatomoya
Copy link
Contributor

@MichaelOrlov i can review this tomorrow.

fujitatomoya
fujitatomoya reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@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.

@MichaelOrlov one thing i want to confirm, probably that can be intention but i really do not see the reason, why it does not call the original signal handler from user application.

rosbag2_py/src/rosbag2_py/_transport.cpp Outdated Show resolved Hide resolved
fujitatomoya
fujitatomoya approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@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

@MichaelOrlov
Copy link
Contributor Author

@clalancette I will need your formal approval or additional review for this PR.

fujitatomoya
fujitatomoya reviewed Mar 29, 2024
View reviewed changes
Copy link
Contributor

@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.

@MichaelOrlov thanks for the fix, i had a couple of minor comments.

rosbag2_py/src/rosbag2_py/_transport.cpp Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_transport.cpp Show resolved Hide resolved
@MichaelOrlov
Copy link
Contributor Author

@clalancette @emersonknapp I need a formal review/approval for this PR.
@fujitatomoya Doesn't have write permissions to the rosbag2 repository and I can't merge with only his approaval.
If you will approve this PR I will re-run CI and if it is green will merge it.

@MichaelOrlov
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/82537b0fd13a1c970b1dc96f41168b65/raw/35835c4132ebe7f1094443d2366e3a27aabaaf48/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_py rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_py rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13670

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

@MichaelOrlov
Gracefully handle SIGINT and SIGTERM signals for play and burst CLI
ef71766 
- Intercept signals to call Player::stop() instead of relying on the
rclcpp::shutdown() in default signal handlers.
- Also added static Player::Cancel() method.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Add test_play_cancel to the test_transport.py
521d0e5 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Add missing imports in test_transport.py
e00804a 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Regenerate Python stub files (.pyi) after altering API
9cc6eef 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Add call for original deferred signal handler for Player and Recorder
28311ed 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov force-pushed the morlov/gracefully_handle_signals_in_player branch from 2909332 to 28311ed Compare April 11, 2024 00:48
@MichaelOrlov
Copy link
Contributor Author

Did rebase before merge to see if RPR and build_test jobs will pass.

@MichaelOrlov
Copy link
Contributor Author

RPR job failed with flake8 errors in unrelated packages. It seems a known issue on the CI and unrelated to this PR. Merging then.

@MichaelOrlov MichaelOrlov merged commit 32bd5c3 into rolling Apr 11, 2024
13 of 14 checks passed
@delete-merged-branch delete-merged-branch bot deleted the morlov/gracefully_handle_signals_in_player branch April 11, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@clalancette clalancette clalancette approved these changes

@emersonknapp emersonknapp emersonknapp approved these changes

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner automatically assigned from ros2/tooling-reviewers

Assignees

@MichaelOrlov MichaelOrlov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MichaelOrlov @sangteak601 @ros-discourse @fujitatomoya @clalancette @emersonknapp