Skip to content

Suppress GeneratorExit from .aclose() for as_safe_channel #3325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 8, 2025
Merged

Suppress GeneratorExit from .aclose() for as_safe_channel #3325

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e334c94
Suppress `.aclose()`'s GeneratorExit in exception groups
A5rocks Sep 3, 2025
14bb557
Add a newsfragment
A5rocks Sep 3, 2025
1904b98
Fix type issue
A5rocks Sep 3, 2025
7993fac
Address coverage
A5rocks Sep 3, 2025
b1fda4f
Try one more coverage workaround
A5rocks Sep 3, 2025
d98b85e
PR review
A5rocks Sep 4, 2025
ea761df
Appease mypy + PR review
A5rocks Sep 5, 2025
1af5620
Address coverage
A5rocks Sep 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions newsfragments/3324.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Avoid having `trio.as_safe_channel` raise if closing the generator wrapped
`GeneratorExit` in a `BaseExceptionGroup`.
13 changes: 11 additions & 2 deletions src/trio/_channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,8 @@ async def _move_elems_to_channel(
# `async with send_chan` will eat exceptions,
# see https://github.com/python-trio/trio/issues/1559
with send_chan:
# replace try-finally with contextlib.aclosing once python39 is
# dropped:
try:
task_status.started()
while True:
Expand All @@ -582,7 +584,14 @@ async def _move_elems_to_channel(
# Send the value to the channel
await send_chan.send(value)
finally:
# replace try-finally with contextlib.aclosing once python39 is dropped
await agen.aclose()
# work around `.aclose()` not suppressing GeneratorExit in an
# ExceptionGroup:
# TODO: make an issue on CPython about this
try:
await agen.aclose()
except BaseExceptionGroup as exceptions:
_, narrowed_exceptions = exceptions.split(GeneratorExit)
if narrowed_exceptions is not None:
raise narrowed_exceptions from None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible that we split out multiple GeneratorExit here and suppress one too many?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if someone raises GeneratorExit in a child task + aclose raises in a nursery. I'm not sure if we should care though, since for instance .close doesn't care if you raise the GeneratorExit or if it does. (I forgot to check if Python suppresses GeneratorExit from next(gen) too... I'll check in a bit)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would except* GeneratorExit: pass do what we want here?

Copy link
Contributor Author

@A5rocks A5rocks Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would have the same behavior as the .split done here (well except if there's 1 other exception? Not too sure.), but I think the issue @jakkdl is talking about is that this implementation can suppress a user's exception.

I'm not sure it's worth fixing this case though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't see a way to avoid suppressing a user-raised GeneratorExit, but also that seems incredibly unlikely to arise in practice so 🤷

and if it's equivalent I tend to prefer "less code" pretty strongly

Copy link
Contributor Author

@A5rocks A5rocks Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be possible to reimplement .aclose() and remove the raised GeneratorExit by identity, but I note that without an exception group Python doesn't care about this. (I agree that I would prefer what is currently here over implementing that)

I guess another possibility is to keep every exception but the first GeneratorExit, though that might toss the user-raised one anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python not distinguishing the source of GeneratorExit is a pretty good argument for us not caring either.
We could raise a warning or something if len(_.exceptions) > 1 if we don't wanna bother adding weird logic, it could lead to weird bugs for users, and it's sufficiently rare. Or even do an assert with a message telling them to open an issue if they care about the behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except* GeneratorExit: pass is IMO actually better behavior than the .split()-based approach: the latter wraps whatever we had into an exception group, while except* GeneratorExit: pass leaves bare exceptions untouched.

This is a big deal for us, because the more layers of ExceptionGroup in your tracebacks the harder they get to read, and this utility function is going to be used all the time - dropping a layer from tracebacks was a huge attraction of @trio.as_safe_generator relative to my older helper function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside from that, I'd be keen to merge and ship 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except* GeneratorExit: pass is IMO actually better behavior than the .split()-based approach: the latter wraps whatever we had into an exception group, while except* GeneratorExit: pass leaves bare exceptions untouched.

I thought that too! But it turns out that my understanding of exception groups is not great:

>>> try:
...     raise ExceptionGroup("...", [ValueError("a"), KeyError("b")])
... except* ValueError:
...     pass
...
  + Exception Group Traceback (most recent call last):
  |   File "<python-input-0>", line 2, in <module>
  |     raise ExceptionGroup("...", [ValueError("a"), KeyError("b")])
  | ExceptionGroup: ... (1 sub-exception)
  +-+---------------- 1 ----------------
    | KeyError: 'b'
    +------------------------------------
>>> try:
...     raise ExceptionGroup("...", [ValueError("a"), KeyError("b")])
... except* ValueError:
...     pass
... except* Exception as eg:
...     raise eg.exceptions[0]
...
  + Exception Group Traceback (most recent call last):
  |   File "<python-input-1>", line 2, in <module>
  |     raise ExceptionGroup("...", [ValueError("a"), KeyError("b")])
  | ExceptionGroup: ... (1 sub-exception)
  +-+---------------- 1 ----------------
    | Traceback (most recent call last):
    |   File "<python-input-1>", line 6, in <module>
    |     raise eg.exceptions[0]
    | KeyError: 'b'
    +------------------------------------

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<python-input-1>", line 6, in <module>
    raise eg.exceptions[0]
KeyError: 'b'

So I think .split is literally identical to except* GeneratorExit: pass.

Or even do an assert with a message telling them to open an issue if they care about the behavior.

This feels weird but weighing the amount of time someone might spend figuring out this as a bug vs being bothered by it, I think it makes sense.


return context_manager
47 changes: 46 additions & 1 deletion src/trio/_tests/test_channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from ..testing import Matcher, RaisesGroup, assert_checkpoints, wait_all_tasks_blocked

if sys.version_info < (3, 11):
from exceptiongroup import ExceptionGroup
from exceptiongroup import BaseExceptionGroup, ExceptionGroup

if TYPE_CHECKING:
from collections.abc import AsyncGenerator
Expand Down Expand Up @@ -625,3 +625,48 @@ async def agen(events: list[str]) -> AsyncGenerator[None]:
events.append("body cancel")
raise
assert events == ["body cancel", "agen cancel"]


async def test_as_safe_channel_genexit_exception_group() -> None:
@as_safe_channel
async def agen() -> AsyncGenerator[None]:
try:
async with trio.open_nursery():
yield
except BaseException as e:
assert isinstance(e, BaseExceptionGroup) # noqa: PT017 # we reraise
raise

async with agen() as g:
async for _ in g:
break


async def test_as_safe_channel_does_not_suppress_nested_genexit() -> None:
@as_safe_channel
async def agen() -> AsyncGenerator[None]:
yield

with pytest.RaisesGroup(GeneratorExit):
async with agen() as g, trio.open_nursery():
await g.receive() # this is for coverage reasons
raise GeneratorExit


async def test_as_safe_channel_genexit_filter() -> None:
async def wait_then_raise() -> None:
try:
await trio.sleep_forever()
except trio.Cancelled:
raise ValueError from None

@as_safe_channel
async def agen() -> AsyncGenerator[None]:
async with trio.open_nursery() as nursery:
nursery.start_soon(wait_then_raise)
yield

with pytest.RaisesGroup(ValueError):
async with agen() as g:
async for _ in g:
break
Loading