gh-135736: Fix asyncio.TaskGroup swallowing errors on GeneratorExit - #154538
gh-135736: Fix asyncio.TaskGroup swallowing errors on GeneratorExit#154538sreehariannam wants to merge 8 commits into
Conversation
| assert not self._tasks | ||
|
|
||
| if self._base_error is not None: | ||
| if isinstance(self._base_error, GeneratorExit): |
There was a problem hiding this comment.
I think the exception handler should be called for all base errors.
There was a problem hiding this comment.
Good catch, fixed — the reporting is no longer gated on isinstance(self._base_error, GeneratorExit), so suppressed sibling-task errors are now reported via call_exception_handler whenever any base error (SystemExit, KeyboardInterrupt, etc.) causes the TaskGroup to exit, not just GeneratorExit. Added test_taskgroup_20f covering the non-GeneratorExit case.
|
All commit authors signed the Contributor License Agreement. |
…, not just GeneratorExit Per review feedback from kumaraditya303: the exception-handler reporting of suppressed sibling-task errors was previously gated on isinstance(self._base_error, GeneratorExit), but any base error (SystemExit, KeyboardInterrupt, etc.) causes the same silent discarding of collected task errors. Broaden the reporting to run whenever self._base_error is set, and add a regression test covering a non-GeneratorExit base error.
331b20c to
f878aeb
Compare
… a custom BaseException TaskGroup._is_base_error() only recognizes SystemExit and KeyboardInterrupt (GeneratorExit is handled separately); an arbitrary BaseException subclass like MyBaseExc never sets self._base_error, so it takes the BaseExceptionGroup-wrapping path instead of the direct-raise path this test meant to exercise, causing CI failures across all platforms. Use KeyboardInterrupt (matching the existing test_taskgroup_20) instead.
|
@kumaraditya303 Following up — I've addressed your comment about the exception handler being gated on CI is green. Would appreciate a re-review when you have a chance. |
|
An alternate fix was merged, if you can restrict this PR to just the logging part then I can review and merge it. |
Per review feedback from kumaraditya303: an alternate fix for the GeneratorExit-wrapped-in-BaseExceptionGroup bug was merged separately (pythongh-154649), so drop the now-redundant GeneratorExit handling from _is_base_error()/_base_error here. Keep only the independent part: reporting sibling task errors via loop.call_exception_handler() when they would otherwise be silently discarded because the "async with" body exited with a SystemExit or KeyboardInterrupt.
|
Done — dropped the |
2 participants
Fixes gh-135736.
Problem
When the body of an
async with asyncio.TaskGroup()block exits via a bareGeneratorExit(for example, because it's inside an async generator that gets closed withaclose()), theGeneratorExitwas wrapped in aBaseExceptionGrouplike any other exception. Since callers ofaclose()/athrow()only know how to handleGeneratorExit(orStopAsyncIteration) coming back out, this surfaced as a spurious, hard-to-debugExceptionGroup: unhandled errors in a TaskGroupinstead of a clean generator close.Fix
TaskGroup._aexit()now treats aGeneratorExitraised by theasync withbody itself as a "base error" (likeSystemExit/KeyboardInterrupt) and re-raises it directly instead of wrapping it, soaclose()sees the exception type it expects.This is deliberately scoped to the body-exception path only.
TaskGroup._is_base_error()itself (used separately in_on_task_done()for exceptions raised by child tasks) is untouched — a task raisingGeneratorExitinternally is not a "close this generator" signal and continues to be wrapped in the group as before (covered by a new test,test_taskgroup_20e).Since only one exception can propagate out through
GeneratorExit-closing APIs, any sibling task errors that would otherwise be silently discarded in this path are now reported vialoop.call_exception_handler()instead of disappearing (test_taskgroup_20c).Testing
Added four tests (
test_taskgroup_20b-test_taskgroup_20e) covering: bare propagation, propagation alongside a failing sibling task (checking the exception handler is invoked), a faithful reproduction of the original report via a real async generator +aclose(), and the unchanged task-level behavior. Verified each new test fails without the fix by reverting it locally and re-running. Fulltest_taskgroupssuite (126 tests) passes.