diff --git a/checkbox-support/checkbox_support/helpers/timeout.py b/checkbox-support/checkbox_support/helpers/timeout.py index 9dce880458..beb2a73fb6 100644 --- a/checkbox-support/checkbox_support/helpers/timeout.py +++ b/checkbox-support/checkbox_support/helpers/timeout.py @@ -23,6 +23,7 @@ functions """ import os +import shutil import pickle import traceback import subprocess @@ -48,6 +49,25 @@ def is_picklable(value): return False +def kill_tree(pid): + """ + Kill a process tree + + This tries to force the shell to what we want instead of letting the system + decide. This is because `sh` kill doesn't support tree kill (-PID), so we + would rather avoid it + """ + if shutil.which("bash"): + return subprocess.run(["bash", "-c", "kill -9 -{}".format(pid)]) + elif shutil.which("zsh"): + return subprocess.run(["zsh", "-c", "kill -9 -{}".format(pid)]) + with suppress(subprocess.CalledProcessError): + # if SHELL is sh or doesn't support -XX pid, this will fail + return subprocess.check_call("kill -9 -{}".format(pid), shell=True) + # lets at least kill the direct pid + return subprocess.run("kill -9 {}".format(pid), shell=True) + + def run_with_timeout(f, timeout_s, *args, **kwargs): """ Runs a function with the given args and kwargs. If the function doesn't @@ -56,6 +76,7 @@ def run_with_timeout(f, timeout_s, *args, **kwargs): Note: the function, *args and **kwargs must be picklable to use this. """ + assert timeout_s > 0, "Timeout must be more than 0" result_queue = Queue() exception_queue = Queue() @@ -109,8 +130,8 @@ def _f(*args, **kwargs): process.join(timeout_s) if process.is_alive(): - # this kills the whole process tree, not just the child - subprocess.run("kill -9 -{}".format(process.pid), shell=True) + # this tries to kill the whole process tree, not just the child. + kill_tree(process.pid) raise TimeoutError("Task unable to finish in {}s".format(timeout_s)) with suppress(Empty): diff --git a/checkbox-support/checkbox_support/tests/test_timeout.py b/checkbox-support/checkbox_support/tests/test_timeout.py index 22ce56018e..e3e7b1a0a6 100644 --- a/checkbox-support/checkbox_support/tests/test_timeout.py +++ b/checkbox-support/checkbox_support/tests/test_timeout.py @@ -17,6 +17,7 @@ # along with Checkbox. If not, see . import os import time +import subprocess import multiprocessing from queue import Empty @@ -28,6 +29,7 @@ is_picklable, run_with_timeout, fake_run_with_timeout, + kill_tree, ) @@ -61,7 +63,7 @@ class TestTimeoutExec(TestCase): def test_class_field_timeouts(self): some = ClassSupport(1) with self.assertRaises(TimeoutError): - run_with_timeout(some.heavy_function, 0) + run_with_timeout(some.heavy_function, 0.1) def test_class_field_ok_return(self): some = ClassSupport(0) @@ -72,11 +74,11 @@ def test_class_field_ok_return(self): def test_function_timeouts(self): with self.assertRaises(TimeoutError): - run_with_timeout(heavy_function, 0, 10) + run_with_timeout(heavy_function, 0.1, 10) def test_function_ok_return(self): self.assertEqual( - run_with_timeout(heavy_function, 10, 0), + run_with_timeout(heavy_function, 10, 0.1), "ClassSupport return value", ) @@ -104,7 +106,7 @@ def f(first, second, third): self.assertEqual(f(1, 2, 3), (1, 2, 3)) def test_decorator_test_fail(self): - @timeout(0) + @timeout(0.1) def f(first, second, third): time.sleep(100) return (first, second, third) @@ -203,7 +205,7 @@ def test_run_with_timeout_double_get(self, process_mock, queue_mock): ] with self.assertRaises(ValueError): - run_with_timeout(lambda: ..., 0) + run_with_timeout(lambda: ..., 0.1) @patch("checkbox_support.helpers.timeout.Queue") @patch("checkbox_support.helpers.timeout.Process") @@ -215,8 +217,27 @@ def test_run_with_timeout_system_exit_no_get( queue_mock().get.side_effect = Empty() with self.assertRaises(SystemExit): - run_with_timeout(lambda: ..., 0) + run_with_timeout(lambda: ..., 0.1) def test_is_picklable(self): self.assertFalse(is_picklable(lambda: ...)) self.assertTrue(is_picklable([1, 2, 3])) + + @patch("shutil.which") + @patch("subprocess.check_call") + @patch("subprocess.run") + def test_kill_tree_fallback(self, run_mock, check_call_mock, which_mock): + which_mock.return_value = None + check_call_mock.side_effect = subprocess.CalledProcessError( + 1, "Some cmd" + ) + + kill_tree(123) + # kill_tree tried to search for known consoles + self.assertTrue(which_mock.called) + # kill_tree also tried to run the default shell to see if kill supports + # the -PID semantic + self.assertTrue(check_call_mock.called) + # finally the function tried to call kill without the - as a last ditch + # tentative + self.assertTrue(run_mock.called)