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

MichaelOrlov · 2024-01-31T03:01:51Z

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 · 2024-03-21T16:07:12Z

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

sangteak601 · 2024-03-22T19:47:22Z

Looks good to me.

ros-discourse · 2024-03-26T00:32:53Z

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 · 2024-03-26T04:06:04Z

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
Linux-aarch64
Windows

fujitatomoya · 2024-03-26T04:40:16Z

@MichaelOrlov i can review this tomorrow.

fujitatomoya

@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

fujitatomoya

lgtm

MichaelOrlov · 2024-03-28T17:29:20Z

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

fujitatomoya

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

rosbag2_py/src/rosbag2_py/_transport.cpp

MichaelOrlov · 2024-04-10T17:39:22Z

@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 · 2024-04-10T19:06:42Z

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
Linux-aarch64
Windows

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

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

MichaelOrlov · 2024-04-11T00:49:40Z

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

MichaelOrlov · 2024-04-11T01:57:44Z

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 · 2024-06-04T18:19:02Z

https://github.com/Mergifyio backport iron humble

mergify · 2024-06-04T18:19:09Z

backport iron humble

✅ Backports have been created

#1690 Gracefully handle SIGINT and SIGTERM signals for play and burst CLI (backport #1557) has been created for branch iron but encountered conflicts
#1691 Gracefully handle SIGINT and SIGTERM signals for play and burst CLI (backport #1557) has been created for branch humble but encountered conflicts

…1557) * Gracefully handle SIGINT and SIGTERM signals for play and burst CLI - 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> * Add test_play_cancel to the test_transport.py Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add missing imports in test_transport.py Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Regenerate Python stub files (.pyi) after altering API Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add call for original deferred signal handler for Player and Recorder Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 32bd5c3) # Conflicts: # rosbag2_py/rosbag2_py/_transport.pyi # rosbag2_py/src/rosbag2_py/_transport.cpp # rosbag2_transport/src/rosbag2_transport/player.cpp

…1557) * Gracefully handle SIGINT and SIGTERM signals for play and burst CLI - 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> * Add test_play_cancel to the test_transport.py Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add missing imports in test_transport.py Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Regenerate Python stub files (.pyi) after altering API Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add call for original deferred signal handler for Player and Recorder Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 32bd5c3) # Conflicts: # rosbag2_py/rosbag2_py/_transport.pyi # rosbag2_py/src/rosbag2_py/_transport.cpp # rosbag2_py/test/test_transport.py # rosbag2_transport/src/rosbag2_transport/player.cpp

…t CLI (backport #1557) (#1690) * Gracefully handle SIGINT and SIGTERM signals for play and burst CLI (#1557) * Gracefully handle SIGINT and SIGTERM signals for play and burst CLI - 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> * Add test_play_cancel to the test_transport.py Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add missing imports in test_transport.py Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Regenerate Python stub files (.pyi) after altering API Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add call for original deferred signal handler for Player and Recorder Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 32bd5c3) # Conflicts: # rosbag2_py/rosbag2_py/_transport.pyi # rosbag2_py/src/rosbag2_py/_transport.cpp # rosbag2_transport/src/rosbag2_transport/player.cpp * Address merge conflicts Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

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 marked this pull request as ready for review March 19, 2024 19:11

MichaelOrlov requested a review from a team as a code owner March 19, 2024 19:11

MichaelOrlov requested review from emersonknapp and hidmic and removed request for a team March 19, 2024 19:11

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 self-assigned this Mar 19, 2024

MichaelOrlov requested a review from fujitatomoya March 19, 2024 19:14

MichaelOrlov force-pushed the morlov/gracefully_handle_signals_in_player branch from 9c71281 to 1e168a1 Compare March 21, 2024 16:04

fujitatomoya reviewed Mar 26, 2024

View reviewed changes

rosbag2_py/src/rosbag2_py/_transport.cpp Outdated Show resolved Hide resolved

fujitatomoya approved these changes Mar 27, 2024

View reviewed changes

fujitatomoya reviewed Mar 29, 2024

View reviewed changes

rosbag2_py/src/rosbag2_py/_transport.cpp Show resolved Hide resolved

rosbag2_py/src/rosbag2_py/_transport.cpp Show resolved Hide resolved

emersonknapp approved these changes Apr 10, 2024

View reviewed changes

clalancette approved these changes Apr 10, 2024

View reviewed changes

MichaelOrlov added 5 commits April 10, 2024 17:47

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>

Add test_play_cancel to the test_transport.py

521d0e5

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Add missing imports in test_transport.py

e00804a

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Regenerate Python stub files (.pyi) after altering API

9cc6eef

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Add call for original deferred signal handler for Player and Recorder

28311ed

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

MichaelOrlov force-pushed the morlov/gracefully_handle_signals_in_player branch from 2909332 to 28311ed Compare April 11, 2024 00:48

MichaelOrlov merged commit 32bd5c3 into rolling Apr 11, 2024
13 of 14 checks passed

delete-merged-branch bot deleted the morlov/gracefully_handle_signals_in_player branch April 11, 2024 01:57

mergify bot mentioned this pull request Jun 4, 2024

[iron] Gracefully handle SIGINT and SIGTERM signals for play and burst CLI (backport #1557) #1690

Merged

mergify bot mentioned this pull request Jun 4, 2024

[humble] Gracefully handle SIGINT and SIGTERM signals for play and burst CLI (backport #1557) #1691

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MichaelOrlov commented Jan 31, 2024 •

edited

Loading

MichaelOrlov commented Mar 21, 2024

sangteak601 commented Mar 22, 2024

ros-discourse commented Mar 26, 2024

MichaelOrlov commented Mar 26, 2024

fujitatomoya commented Mar 26, 2024

fujitatomoya left a comment

fujitatomoya left a comment

MichaelOrlov commented Mar 28, 2024

fujitatomoya left a comment

MichaelOrlov commented Apr 10, 2024

MichaelOrlov commented Apr 10, 2024

MichaelOrlov commented Apr 11, 2024

MichaelOrlov commented Apr 11, 2024

MichaelOrlov commented Jun 4, 2024

mergify bot commented Jun 4, 2024 •

edited

Loading

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

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

Conversation

MichaelOrlov commented Jan 31, 2024 • edited Loading

MichaelOrlov commented Mar 21, 2024

sangteak601 commented Mar 22, 2024

ros-discourse commented Mar 26, 2024

MichaelOrlov commented Mar 26, 2024

fujitatomoya commented Mar 26, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

MichaelOrlov commented Mar 28, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

MichaelOrlov commented Apr 10, 2024

MichaelOrlov commented Apr 10, 2024

MichaelOrlov commented Apr 11, 2024

MichaelOrlov commented Apr 11, 2024

MichaelOrlov commented Jun 4, 2024

mergify bot commented Jun 4, 2024 • edited Loading

✅ Backports have been created

MichaelOrlov commented Jan 31, 2024 •

edited

Loading

mergify bot commented Jun 4, 2024 •

edited

Loading