Skip to content

Commit

Permalink
pythongh-117293: Fix race condition in run_workers.py (python#117298)
Browse files Browse the repository at this point in the history
The worker thread may still be alive after it enqueues it's last result,
which can lead to a delay of 30 seconds after the test finishes. This
happens much more frequently in the free-threaded build with the GIL
disabled.

This changes run_workers.py to track of live workers by enqueueing a
`WorkerExited()` instance before the worker exits.
  • Loading branch information
colesbury authored and diegorusso committed Apr 17, 2024
1 parent bae5ba6 commit 77cde1f
Showing 1 changed file with 21 additions and 16 deletions.
37 changes: 21 additions & 16 deletions Lib/test/libregrtest/run_workers.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,12 @@ class MultiprocessResult:
err_msg: str | None = None


class WorkerThreadExited:
"""Indicates that a worker thread has exited"""

ExcStr = str
QueueOutput = tuple[Literal[False], MultiprocessResult] | tuple[Literal[True], ExcStr]
QueueContent = QueueOutput | WorkerThreadExited


class ExitThread(Exception):
Expand Down Expand Up @@ -376,8 +380,8 @@ def _runtest(self, test_name: TestName) -> MultiprocessResult:
def run(self) -> None:
fail_fast = self.runtests.fail_fast
fail_env_changed = self.runtests.fail_env_changed
while not self._stopped:
try:
try:
while not self._stopped:
try:
test_name = next(self.pending)
except StopIteration:
Expand All @@ -396,11 +400,12 @@ def run(self) -> None:

if mp_result.result.must_stop(fail_fast, fail_env_changed):
break
except ExitThread:
break
except BaseException:
self.output.put((True, traceback.format_exc()))
break
except ExitThread:
pass
except BaseException:
self.output.put((True, traceback.format_exc()))
finally:
self.output.put(WorkerThreadExited())

def _wait_completed(self) -> None:
popen = self._popen
Expand Down Expand Up @@ -458,8 +463,9 @@ def __init__(self, num_workers: int, runtests: RunTests,
self.log = logger.log
self.display_progress = logger.display_progress
self.results: TestResults = results
self.live_worker_count = 0

self.output: queue.Queue[QueueOutput] = queue.Queue()
self.output: queue.Queue[QueueContent] = queue.Queue()
tests_iter = runtests.iter_tests()
self.pending = MultiprocessIterator(tests_iter)
self.timeout = runtests.timeout
Expand Down Expand Up @@ -497,6 +503,7 @@ def start_workers(self) -> None:
self.log(msg)
for worker in self.workers:
worker.start()
self.live_worker_count += 1

def stop_workers(self) -> None:
start_time = time.monotonic()
Expand All @@ -511,14 +518,18 @@ def _get_result(self) -> QueueOutput | None:

# bpo-46205: check the status of workers every iteration to avoid
# waiting forever on an empty queue.
while any(worker.is_alive() for worker in self.workers):
while self.live_worker_count > 0:
if use_faulthandler:
faulthandler.dump_traceback_later(MAIN_PROCESS_TIMEOUT,
exit=True)

# wait for a thread
try:
return self.output.get(timeout=PROGRESS_UPDATE)
result = self.output.get(timeout=PROGRESS_UPDATE)
if isinstance(result, WorkerThreadExited):
self.live_worker_count -= 1
continue
return result
except queue.Empty:
pass

Expand All @@ -528,12 +539,6 @@ def _get_result(self) -> QueueOutput | None:
if running:
self.log(running)

# all worker threads are done: consume pending results
try:
return self.output.get(timeout=0)
except queue.Empty:
return None

def display_result(self, mp_result: MultiprocessResult) -> None:
result = mp_result.result
pgo = self.runtests.pgo
Expand Down

0 comments on commit 77cde1f

Please sign in to comment.