Skip to content

Commit 9f8bdcc

Browse files
gh-159689: Fix ProcessPoolExecutor hang when a worker is killed mid-result
A worker killed after writing part of a large result left the manager thread blocked in Connection.recv(). The parent holds the result pipe's write end, so that death is not delivered as EOF. Watch worker sentinels for the rest of the message and raise BrokenProcessPool.
1 parent 999855b commit 9f8bdcc

4 files changed

Lines changed: 231 additions & 23 deletions

File tree

‎Lib/concurrent/futures/process.py‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
# This import is required to load the multiprocessing.connection submodule
5353
# so that it can be accessed later as `mp.connection`
5454
import multiprocessing.connection
55+
from multiprocessing.connection import _SentinelReady
5556
from multiprocessing.queues import Queue
5657
import threading
5758
import weakref
@@ -518,8 +519,16 @@ def wait_result_broken_or_wakeup(self):
518519
result_item = None
519520
if result_reader in ready:
520521
try:
521-
result_item = result_reader.recv()
522+
# Keep watching worker sentinels for the rest of the
523+
# message. recv() otherwise blocks in plain reads, and the
524+
# parent's write end of this pipe prevents a killed worker
525+
# from delivering EOF. gh-159689.
526+
result_item = result_reader.recv(sentinels=worker_sentinels)
522527
is_broken = False
528+
except _SentinelReady:
529+
# A worker exited while its result was only partly written.
530+
# Leave is_broken set so the exit code is reported.
531+
pass
523532
except BaseException as exc:
524533
cause = format_exception(exc)
525534

‎Lib/multiprocessing/connection.py‎

Lines changed: 104 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,20 @@ def _init_timeout(timeout=CONNECTION_TIMEOUT):
6565
def _check_timeout(t):
6666
return time.monotonic() > t
6767

68+
69+
class _SentinelReady(Exception):
70+
"""A wait sentinel became readable while a connection read was in progress.
71+
72+
Used by ProcessPoolExecutor so a worker killed mid-result does not leave
73+
the manager thread blocked in a read that can never see EOF. The parent's
74+
copy of the result pipe's write end stays open for the executor's
75+
lifetime, so the death of the writer is not delivered as end-of-file.
76+
"""
77+
78+
def __init__(self, ready):
79+
super().__init__(ready)
80+
self.ready = ready
81+
6882
#
6983
#
7084
#
@@ -253,11 +267,18 @@ def recv_bytes_into(self, buf, offset=0):
253267
(offset + size) // itemsize])
254268
return size
255269

256-
def recv(self):
257-
"""Receive a (picklable) object"""
270+
def recv(self, *, sentinels=()):
271+
"""Receive a (picklable) object.
272+
273+
*sentinels* is an optional sequence of objects accepted by
274+
:func:`wait`, such as process sentinels. Before each read, those
275+
objects are waited on together with this connection. If one of them
276+
is ready and this connection is not, :class:`_SentinelReady` is
277+
raised instead of blocking until the rest of the message arrives.
278+
"""
258279
self._check_closed()
259280
self._check_readable()
260-
buf = self._recv_bytes()
281+
buf = self._recv_bytes(sentinels=sentinels)
261282
return _ForkingPickler.loads(buf.getbuffer())
262283

263284
def poll(self, timeout=0.0):
@@ -317,7 +338,31 @@ def _send_bytes(self, buf):
317338
assert err == 0
318339
assert nwritten == len(buf)
319340

320-
def _recv_bytes(self, maxsize=None):
341+
def _wait_overlapped_or_sentinels(self, ov, sentinels):
342+
"""Block until *ov* completes, or a sentinel is signaled."""
343+
if not sentinels:
344+
waitres = _winapi.WaitForMultipleObjects(
345+
[ov.event], False, INFINITE)
346+
assert waitres == WAIT_OBJECT_0
347+
return
348+
sentinels = list(sentinels)
349+
handles = [ov.event, *(int(item) for item in sentinels)]
350+
waitres = _winapi.WaitForMultipleObjects(handles, False, INFINITE)
351+
if waitres == WAIT_OBJECT_0:
352+
return
353+
ov.cancel()
354+
try:
355+
ov.GetOverlappedResult(True)
356+
except OSError:
357+
pass
358+
idx = waitres - WAIT_OBJECT_0
359+
if not 0 < idx <= len(sentinels):
360+
idx = waitres - WAIT_ABANDONED_0
361+
if 0 < idx <= len(sentinels):
362+
raise _SentinelReady([sentinels[idx - 1]])
363+
raise OSError("failed waiting for pipe data")
364+
365+
def _recv_bytes(self, maxsize=None, sentinels=()):
321366
if self._got_empty_message:
322367
self._got_empty_message = False
323368
return io.BytesIO()
@@ -329,23 +374,29 @@ def _recv_bytes(self, maxsize=None):
329374

330375
sentinel = object()
331376
return_value = sentinel
377+
sentinel_ready = False
332378
try:
333379
try:
334380
if err == _winapi.ERROR_IO_PENDING:
335-
waitres = _winapi.WaitForMultipleObjects(
336-
[ov.event], False, INFINITE)
337-
assert waitres == WAIT_OBJECT_0
381+
self._wait_overlapped_or_sentinels(
382+
ov, sentinels)
383+
except _SentinelReady:
384+
# Already cancelled and reaped above.
385+
sentinel_ready = True
386+
raise
338387
except:
339388
ov.cancel()
340389
raise
341390
finally:
342-
nread, err = ov.GetOverlappedResult(True)
343-
if err == 0:
344-
f = io.BytesIO()
345-
f.write(ov.getbuffer())
346-
return_value = f
347-
elif err == _winapi.ERROR_MORE_DATA:
348-
return_value = self._get_more_data(ov, maxsize)
391+
if not sentinel_ready:
392+
nread, err = ov.GetOverlappedResult(True)
393+
if err == 0:
394+
f = io.BytesIO()
395+
f.write(ov.getbuffer())
396+
return_value = f
397+
elif err == _winapi.ERROR_MORE_DATA:
398+
return_value = self._get_more_data(
399+
ov, maxsize, sentinels)
349400
except:
350401
if return_value is sentinel:
351402
raise
@@ -365,11 +416,28 @@ def _poll(self, timeout):
365416
return True
366417
return bool(wait([self], timeout))
367418

368-
def _get_more_data(self, ov, maxsize):
419+
def _get_more_data(self, ov, maxsize, sentinels=()):
369420
buf = ov.getbuffer()
370421
f = io.BytesIO()
371422
f.write(buf)
372423
left = _winapi.PeekNamedPipe(self._handle)[1]
424+
while left == 0 and sentinels:
425+
# The rest of this message has not been written yet. The
426+
# parent keeps its own write end open, so a killed writer
427+
# does not surface as EOF. Poll the sentinels directly:
428+
# waiting on this pipe here would start another overlapped
429+
# read and consume the message.
430+
handles = [int(item) for item in sentinels]
431+
waitres = _winapi.WaitForMultipleObjects(handles, False, 50)
432+
if waitres == WAIT_TIMEOUT:
433+
left = _winapi.PeekNamedPipe(self._handle)[1]
434+
continue
435+
idx = waitres - WAIT_OBJECT_0
436+
if not 0 <= idx < len(handles):
437+
idx = waitres - WAIT_ABANDONED_0
438+
if 0 <= idx < len(handles):
439+
raise _SentinelReady([sentinels[idx]])
440+
raise OSError("failed waiting for pipe data")
373441
assert left > 0
374442
if maxsize is not None and len(buf) + left > maxsize:
375443
self._bad_message_length()
@@ -407,11 +475,27 @@ def _send(self, buf, write=_write):
407475
break
408476
buf = buf[n:]
409477

410-
def _recv(self, size, read=_read):
478+
def _abandon_read_if_sentinel_ready(self, sentinels):
479+
"""Raise if a sentinel is ready and this connection has no data.
480+
481+
A non-blocking poll covers the common case where the next chunk is
482+
already buffered, so a large result does not block once per chunk.
483+
"""
484+
ready = wait([self, *sentinels], timeout=0)
485+
if self in ready:
486+
return
487+
if not ready:
488+
ready = wait([self, *sentinels])
489+
if self not in ready:
490+
raise _SentinelReady(ready)
491+
492+
def _recv(self, size, sentinels=(), read=_read):
411493
buf = io.BytesIO()
412494
handle = self._handle
413495
remaining = size
414496
while remaining > 0:
497+
if sentinels:
498+
self._abandon_read_if_sentinel_ready(sentinels)
415499
to_read = min(BUFSIZE, remaining)
416500
chunk = read(handle, to_read)
417501
n = len(chunk)
@@ -447,15 +531,15 @@ def _send_bytes(self, buf):
447531
# to avoid "broken pipe" errors if the other end closed the pipe.
448532
self._send(header + buf)
449533

450-
def _recv_bytes(self, maxsize=None):
451-
buf = self._recv(4)
534+
def _recv_bytes(self, maxsize=None, sentinels=()):
535+
buf = self._recv(4, sentinels)
452536
size, = struct.unpack("!i", buf.getvalue())
453537
if size == -1:
454-
buf = self._recv(8)
538+
buf = self._recv(8, sentinels)
455539
size, = struct.unpack("!Q", buf.getvalue())
456540
if maxsize is not None and size > maxsize:
457541
return None
458-
return self._recv(size)
542+
return self._recv(size, sentinels)
459543

460544
def _poll(self, timeout):
461545
r = wait([self], timeout)

‎Lib/test/test_concurrent_futures/test_process_pool.py‎

Lines changed: 114 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
1+
import multiprocessing as mp
12
import os
23
import queue
4+
import signal
5+
import struct
36
import sys
47
import threading
58
import time
@@ -16,8 +19,8 @@
1619

1720
from .executor import ExecutorTest, mul
1821
from .util import (
19-
ProcessPoolForkMixin, ProcessPoolForkserverMixin, ProcessPoolSpawnMixin,
20-
create_executor_tests, setup_module)
22+
BaseTestCase, ProcessPoolForkMixin, ProcessPoolForkserverMixin,
23+
ProcessPoolSpawnMixin, create_executor_tests, setup_module)
2124

2225

2326
class EventfulGCObj():
@@ -50,6 +53,40 @@ def _report_wait_return(queue, event, value):
5053
return value
5154

5255

56+
def _send_result_header_only(writer):
57+
"""Write a length header and exit before the message body."""
58+
writer._send(struct.pack("!i", 100_000))
59+
60+
61+
def _send_object(writer):
62+
writer.send({"ok": True})
63+
64+
65+
def _return_large_result():
66+
# Larger than Connection._send_bytes' 16384-byte inline threshold, so the
67+
# length header is written separately from the body.
68+
return b"x" * (32 * 1024)
69+
70+
71+
def _pause_after_result_header(writer):
72+
"""Stop this worker after the result length is written, before the body."""
73+
import multiprocessing.connection as connection
74+
75+
original = connection.Connection._send_bytes
76+
77+
def _send_bytes(self, buf):
78+
n = len(buf)
79+
if n > 16384:
80+
self._send(struct.pack("!i", n))
81+
writer.send_bytes(b"1")
82+
os.kill(os.getpid(), signal.SIGSTOP)
83+
self._send(buf)
84+
else:
85+
original(self, buf)
86+
87+
connection.Connection._send_bytes = _send_bytes
88+
89+
5390
class ProcessPoolExecutorTest(ExecutorTest):
5491

5592
@unittest.skipUnless(sys.platform=='win32', 'Windows-only process limit')
@@ -58,6 +95,33 @@ def test_max_workers_too_large(self):
5895
"max_workers must be <= 61"):
5996
futures.ProcessPoolExecutor(max_workers=62)
6097

98+
@unittest.skipUnless(hasattr(signal, "SIGSTOP"),
99+
"requires SIGSTOP to pause a worker mid-send")
100+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
101+
def test_killed_child_during_result_send(self):
102+
# gh-159689: a worker killed after writing the result length but
103+
# before the body must fail the future. The parent holds the
104+
# result pipe's write end, so this is not delivered as EOF.
105+
ctx = self.get_context()
106+
reader, writer = ctx.Pipe(duplex=False)
107+
executor = self.executor_type(
108+
max_workers=1,
109+
mp_context=ctx,
110+
initializer=_pause_after_result_header,
111+
initargs=(writer,))
112+
try:
113+
future = executor.submit(_return_large_result)
114+
self.assertTrue(reader.poll(support.SHORT_TIMEOUT))
115+
self.assertEqual(reader.recv_bytes(), b"1")
116+
worker = next(iter(executor._processes.values()))
117+
os.kill(worker.pid, signal.SIGKILL)
118+
with self.assertRaises(BrokenProcessPool):
119+
future.result(timeout=support.SHORT_TIMEOUT)
120+
finally:
121+
executor.shutdown(wait=True, cancel_futures=True)
122+
writer.close()
123+
reader.close()
124+
61125
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
62126
def test_killed_child(self):
63127
# When a child process is abruptly terminated, the whole pool gets
@@ -543,6 +607,54 @@ def test_force_shutdown_workers_stops_pool(self, function_name):
543607
break
544608

545609

610+
@unittest.skipUnless(sys.platform != "win32",
611+
"partial pipe reads are specific to Unix connections")
612+
class ResultPipeSentinelTest(BaseTestCase):
613+
"""Connection.recv(sentinels=...) when a writer dies mid-message.
614+
615+
The parent keeps its copy of the write end, which is how
616+
ProcessPoolExecutor's result queue is set up. gh-159689.
617+
"""
618+
619+
def _context(self):
620+
return mp.get_context("spawn")
621+
622+
def test_partial_message_raises_when_writer_exits(self):
623+
ctx = self._context()
624+
reader, writer = ctx.Pipe(duplex=False)
625+
proc = ctx.Process(target=_send_result_header_only, args=(writer,))
626+
try:
627+
proc.start()
628+
proc.join(support.SHORT_TIMEOUT)
629+
self.assertEqual(proc.exitcode, 0)
630+
started = time.monotonic()
631+
with self.assertRaises(mp.connection._SentinelReady):
632+
reader.recv(sentinels=[proc.sentinel])
633+
self.assertLess(time.monotonic() - started, support.SHORT_TIMEOUT)
634+
finally:
635+
writer.close()
636+
reader.close()
637+
proc.join(support.SHORT_TIMEOUT)
638+
proc.close()
639+
640+
def test_complete_message_still_received(self):
641+
ctx = self._context()
642+
reader, writer = ctx.Pipe(duplex=False)
643+
proc = ctx.Process(target=_send_object, args=(writer,))
644+
try:
645+
proc.start()
646+
proc.join(support.SHORT_TIMEOUT)
647+
self.assertEqual(proc.exitcode, 0)
648+
writer.close()
649+
self.assertEqual(reader.recv(sentinels=[proc.sentinel]),
650+
{"ok": True})
651+
finally:
652+
writer.close()
653+
reader.close()
654+
proc.join(support.SHORT_TIMEOUT)
655+
proc.close()
656+
657+
546658
create_executor_tests(globals(), ProcessPoolExecutorTest,
547659
executor_mixins=(ProcessPoolForkMixin,
548660
ProcessPoolForkserverMixin,
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix :class:`concurrent.futures.ProcessPoolExecutor` hanging when a worker
2+
is killed while writing a large result. The pending future now raises
3+
:exc:`concurrent.futures.BrokenProcessPool`, and the interpreter can exit.

0 commit comments

Comments
 (0)