Timeouts will now always raise TimeoutExpired

regardless of the raise_timeout_exception argument.
This also deprecates the raise_timeout_exception argument.
If that is still set to False a UserWarning is emitted.
If it is set to True a DeprecationWarning is emitted.

Author: Anthchirp

HISTORY.rst

@@ -8,6 +8,10 @@ History
 * The run() function now returns a subprocess.CompletedProcess object,
   which no longer allows array access operations
   (those were deprecated in `#60 <https://github.com/DiamondLightSource/python-procrunner/pull/60>`_)
+* The run() argument 'raise_timeout_exception' is now set by default,
+  a 'False' value will lead to a UserWarning and a behavioural change.
+  The argument is now deprecated and will be removed in a future version.
+  (previously introduced in `#61 <https://github.com/DiamondLightSource/python-procrunner/pull/61>`_)
 * Calling the run() function with multiple unnamed arguments is no longer supported
   (previously deprecated in `#62 <https://github.com/DiamondLightSource/python-procrunner/pull/62>`_)
 * The run() function no longer accepts a 'debug' argument

src/procrunner/__init__.py

@@ -13,7 +13,7 @@
 import warnings
 from multiprocessing import Pipe
 from threading import Thread
-from typing import Callable, Optional
+from typing import Any, Callable, Optional
 
 #
 #  run() - A function to synchronously run an external process, supporting
@@ -313,7 +313,7 @@ def run(
     environment_override: Optional[dict[str, str]] = None,
     win32resolve: bool = True,
     working_directory: Optional[str] = None,
-    raise_timeout_exception: bool = False,
+    raise_timeout_exception: Any = ...,
 ) -> subprocess.CompletedProcess:
     """
     Run an external process.
@@ -339,11 +339,7 @@ def run(
                                  extension.
     :param string working_directory: If specified, run the executable from
                                      within this working directory.
-    :param boolean raise_timeout_exception: Forward compatibility flag. If set
-                             then a subprocess.TimeoutExpired exception is raised
-                             instead of returning an object that can be checked
-                             for a timeout condition. Defaults to False, will be
-                             changed to True in a future release.
+    :param boolean raise_timeout_exception: Deprecated compatibility flag.
     :return: The exit code, stdout, stderr (separately, as byte strings)
              as a subprocess.CompletedProcess object.
     """
@@ -359,12 +355,18 @@ def run(
     start_time = timeit.default_timer()
     if timeout is not None:
         max_time = start_time + timeout
-        if not raise_timeout_exception:
-            warnings.warn(
-                "Using procrunner with timeout and without raise_timeout_exception set is deprecated",
-                DeprecationWarning,
-                stacklevel=3,
-            )
+    if not raise_timeout_exception:
+        warnings.warn(
+            "Using procrunner with raise_timeout_exception=False is no longer supported",
+            UserWarning,
+            stacklevel=3,
+        )
+    elif raise_timeout_exception is True:
+        warnings.warn(
+            "The raise_timeout_exception argument is deprecated and will be removed in a future release",
+            DeprecationWarning,
+            stacklevel=3,
+        )
 
     if environment is not None:
         env = {key: _path_resolve(environment[key]) for key in environment}
@@ -503,7 +505,7 @@ def run(
     output_stdout = stdout.get_output()
     output_stderr = stderr.get_output()
 
-    if timeout is not None and timeout_encountered and raise_timeout_exception:
+    if timeout is not None and timeout_encountered:
         raise subprocess.TimeoutExpired(
             cmd=command, timeout=timeout, output=output_stdout, stderr=output_stderr
         )

tests/test_procrunner.py

@@ -47,7 +47,7 @@ def test_run_command_aborts_after_timeout(
     task = ["___"]
 
     with pytest.raises(RuntimeError):
-        procrunner.run(task, timeout=-1, raise_timeout_exception=True)
+        procrunner.run(task, timeout=-1)
 
     assert mock_subprocess.Popen.called
     assert mock_process.terminate.called
@@ -81,7 +81,6 @@ def streamreader_processing(*args, **kwargs):
         callback_stdout=mock.sentinel.callback_stdout,
         callback_stderr=mock.sentinel.callback_stderr,
         working_directory=pathlib.Path("somecwd"),
-        raise_timeout_exception=True,
     )
 
     assert mock_subprocess.Popen.called
@@ -122,7 +121,7 @@ def streamreader_processing(*args, **kwargs):
 def test_default_process_environment_is_parent_environment(mock_subprocess):
     mock_subprocess.Popen.side_effect = NotImplementedError()  # cut calls short
     with pytest.raises(NotImplementedError):
-        procrunner.run([mock.Mock()], timeout=-1, raise_timeout_exception=True)
+        procrunner.run([mock.Mock()], timeout=-1)
     assert mock_subprocess.Popen.call_args[1]["env"] == os.environ
 
 
@@ -136,7 +135,6 @@ def test_pass_custom_environment_to_process(mock_subprocess):
             [mock.Mock()],
             timeout=-1,
             environment=copy.copy(mock_env),
-            raise_timeout_exception=True,
         )
     assert mock_subprocess.Popen.call_args[1]["env"] == mock_env
 
@@ -153,7 +151,6 @@ def test_pass_custom_environment_to_process_and_add_another_value(mock_subproces
             timeout=-1,
             environment=copy.copy(mock_env1),
             environment_override=copy.copy(mock_env2),
-            raise_timeout_exception=True,
         )
     mock_env_sum = copy.copy(mock_env1)
     mock_env_sum.update({key: str(mock_env2[key]) for key in mock_env2})
@@ -169,7 +166,6 @@ def test_use_default_process_environment_and_add_another_value(mock_subprocess):
             [mock.Mock()],
             timeout=-1,
             environment_override=copy.copy(mock_env2),
-            raise_timeout_exception=True,
         )
     random_environment_variable = list(os.environ)[0]
     if random_environment_variable == list(mock_env2)[0]:
@@ -199,7 +195,6 @@ def test_use_default_process_environment_and_override_a_value(mock_subprocess):
             environment_override={
                 random_environment_variable: "X" + random_environment_value
             },
-            raise_timeout_exception=True,
         )
     assert (
         mock_subprocess.Popen.call_args[1]["env"][random_environment_variable]

tests/test_procrunner_system.py

@@ -76,26 +76,54 @@ def test_path_object_resolution(tmp_path):
     ), "overridden environment variable leaked into parent process"
 
 
+def test_timeout_behaviour_old_legacy(tmp_path):
+    command = (sys.executable, "-c", "import time; time.sleep(5)")
+    start = timeit.default_timer()
+    try:
+        with pytest.raises(subprocess.TimeoutExpired) as te:
+            with pytest.warns(UserWarning, match="timeout"):
+                procrunner.run(
+                    command,
+                    timeout=0.1,
+                    working_directory=tmp_path,
+                    raise_timeout_exception=False,
+                )
+    except RuntimeError:
+        # This test sometimes fails with a RuntimeError.
+        runtime = timeit.default_timer() - start
+        assert runtime < 3
+        return
+    runtime = timeit.default_timer() - start
+    assert runtime < 3
+    assert te.value.stdout == b""
+    assert te.value.stderr == b""
+    assert te.value.timeout == 0.1
+    assert te.value.cmd == command
+
+
 def test_timeout_behaviour_legacy(tmp_path):
+    command = (sys.executable, "-c", "import time; time.sleep(5)")
     start = timeit.default_timer()
     try:
-        with pytest.warns(DeprecationWarning, match="timeout"):
-            result = procrunner.run(
-                [sys.executable, "-c", "import time; time.sleep(5)"],
-                timeout=0.1,
-                working_directory=tmp_path,
-                raise_timeout_exception=False,
-            )
+        with pytest.raises(subprocess.TimeoutExpired) as te:
+            with pytest.warns(DeprecationWarning, match="timeout"):
+                procrunner.run(
+                    command,
+                    timeout=0.1,
+                    working_directory=tmp_path,
+                    raise_timeout_exception=True,
+                )
     except RuntimeError:
         # This test sometimes fails with a RuntimeError.
         runtime = timeit.default_timer() - start
         assert runtime < 3
         return
     runtime = timeit.default_timer() - start
     assert runtime < 3
-    assert not result.stdout
-    assert not result.stderr
-    assert result.returncode
+    assert te.value.stdout == b""
+    assert te.value.stderr == b""
+    assert te.value.timeout == 0.1
+    assert te.value.cmd == command
 
 
 def test_timeout_behaviour(tmp_path):
@@ -107,7 +135,6 @@ def test_timeout_behaviour(tmp_path):
                 command,
                 timeout=0.1,
                 working_directory=tmp_path,
-                raise_timeout_exception=True,
             )
     except RuntimeError:
         # This test sometimes fails with a RuntimeError.