Skip to content

Commit 44d3d9e

Browse files
committed
gh-119710: Let asyncio Process.wait() finish on only process exit.
Letting Process.wait() only wait on actual process return is closer to how it's documented and consistent with Popen.wait(). This also reduces complexity for waking waiters which was inconsistend depending on ordering of wait/exit.
1 parent 2ac1611 commit 44d3d9e

3 files changed

Lines changed: 82 additions & 19 deletions

File tree

Lib/asyncio/base_subprocess.py

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ def __init__(self, loop, protocol, args, shell,
2626
self._pending_calls = collections.deque()
2727
self._pipes = {}
2828
self._finished = False
29-
self._pipes_connected = False
3029

3130
if stdin == subprocess.PIPE:
3231
self._pipes[0] = None
@@ -214,7 +213,6 @@ async def _connect_pipes(self, waiter):
214213
else:
215214
if waiter is not None and not waiter.cancelled():
216215
waiter.set_result(None)
217-
self._pipes_connected = True
218216

219217
def _call(self, cb, *data):
220218
if self._pending_calls is not None:
@@ -235,6 +233,16 @@ def _process_exited(self, returncode):
235233
if self._loop.get_debug():
236234
logger.info('%r exited with return code %r', self, returncode)
237235
self._returncode = returncode
236+
237+
# gh-119710: Wake up futures waiting for wait() as soon as the process
238+
# exits. The pipe transports now check for the loop being closed before
239+
# scheduling a callback preventing gh-114177. This is consistent with
240+
# the behavior prior to 3.11 and the documented semantics in _wait().
241+
for waiter in self._exit_waiters:
242+
if not waiter.done():
243+
waiter.set_result(returncode)
244+
self._exit_waiters = None
245+
238246
if self._proc.returncode is None:
239247
# asyncio uses a child watcher: copy the status into the Popen
240248
# object. On Python 3.6, it is required to avoid a ResourceWarning.
@@ -258,15 +266,7 @@ def _try_finish(self):
258266
assert not self._finished
259267
if self._returncode is None:
260268
return
261-
if not self._pipes_connected:
262-
# self._pipes_connected can be False if not all pipes were connected
263-
# because either the process failed to start or the self._connect_pipes task
264-
# got cancelled. In this broken state we consider all pipes disconnected and
265-
# to avoid hanging forever in self._wait as otherwise _exit_waiters
266-
# would never be woken up, we wake them up here.
267-
for waiter in self._exit_waiters:
268-
if not waiter.done():
269-
waiter.set_result(self._returncode)
269+
270270
if all(p is not None and p.disconnected
271271
for p in self._pipes.values()):
272272
self._finished = True
@@ -276,11 +276,6 @@ def _call_connection_lost(self, exc):
276276
try:
277277
self._protocol.connection_lost(exc)
278278
finally:
279-
# wake up futures waiting for wait()
280-
for waiter in self._exit_waiters:
281-
if not waiter.done():
282-
waiter.set_result(self._returncode)
283-
self._exit_waiters = None
284279
self._loop = None
285280
self._proc = None
286281
self._protocol = None

Lib/test/test_asyncio/test_subprocess.py

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,6 @@ def test_proc_exited_no_invalid_state_error_on_exit_waiters(self):
128128
exit_waiter = self.loop.create_future()
129129
transport._exit_waiters.append(exit_waiter)
130130

131-
# _connect_pipes hasn't completed, so _pipes_connected is False.
132-
self.assertFalse(transport._pipes_connected)
133-
134131
# Simulate process exit. _try_finish() will set the result on
135132
# exit_waiter because _pipes_connected is False, and then schedule
136133
# _call_connection_lost() because _pipes is empty (vacuously all
@@ -143,6 +140,32 @@ def test_proc_exited_no_invalid_state_error_on_exit_waiters(self):
143140

144141
transport.close()
145142

143+
def test_wait_returns_on_exit_with_open_pipe(self):
144+
# gh-119710: wait() must resolve when the process exits even if a
145+
# pipe is still open and never reaches EOF (e.g. inherited by a
146+
# grandchild). Otherwise _call_connection_lost() never runs and
147+
# _wait() would hang forever despite the returncode being known.
148+
transport, protocol = self.create_transport()
149+
150+
# Pipes are fully connected, but fd 1 stays open (never disconnects).
151+
pipe = mock.Mock()
152+
pipe.disconnected = False
153+
transport._pipes[1] = pipe
154+
155+
# A waiter registered via _wait() before the process exits.
156+
exit_waiter = self.loop.create_future()
157+
transport._exit_waiters.append(exit_waiter)
158+
159+
# _process_exited() must resolve exit_waiter even though the pipe
160+
# never disconnects (so _call_connection_lost() never runs). Without
161+
# the fix, exit_waiter stays pending forever and this hangs.
162+
transport._process_exited(7)
163+
self.loop.run_until_complete(exit_waiter)
164+
165+
self.assertEqual(exit_waiter.result(), 7)
166+
167+
transport.close()
168+
146169

147170
class SubprocessMixin:
148171

@@ -436,6 +459,47 @@ async def len_message(message):
436459
self.assertEqual(output.rstrip(), b'3')
437460
self.assertEqual(exitcode, 0)
438461

462+
def test_wait_even_if_pipe_is_open(self):
463+
# gh-119710: Process.wait() must return once the process exits even
464+
# if its stdout pipe is inherited by a grandchild that keeps it open,
465+
# so the pipe never reaches EOF. Otherwise wait() hangs forever
466+
# despite the returncode being known.
467+
468+
async def run():
469+
# Just setup a pipe to pass to the grandchild for reading to ensure it dies.
470+
# Inheritable is to allow it to be passed on windows
471+
r, w = os.pipe()
472+
os.set_inheritable(r, True)
473+
474+
code = textwrap.dedent(f"""\
475+
import subprocess, sys
476+
subprocess.run([sys.executable, "-c", "import sys;sys.stdin.read()"])
477+
""")
478+
479+
proc = await asyncio.create_subprocess_exec(
480+
sys.executable, "-c", code,
481+
# This will be inherited by granchild and should not prevent
482+
# *this* process from firing .wait().
483+
stdout=subprocess.PIPE,
484+
stdin=r,
485+
pass_fds=(r,) if sys.platform != "win32" else (),
486+
close_fds=False if sys.platform == "win32" else True,
487+
)
488+
os.close(r)
489+
490+
try:
491+
# Ensure we start waiting before the process is killed.
492+
wait_proc = asyncio.create_task(proc.wait())
493+
await asyncio.sleep(0.1)
494+
proc.kill()
495+
await asyncio.wait_for(wait_proc, timeout=2.0)
496+
finally:
497+
os.close(w) # Allows the grandchild to exit
498+
if proc.stdout is not None:
499+
await proc.stdout.read()
500+
501+
self.loop.run_until_complete(run())
502+
439503
def test_empty_input(self):
440504

441505
async def empty_input():
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix :mod:`asyncio` subprocess :meth:`~asyncio.subprocess.Process.wait`
2+
hanging when the process has exited but one of its pipes is kept open by an
3+
inherited child process (so the pipe never reaches EOF). ``wait()`` now
4+
returns as soon as the process exits, regardless of the pipes' state.

0 commit comments

Comments
 (0)