From 78d76d45ab09005dcc8fc70ec132d2b68ee33b80 Mon Sep 17 00:00:00 2001 From: Hook25 Date: Thu, 12 Dec 2024 15:22:26 +0100 Subject: [PATCH 1/3] Multishell support and add assertion on min. timeout --- .../checkbox_support/helpers/timeout.py | 23 ++++++++++++++++++- .../checkbox_support/tests/test_timeout.py | 12 +++++----- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/checkbox-support/checkbox_support/helpers/timeout.py b/checkbox-support/checkbox_support/helpers/timeout.py index 9dce880458..567eec36f4 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() @@ -110,7 +131,7 @@ def _f(*args, **kwargs): if process.is_alive(): # this kills the whole process tree, not just the child - subprocess.run("kill -9 -{}".format(process.pid), shell=True) + 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..479ed3f839 100644 --- a/checkbox-support/checkbox_support/tests/test_timeout.py +++ b/checkbox-support/checkbox_support/tests/test_timeout.py @@ -61,7 +61,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 +72,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 +104,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 +203,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,7 +215,7 @@ 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: ...)) From 7390a9f0e4007e75a867da4cb8039eeeae516900 Mon Sep 17 00:00:00 2001 From: Hook25 Date: Thu, 12 Dec 2024 16:12:46 +0100 Subject: [PATCH 2/3] Test the new kill_tree function --- .../checkbox_support/tests/test_timeout.py | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/checkbox-support/checkbox_support/tests/test_timeout.py b/checkbox-support/checkbox_support/tests/test_timeout.py index 479ed3f839..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, ) @@ -220,3 +222,22 @@ def test_run_with_timeout_system_exit_no_get( 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) From c3972d9f3ebfec2e0c996712e6800cb4c55b2e0a Mon Sep 17 00:00:00 2001 From: Massimiliano Date: Thu, 12 Dec 2024 16:59:42 +0100 Subject: [PATCH 3/3] Update checkbox-support/checkbox_support/helpers/timeout.py Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com> --- checkbox-support/checkbox_support/helpers/timeout.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checkbox-support/checkbox_support/helpers/timeout.py b/checkbox-support/checkbox_support/helpers/timeout.py index 567eec36f4..beb2a73fb6 100644 --- a/checkbox-support/checkbox_support/helpers/timeout.py +++ b/checkbox-support/checkbox_support/helpers/timeout.py @@ -130,7 +130,7 @@ def _f(*args, **kwargs): process.join(timeout_s) if process.is_alive(): - # this kills the whole process tree, not just the child + # 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))