GoogleCloudPlatform / GoogleCloudPlatform/PerfKitBenchmarker

IssueCommand timeouts don't kill subprocesses.

Open
#298 7 comments 0 reactions 1 assignee Claimed by @voellm View on GitHub
bug P1
Dominant language
Python
Stars
2k
Forks
562
Avg merge
4h 55m
Merged PRs (30d)
69

Description

If IssueCommand times out, the reaper only kills the specific process spawned. Any subprocesses it had created will keep running. This hasn't been an issue so far, but if this ends up causing problems we should fix it.

For background, see: http://stackoverflow.com/questions/4789837/how-to-terminate-a-python-subprocess-launched-with-shell-true

Method A: set the child as session or process group leader via `creationflags=subprocess.CREATE_NEW_PROCESS_GROUP (Windows)`, or `preexec_fn=os.setsid` or `preexec_fn=os.setprgp` (Unix). Unfortunately this seems to have the side effect that Ctrl-C
will no longer terminate the subprocesses, we'd need to propagate keyboard signals manually with this approach.

Method B: explicitly find and kill child processes. This is easy when using the [psutil](http://pythonhosted.org/psutil/) package, but that package requires compilation, and we'd prefer to avoid this dependency.

Here's an experimental patch for method B, including a test that shows the problem if _vm_util.py_ is not patched:

``` Diff
diff --git a/perfkitbenchmarker/vm_util.py b/perfkitbenchmarker/vm_util.py
index a95ca95..f3889b7 100644
--- a/perfkitbenchmarker/vm_util.py
+++ b/perfkitbenchmarker/vm_util.py
@@ -18,6 +18,7 @@ import contextlib
import logging
import os
import posixpath
+import psutil
import random
import re
import socket
@@ -386,6 +387,8 @@ def IssueCommand(cmd, force_info_log=False, suppress_warning=False,
def _KillProcess():
logging.error('IssueCommand timed out after %d seconds. '
'Killing command "%s".', timeout, full_cmd)
+ for child in psutil.Process(process.pid).get_children(recursive=True):
+ child.kill()
process.kill()

timer = threading.Timer(timeout, _KillProcess)
diff --git a/requirements.txt b/requirements.txt
index 2219e69..79d68e5 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -15,3 +15,4 @@ python-gflags==2.0
jinja2>=2.7
setuptools
colorlog[windows]==2.6.0
+psutil
diff --git a/tests/vm_util_test.py b/tests/vm_util_test.py
index 7a655d5..ed8606d 100644
--- a/tests/vm_util_test.py
+++ b/tests/vm_util_test.py
@@ -111,6 +111,15 @@ class WaitUntilSleepTimer(threading.Thread):
self.finished.set()

+class Stopwatch(object):
+ def __enter__(self):
+ self.start = time.time()
+ return self
+
+ def __exit__(self, *args):
+ self.seconds = time.time() - self.start
+
+
class IssueCommandTestCase(unittest.TestCase):

def testTimeoutNotReached(self):
@@ -134,6 +143,26 @@ class IssueCommandTestCase(unittest.TestCase):
vm_util.IssueCommand(['sleep', '2s'], timeout=None)
self.assertFalse(HaveSleepSubprocess())

+ @mock.patch('threading.Timer', new=WaitUntilSleepTimer)
+ def testSimpleTimeout(self):
+ with Stopwatch() as elapsed:
+ _, _, retcode = vm_util.IssueCommand(['sleep', '5s'], timeout=1)
+ self.assertEqual(retcode, -9)
+ self.assertFalse(HaveSleepSubprocess())
+ self.assertLess(elapsed.seconds, 2)
+
+ @mock.patch('threading.Timer', new=WaitUntilSleepTimer)
+ def testSubProcessTimeout(self):
+ if vm_util.RunningOnWindows():
+ cmd = ['sleep 4s & sleep 5s']
+ else:
+ cmd = ['sh', '-c', 'sleep 4s; sleep 5s']
+ with Stopwatch() as elapsed:
+ _, _, retcode = vm_util.IssueCommand(cmd, timeout=1)
+ self.assertEqual(retcode, -9)
+ self.assertFalse(HaveSleepSubprocess())
+ self.assertLess(elapsed.seconds, 2)
+

if __name__ == '__main__':
unittest.main()
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.