Repository: python/cpython
Pull request: gh-133089: Use original timeout value for `TimeoutExpired` when the func `subprocess.run` is called with a timeout
Fix #133089

<!-- gh-issue-number: gh-133089 -->
* Issue: gh-133089
<!-- /gh-issue-number -->


Problem statement:
Negative run-time reported by `subprocess.run`'s `TimeoutExpired` exception when setting `timeout=0`
# Bug report

### Bug description:

```python
import subprocess
subprocess.run(['echo', 'hi'], timeout = 0)

# subprocess.TimeoutExpired: Command '['echo', 'hi']' timed out after -0.00010189996100962162 seconds
# subprocess.TimeoutExpired: Command '['echo', 'hi']' timed out after -4.819990135729313e-05 seconds
```

This is quite unexpected that the measured run-time can be negative

In general, it's unclear in the docs what behavior should have `timeout=0`. I would propose that it should disable timeout control, and should be equivalent to `timeout=None`.

But in any case, negative run-times are quite strange :)

### CPython versions tested on:

3.12

### Operating systems tested on:

WSLv1+Ubuntu24.04

<!-- gh-linked-prs -->
### Linked PRs
* gh-133103
* gh-133418
<!-- /gh-linked-prs -->

Hints from issue discussion:
Btw, even outside of unspecified behavior for `timeout=0`, setting `timeout` to a very small value also prints negative time measurement:

```python
import subprocess
subprocess.run(['echo', 'hi'], timeout = 1e-9)

# subprocess.TimeoutExpired: Command '['echo', 'hi']' timed out after -2.162200007660431e-05 seconds
```

How can time measurement be negative? Is it indicative of some other bug related to execution time measurement?
if someone specifies an unrealistically small timeout such as 0 or 0.00001 they should expect to get a TimeoutError.  timeout=0 should not disable the timeout.  Requiring None for that is not a bug.

It'd be reasonable to raise ValueError if the timeout supplied were < 0 but I would not consider that a bugfix.

The tiny negative number in the error message in this case is due to timeout math being floating point, inexact, and fuzzy on posix it is calculated as an time delta of remaining time between steps and thus floating point precision can yield an odd looking result.  the value in the error message is not the original timeout passed into the API call as that is not preserved and passed through into the self.wait(timeout=self._remaining_time(endtime)) call within `Popen._communicate` that raises in this specific case.
> timeout=0 should not disable the timeout. Requiring None for that is not a bug.

I propose that semantics of this edge case is explained clearly in the docs. Because in different systems the semantics of `timeout=0` is different, e.g. in bash `export TMOUT=0` diables the auto-logout timeout


For the negative number, of course I realize that it's most likely an artifact of some calculation, but it is again very surprising to see it as end user and there might be a suspicion of some bug somewhere. I propose that it's mentioned in the docs somewhere - or clipped up to zero to fix up the semantics

---

(Allowing timeout=0 to have disabling semantics can simplify types to just being "float" instead of "float | None" in client user code, where the timeout value gets passed around many layers of indirection (including e.g. configs)
I think the awkward error message is fixable, i left a note on the PR to see if they want to try doing that.

Commit message: Fix windows test

Signed-off-by: Manjusaka <me@manjusaka.me>

Patch to review:
diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst
index 05d09e304b32bf..028a7861f36798 100644
--- a/Doc/library/subprocess.rst
+++ b/Doc/library/subprocess.rst
@@ -1525,6 +1525,24 @@ handling consistency are valid for these functions.
 Notes
 -----
 
+.. _subprocess-timeout-behavior:
+
+Timeout Behavior
+^^^^^^^^^^^^^^^^
+
+When using the ``timeout`` parameter in functions like :func:`run`,
+:meth:`Popen.wait`, or :meth:`Popen.communicate`,
+users should be aware of the following behaviors:
+
+1. **Process Creation Delay**: The initial process creation itself cannot be interrupted
+   on many platform APIs. This means that even when specifying a timeout, you are not
+   guaranteed to see a timeout exception until at least after however long process
+   creation takes.
+
+2. **Extremely Small Timeout Values**: Setting very small timeout values (such as a few
+   milliseconds) may result in almost immediate :exc:`TimeoutExpired` exceptions because
+   process creation and system scheduling inherently require time.
+
 .. _converting-argument-sequence:
 
 Converting an argument sequence to a string on Windows
diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index da5f5729e097e9..59bb3b0934de74 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -1235,8 +1235,10 @@ def communicate(self, input=None, timeout=None):
 
             finally:
                 self._communication_started = True
-
-            sts = self.wait(timeout=self._remaining_time(endtime))
+            try:
+                sts = self.wait(timeout=self._remaining_time(endtime))
+            except TimeoutExpired:
+                raise TimeoutExpired(self.args, timeout)
 
         return (stdout, stderr)
 
@@ -2145,8 +2147,10 @@ def _communicate(self, input, endtime, orig_timeout):
                                 selector.unregister(key.fileobj)
                                 key.fileobj.close()
                             self._fileobj2output[key.fileobj].append(data)
-
-            self.wait(timeout=self._remaining_time(endtime))
+            try:
+                self.wait(timeout=self._remaining_time(endtime))
+            except TimeoutExpired:
+                raise TimeoutExpired(self.args, orig_timeout)
 
             # All data exchanged.  Translate lists into strings.
             if stdout is not None:
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 3cb755cd56cac8..d2db8fbcb64d52 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -162,6 +162,20 @@ def test_call_timeout(self):
                           [sys.executable, "-c", "while True: pass"],
                           timeout=0.1)
 
+    def test_timeout_exception(self):
+        try:
+            subprocess.run(['echo', 'hi'], timeout = -1)
+        except subprocess.TimeoutExpired as e:
+            self.assertIn("-1 seconds", str(e))
+        else:
+            self.fail("Expected TimeoutExpired exception not raised")
+        try:
+            subprocess.run(['echo', 'hi'], timeout = 0)
+        except subprocess.TimeoutExpired as e:
+            self.assertIn("0 seconds", str(e))
+        else:
+            self.fail("Expected TimeoutExpired exception not raised")
+
     def test_check_call_zero(self):
         # check_call() function with zero return code
         rc = subprocess.check_call(ZERO_RETURN_CMD)
diff --git a/Misc/NEWS.d/next/Library/2025-04-29-02-23-04.gh-issue-133089.8Jy1ZS.rst b/Misc/NEWS.d/next/Library/2025-04-29-02-23-04.gh-issue-133089.8Jy1ZS.rst
new file mode 100644
index 00000000000000..36c4efcf9dc2a6
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2025-04-29-02-23-04.gh-issue-133089.8Jy1ZS.rst
@@ -0,0 +1,2 @@
+Use origional timeout value for :exc:`subprocess.TimeoutExpired`
+when the func :meth:`subprocess.run` is called with a timeout


Code review comments:
diff_hunk: @@ -1235,8 +1235,10 @@ def communicate(self, input=None, timeout=None):
 
             finally:
                 self._communication_started = True
-
-            sts = self.wait(timeout=self._remaining_time(endtime))
+            try:
+                sts = self.wait(timeout=self._remaining_time(endtime))
+            except TimeoutExpired:
+                raise TimeoutExpired(self.args, timeout)
path: Lib/subprocess.py
text: @user1:
I modified these to not create a new one, just reassign a new timeout value to the existing exception instance and reraise it.