Skip to content

Commit e703a76

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 abdd7ae commit e703a76

3 files changed

Lines changed: 56 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: 41 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
@@ -436,6 +433,47 @@ async def len_message(message):
436433
self.assertEqual(output.rstrip(), b'3')
437434
self.assertEqual(exitcode, 0)
438435

436+
def test_wait_even_if_pipe_is_open(self):
437+
# gh-119710: Process.wait() must return once the process exits even
438+
# if its stdout pipe is inherited by a grandchild that keeps it open,
439+
# so the pipe never reaches EOF. Otherwise wait() hangs forever
440+
# despite the returncode being known.
441+
442+
async def run():
443+
# Just setup a pipe to pass to the grandchild for reading to ensure it dies.
444+
# Inheritable is to allow it to be passed on windows
445+
r, w = os.pipe()
446+
os.set_inheritable(r, True)
447+
448+
code = textwrap.dedent(f"""\
449+
import subprocess, sys
450+
subprocess.run([sys.executable, "-c", "import sys;sys.stdin.read()"])
451+
""")
452+
453+
proc = await asyncio.create_subprocess_exec(
454+
sys.executable, "-c", code,
455+
# This will be inherited by granchild and should not prevent
456+
# *this* process from firing .wait().
457+
stdout=subprocess.PIPE,
458+
stdin=r,
459+
pass_fds=(r,) if sys.platform != "win32" else (),
460+
close_fds=False if sys.platform == "win32" else True,
461+
)
462+
os.close(r)
463+
464+
try:
465+
# Ensure we start waiting before the process is killed.
466+
wait_proc = asyncio.create_task(proc.wait())
467+
await asyncio.sleep(0.1)
468+
proc.kill()
469+
await asyncio.wait_for(wait_proc, timeout=2.0)
470+
finally:
471+
os.close(w) # Allows the grandchild to exit
472+
if proc.stdout is not None:
473+
await proc.stdout.read()
474+
475+
self.loop.run_until_complete(run())
476+
439477
def test_empty_input(self):
440478

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