Gracefully stop recording upon SIGTERM and SIGINT by dabonnie · Pull Request #2038 · ros/ros_comm

dabonnie · 2020-09-04T21:20:47Z

Resolves #2006

Add handling for SIGTERM in recorder
Preserve handling for SIGINT in recorder

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

Add unit test for rosbag record SIGTERM handling Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie · 2020-09-04T21:21:11Z

@dirk-thomas could you please review?

dirk-thomas · 2020-09-04T21:37:43Z

The default signal handler calls ros::requestShutdown() since it needs other threads to gracefully end their execution. I don't sync synchronously calling ros::shutdown() in the signal handler is a viable option. Other threads are likely still using the various singleton managers.

dabonnie · 2020-09-04T21:48:41Z

The default signal handler calls ros::requestShutdown() since it needs other threads to gracefully end their execution. I don't sync synchronously calling ros::shutdown() in the signal handler is a viable option. Other threads are likely still using the various singleton managers.

Thanks for the info. I will ensure that the default behavior happens for the SIGINT case and will test for the SIGTERM case.

dabonnie · 2020-09-04T22:48:07Z

@dirk-thomas is this incorrect? This is where I thought the default behavior was calling shutdown. FWIW using ros::requestShutdown() for both SIGINT and SIGTERM yields a foo.bag.active file.

thomas-moulard · 2020-09-11T18:19:43Z

@dabonnie PTAL at man 7 signal-safety : https://man7.org/linux/man-pages/man7/signal-safety.7.html

There are many functions you should not call from a signal handler, and generally signal handlers should not be blocking. You can call requestShutdown in sigterm too if you want, but for the rest, we should patch rosbag so that when rospy.is_shutting_down is true, we properly process the latest active bag file.

dabonnie · 2020-09-12T00:16:56Z

Note: sending kill -2 $(pgrep rosbag) where, per kill -l 2) SIGINT, also results in bad behavior: the bag file is left active and the child record process is left running. So, to reproduce:

Run roscore in terminal window a
Run rosbag record -a in terminal window b
Run kill -2 $(pgrep rosbag) in terminal window c
Run pgrep record in terminal window b or c, notice a returned PID
Run ls *.bag in terminal window b, notice an active bag file

Fix sending SIGINT to main process Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie · 2020-09-12T01:18:08Z

@dirk-thomas addressed feedback we discussed earlier today.

tools/rosbag/src/record.cpp

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie · 2020-09-17T16:15:28Z

@dirk-thomas test failure seems unrelated?

test/test_rosbag/test/record_cleanup.py

tools/rosbag/src/record.cpp

tools/rosbag/src/rosbag/rosbag_main.py

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie · 2020-09-22T18:10:33Z

@dirk-thomas addressed review comments and green CI, please review.

test/test_rosbag/test/record_cleanup.py

tools/rosbag/src/record.cpp

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

test/test_rosbag/CMakeLists.txt

Add record cleanup unit test helper Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

tools/rosbag/src/record.cpp

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dirk-thomas · 2020-09-23T20:48:28Z

@ros-pull-request-builder retest this please

dirk-thomas · 2020-09-23T22:17:10Z

Thanks for the patch and for iterating on it!

* Add SIGTERM and SIGINT handlers to rosbag record Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Add unit test for rosbag record SIGINT handling Add unit test for rosbag record SIGTERM handling Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Address review comments Fix sending SIGINT to main process Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Revert added whitespace Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Revert SIGINT handler addition: use default Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Remove unnecessary wait Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Use BSD License Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Add test improvements Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Move test helper function Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Remove redundant test rosbag launch Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Add Amazon to new python test copyright Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Remove unrelated whitespace Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Split record cleanup unit tests Add record cleanup unit test helper Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Revert whitespace change Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * revert white space change Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Because DEPEND_ABI.ros-comm.noetic?= ros-comm>=1.15 1.15.9 (2020-10-16) ------------------- * Fix deadlock when service connection is dropped (ros/ros_comm#2074) * Update maintainers (ros/ros_comm#2075) * Fix case where accessing cached parameters shuts down another node (ros/ros_comm#2068) * Fix spelling (ros/ros_comm#2066) * Fix Lost Wake Bug in ROSOutAppender (ros/ros_comm#2033) * Fix compatibility issue with boost 1.73 and above (ros/ros_comm#2023) * Gracefully stop recording upon SIGTERM and SIGINT (ros/ros_comm#2038) * Use heapq.merge instead of custom merge sort code (ros/ros_comm#2017) * Fix handling of single quotes in command arguments on Windows (ros/ros_comm#2051) * Clearer error message (ros/ros_comm#2035) * Ignore underscores when parsing literal numeric values for Python 3 compatibility (ros/ros_comm#2022) * Clear cached URI for a node that has gone offline (ros/ros_comm#2010) * Add skip_cache parameter to rosnode_ping() (ros/ros_comm#2009) * Install advertisetest (ros/ros_comm#2046) * Use range instead of xrange for Python 3 compatibility (ros/ros_comm#2013) * Fix to address CVE-2020-16124 (ros/ros_comm#2065) * Fix XmlRpcValue::_doubleFormat being unused (ros/ros_comm#2003) 1.15.8 (2020-07-23) ------------------- * change is_async_connected to use epoll when available (ros/ros_comm#1983) * allow mixing latched and unlatched publishers (ros/ros_comm#1991) * remove not existing NodeProxy from rospy __all_\_ (ros/ros_comm#2007) * fix typo in topics.py (ros/ros_comm#1977) * fix bad relative import (still Python 2 style) (ros/ros_comm#1973) * improve shutdown message with duplicate node name (ros/ros_comm#1992) * remove dependency on rostopic from rostest package (ros/ros_comm#2002) * fix missing reload() function in Python 3 (ros/ros_comm#1968) * add latch param to throttle (ros/ros_comm#1944) * add const versions of XmlRpcValue converting operators (ros/ros_comm#1978) 1.15.7 (2020-05-28) ------------------- * fix Windows build break (ros/ros_comm#1961) * fix NameError in launch error handling (ros/ros_comm#1965) 1.15.6 (2020-05-21) ------------------- * fix a bug that using a destroyed connection object (ros/ros_comm#1950) 1.15.5 (2020-05-15) ------------------- * check if async socket connect is success or failure before TransportTCP::read() and TransportTCP::write() (ros/ros_comm#1954) * fix bug that connection drop signal related funtion throw a bad_weak exception (ros/ros_comm#1940) * multiple latched publishers per process on the same topic (ros/ros_comm#1544) * fix negative numbers in ros statistics (ros/ros_comm#1531) * remove extra \n in ROS_DEBUG (ros/ros_comm#1925) * add option to repeat latched messages at the start of bag splits (ros/ros_comm#1850) * fix bag migration failures caused by typo in connection_header assignment (ros/ros_comm#1952) * fix brief description comments after members (ros/ros_comm#1920) * add --sigint-timeout and --sigterm-timeout parameters (ros/ros_comm#1937) * roslaunch-check: search dir recursively (ros/ros_comm#1914) * sort printed nodes by namespace alphabetically (ros/ros_comm#1934) * remove pycrypto import (not used) (ros/ros_comm#1922) * avoid infinite recursion in rosrun tab completion when rosbash is not installed (ros/ros_comm#1948) * fix bare pointer in topic_tools::ShapeShifter (ros/ros_comm#1722) * clear message queue on simtime jumping back (ros/ros_comm#1518) * use undefined dynamic_lookup on macOS (ros/ros_comm#1923) * check if enough FDs are free, instead counting the total free FDs (ros/ros_comm#1929) 1.15.4 (2020-03-19) ------------------- * restrict boost dependencies to components used (ros/ros_comm#1871) * add exception for ConnectionAbortedError (ros/ros_comm#1908) * fix mac trying to use epoll instead of kqueue (ros/ros_comm#1907) * fix AttributeError: __exit__ (ros/ros_comm#1915, regression from 1.14.4) 1.15.3 (2020-02-28) ------------------- * remove Boost version check since Noetic only targets platforms with 1.67+ (ros/ros_comm#1903) 1.15.2 (2020-02-25) ------------------- * export missing Boost dependency (ros/ros_comm#1898) * add timestamp formatting for rosconsole (ros/ros_comm#1892) 1.15.1 (2020-02-24) ------------------- * fix missing boost dependencies (ros/ros_comm#1895) * use setuptools instead of distutils (ros/ros_comm#1870) * increase time limit of advertisetest/publishtest.test to reduce flakyness (ros/ros_comm#1897) 1.15.0 (2020-02-21) ------------------- * fix dictionary changed size during iteration (ros/ros_comm#1894) * update test to pass with old and new yaml (ros/ros_comm#1893) Packaging changes: - removed patch-an, as there are no more boost version checks - updated patch-ao

dabonnie added 2 commits August 31, 2020 10:44

Add SIGTERM and SIGINT handlers to rosbag record

b597b38

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

Add unit test for rosbag record SIGINT handling

cac17de

Add unit test for rosbag record SIGTERM handling Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

Address review comments

c6e59c9

Fix sending SIGINT to main process Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dirk-thomas reviewed Sep 15, 2020

View reviewed changes

tools/rosbag/src/record.cpp Outdated Show resolved Hide resolved

tools/rosbag/src/record.cpp Outdated Show resolved Hide resolved

dabonnie added 2 commits September 15, 2020 14:38

Revert added whitespace

009066b

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

Revert SIGINT handler addition: use default

bce95bd

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie force-pushed the dabonnie/add-sigterm-handler branch from 49a5d1b to bce95bd Compare September 15, 2020 23:19

dabonnie requested a review from dirk-thomas September 15, 2020 23:19

dirk-thomas reviewed Sep 17, 2020

View reviewed changes

test/test_rosbag/test/record_cleanup.py Outdated Show resolved Hide resolved

test/test_rosbag/test/record_cleanup.py Outdated Show resolved Hide resolved

tools/rosbag/src/record.cpp Outdated Show resolved Hide resolved

tools/rosbag/src/rosbag/rosbag_main.py Show resolved Hide resolved

Remove unnecessary wait

b3ecfbe

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie changed the title ~~Gracefully stop recording upon SIGTERM~~ Gracefully stop recording upon SIGTERM and SIGINT Sep 18, 2020

dabonnie added 3 commits September 21, 2020 15:39

Use BSD License

6acecc8

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

Add test improvements

4c3a721

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

Move test helper function

102a8fd

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie force-pushed the dabonnie/add-sigterm-handler branch from 6ffeb2e to 102a8fd Compare September 22, 2020 03:11

Remove redundant test rosbag launch

9636fa0

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie requested a review from dirk-thomas September 22, 2020 18:09

dirk-thomas reviewed Sep 22, 2020

View reviewed changes

test/test_rosbag/test/record_cleanup.py Outdated Show resolved Hide resolved

tools/rosbag/src/record.cpp Outdated Show resolved Hide resolved

dabonnie added 2 commits September 22, 2020 16:13

Add Amazon to new python test copyright

328a550

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

Remove unrelated whitespace

08315e8

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie requested a review from dirk-thomas September 22, 2020 23:15

dirk-thomas reviewed Sep 22, 2020

View reviewed changes

test/test_rosbag/CMakeLists.txt Show resolved Hide resolved

Split record cleanup unit tests

3436965

Add record cleanup unit test helper Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

dabonnie requested a review from dirk-thomas September 23, 2020 19:14

dirk-thomas reviewed Sep 23, 2020

View reviewed changes

tools/rosbag/src/record.cpp Show resolved Hide resolved

dabonnie and others added 2 commits September 23, 2020 13:30

Revert whitespace change

c6dadb2

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>

revert white space change

2d5c413

dirk-thomas added the bug label Sep 23, 2020

dirk-thomas merged commit 7b5a0be into ros:noetic-devel Sep 23, 2020

dabonnie mentioned this pull request Sep 24, 2020

[Kinetic] Backport #2038 #2058

Closed

dirk-thomas added the consider-backport label Sep 24, 2020

dabonnie mentioned this pull request Sep 24, 2020

Backport #2038 to Melodic #2060

Closed

dirk-thomas mentioned this pull request Oct 2, 2020

Sending SIGKILL to the roslaunch process group doesn't kill launched nodes #2069

Closed

jacobperron mentioned this pull request Oct 16, 2020

Changes between 1.15.8 and 1.15.9 for backporting to Melodic #2081

Merged

jacobperron mentioned this pull request Oct 22, 2020

Changes between 1.14.9 and 1.14.10 for backporting to Kinetic #2084

Merged

lucasw mentioned this pull request Jan 1, 2024

ros1 ros_comm that publishes/subscribes straight to zenoh lucasw/ros_one2z#7

Open

Conversation

dabonnie commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dabonnie commented Sep 4, 2020

Uh oh!

dirk-thomas commented Sep 4, 2020

Uh oh!

dabonnie commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dabonnie commented Sep 4, 2020

Uh oh!

thomas-moulard commented Sep 11, 2020

Uh oh!

dabonnie commented Sep 12, 2020

Uh oh!

dabonnie commented Sep 12, 2020

Uh oh!

Uh oh!

Uh oh!

dabonnie commented Sep 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dabonnie commented Sep 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dirk-thomas commented Sep 23, 2020

Uh oh!

dirk-thomas commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dabonnie commented Sep 4, 2020 •

edited

Loading

dabonnie commented Sep 4, 2020 •

edited

Loading