Skip to content

Commit d37666d

Browse files
committed
PYTHON-5784 Cover threaded PeriodicExecutor exception path via synchro
Consolidate the target-exception test into the synchro'd source so both AsyncPeriodicExecutor and the threaded PeriodicExecutor are covered from a single source, and delete the async-only test file. Why this reverses the async-only split from 3340959: that commit carved the exception test out into test_async_periodic_executor.py to dodge the noisy traceback an uncaught exception prints via threading.excepthook on the background thread. But the two _run loops handle the exception differently -- the threaded version additionally takes the lock and sets _thread_will_exit, which feeds the join-on-reopen path in open(). The async test therefore did not cover that sync-only branch, leaving a real gap (also flagged by Copilot). Rather than maintain two near-identical hand-written tests (which cuts against the synchro direction this PR has otherwise followed), the test now lives in the async source with an `if _IS_SYNC:` branch that swaps threading.excepthook to a no-op for the threaded case. Verified the generated sync test passes 15/15 with no flakiness or stderr noise, which confirms the path is testable and there was no real reason to keep it async-only.
1 parent ea0db63 commit d37666d

4 files changed

Lines changed: 70 additions & 49 deletions

File tree

test/asynchronous/test_async_periodic_executor.py

Lines changed: 0 additions & 48 deletions
This file was deleted.

test/asynchronous/test_periodic_executor.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,41 @@ async def target():
142142
await executor.join(timeout=2)
143143
self.assertGreaterEqual(called, 2)
144144

145+
async def test_target_exception_stops_executor(self):
146+
call_count = 0
147+
148+
async def target():
149+
nonlocal call_count
150+
call_count += 1
151+
raise RuntimeError("error")
152+
153+
executor = self._make_executor(target=target)
154+
155+
if _IS_SYNC:
156+
# The exception re-raises on the executor's background thread,
157+
# which would otherwise trigger threading.excepthook and print a
158+
# noisy traceback. Swap it for a no-op for the duration of the test.
159+
original_excepthook = threading.excepthook
160+
threading.excepthook = lambda args: None
161+
self.addCleanup(setattr, threading, "excepthook", original_excepthook)
162+
163+
executor.open()
164+
await executor.join(timeout=2)
165+
if not _IS_SYNC and executor._task is not None and executor._task.done():
166+
# Retrieve the exception to avoid "Task exception was never
167+
# retrieved" warnings when the task is garbage collected.
168+
executor._task.exception()
169+
self.assertEqual(call_count, 1, "target should stop after raising")
170+
171+
# Re-opening after an exception restarts the executor. For the threaded
172+
# PeriodicExecutor this also exercises the _thread_will_exit join path
173+
# in open().
174+
executor.open()
175+
await executor.join(timeout=2)
176+
if not _IS_SYNC and executor._task is not None and executor._task.done():
177+
executor._task.exception()
178+
self.assertEqual(call_count, 2, "executor should run again after re-open")
179+
145180

146181
if __name__ == "__main__":
147182
unittest.main()

test/test_periodic_executor.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,41 @@ def target():
142142
executor.join(timeout=2)
143143
self.assertGreaterEqual(called, 2)
144144

145+
def test_target_exception_stops_executor(self):
146+
call_count = 0
147+
148+
def target():
149+
nonlocal call_count
150+
call_count += 1
151+
raise RuntimeError("error")
152+
153+
executor = self._make_executor(target=target)
154+
155+
if _IS_SYNC:
156+
# The exception re-raises on the executor's background thread,
157+
# which would otherwise trigger threading.excepthook and print a
158+
# noisy traceback. Swap it for a no-op for the duration of the test.
159+
original_excepthook = threading.excepthook
160+
threading.excepthook = lambda args: None
161+
self.addCleanup(setattr, threading, "excepthook", original_excepthook)
162+
163+
executor.open()
164+
executor.join(timeout=2)
165+
if not _IS_SYNC and executor._task is not None and executor._task.done():
166+
# Retrieve the exception to avoid "Task exception was never
167+
# retrieved" warnings when the task is garbage collected.
168+
executor._task.exception()
169+
self.assertEqual(call_count, 1, "target should stop after raising")
170+
171+
# Re-opening after an exception restarts the executor. For the threaded
172+
# PeriodicExecutor this also exercises the _thread_will_exit join path
173+
# in open().
174+
executor.open()
175+
executor.join(timeout=2)
176+
if not _IS_SYNC and executor._task is not None and executor._task.done():
177+
executor._task.exception()
178+
self.assertEqual(call_count, 2, "executor should run again after re-open")
179+
145180

146181
if __name__ == "__main__":
147182
unittest.main()

tools/synchro.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,6 @@ def async_only_test(f: str) -> bool:
190190
"test_async_loop_safety.py",
191191
"test_async_contextvars_reset.py",
192192
"test_async_loop_unblocked.py",
193-
"test_async_periodic_executor.py",
194193
]
195194

196195

0 commit comments

Comments
 (0)