Multishell support and add assertion on min. timeout (bugfix) (#1654)

* Multishell support and add assertion on min. timeout

* Test the new kill_tree function

* Update checkbox-support/checkbox_support/helpers/timeout.py

Co-authored-by: Fernando Bravo <[email protected]>

---------

Co-authored-by: Fernando Bravo <[email protected]>

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):

checkbox-support/checkbox_support/tests/test_timeout.py

@@ -17,6 +17,7 @@
 # along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
 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)