terminate underlying 'rosbag {play,record}' on SIGTERM

[jwm]

#189 (which is merged, but its underlying bug #114 is not closed) migrated from os.execv() to subprocess.call() to run the play and record nodes.

Unfortunately, when you send rosbag a SIGTERM, such as with control-C, the parent rosbag process exits, but leaves the child node running, playing or recording the bag.

This traps SIGTERM and propagates the signal to the underlying node process. Then, a control-C ends the process as the user would expect.

I don't have a Windows machine to test on, and it looks like I'd have to compile ROS from the source. However, the Python documentation says this approach is compatible with Windows:

https://docs.python.org/2/library/signal.html

On Windows, signal() can only be called with SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, or SIGTERM. A ValueError will be raised in any other case.

https://docs.python.org/2/library/subprocess.html

Popen.terminate()
Stop the child. On Posix OSs the method sends SIGTERM to the child. On Windows the Win32 API function TerminateProcess() is called to stop the child.

[jwm]

Example process tree:

vagrant@local-driver-ui:~$ ps auwwwxf | grep pts/0
vagrant   9033  0.0  0.2 105644  4940 ?        S    17:42   0:00  |   \_ sshd: vagrant@pts/0
vagrant   9034  0.0  0.3  24504  7384 pts/0    Ss   17:42   0:00  |       \_ -bash
vagrant   9438  2.0  1.0  73328 20532 pts/0    S+   17:50   0:00  |           \_ /usr/bin/python /opt/ros/indigo/bin/rosbag play -l driver_ui_test.bag
vagrant   9441  1.0  0.5 413664 11368 pts/0    Sl+  17:50   0:00  |               \_ /opt/ros/indigo/lib/rosbag/play --loop --queue 100 --rate 1.0 --delay 0.2 --start 0.0 driver_ui_test.bag

With the patch, after hitting control-C:

vagrant@local-driver-ui:~$ ps auwwwxf | grep pts/0
vagrant   9033  0.0  0.2 105644  4940 ?        S    17:42   0:00  |   \_ sshd: vagrant@pts/0
vagrant   9034  0.0  0.3  24504  7384 pts/0    Ss+  17:42   0:00  |       \_ -bash
vagrant   9473  0.0  0.1  11752  2244 pts/1    S+   17:50   0:00              \_ grep --color=auto pts/0

[jspricke]

+1

[dirk-thomas]

@jwm Can you please provide a short sequence of command to reproduce the problem before / the improved behavior after the patch.

[jwm]

@dirk-thomas Sure, it's pretty straightforward.

Without the patch, run rosbag play some-bag.bag and hit Ctrl-C. The rosbag Python process exits, leaving the underlying play node running and emitting output, even though I'm back at a command prompt.

With the patch, Ctrl-C terminates both the Python process and the underlying node, leaving me at a command prompt with no further output from the play node.

[dirk-thomas]

I don't see any difference in behavior when hitting Ctrl-C. Looking at the patch that is not surprising since it only handles SIGTERM but Ctrl-C emits a SIGINT.

[jwm]

Ah, it's been a few weeks, I was misremebering.

We were running rosbag play from a pytest fixture that begins playback, yields to the test function, then cleans up by killing the child. This results in the node process remaining after the Python interpreter exits.

[dirk-thomas]

Can you please share a reproducible example (SSCCE).

[jwm]

• Run rosbag play some-bag.bag
• Send the Python process a SIGTERM. See the Python interpreter process disappear from the process table.
• Watch the node process continue to play the bag, emitting output.

[dirk-thomas]

I can reproduce that case and also that the patch fixes the problem.

But currently the patch overwrites the signal handler. Instead it should make sure to also call any previously registered signal handle from within the lambda in order to not ignore previously registered signal handler.

[jwm]

Force-pushed an updated patch.

[dirk-thomas]

Thank you for the patch and iterating on it.

[jwm]

Thanks for merging, Dirk!