Suppress GeneratorExit from .aclose() for as_safe_channel

[A5rocks]

Fixes #3324.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00000%. Comparing base (fc352b2) to head (1af5620).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@               Coverage Diff               @@
##                 main        #3325   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             125          125           
  Lines           19426        19497   +71     
  Branches         1326         1334    +8     
===============================================
+ Hits            19426        19497   +71     

Files with missing lines	Coverage Δ
src/trio/_channel.py	100.00000% <100.00000%> (ø)
src/trio/_tests/test_channel.py	100.00000% <100.00000%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jakkdl]

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

[A5rocks]

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)

[Zac-HD]

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

[A5rocks]

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.

[Zac-HD]

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

[A5rocks]

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.

[jakkdl]

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.

[Zac-HD]

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.

[Zac-HD]

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

[A5rocks]

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.

[Zac-HD]

Consider:

try:
    raise ValueError
except* BaseException as group:
    exits, narrowed_exceptions = group.split(GeneratorExit)

    if exits is None:
        raise
    # or
    if narrowed_exceptions is not None:
        raise narrowed_exceptions from None

both raise an ExceptionGroup containing a ValueError, whereas

try:
    raise ValueError
except* GeneratorExit: 
    pass

just raises a ValueError, which I very strongly prefer.

---

IMO raise eg.exceptions[0] is not viable here, because we also don't know how deeply-nested the GeneratorExit might be.

(I'm also proposing a language change to migitate the attach-the-whole-group issue in this example - python/peps#4568)

---

I'd prefer to put a note in the docstring rather than an assertion, since I expect that to be less disruptive overall, but I expect it to be so rare that I don't really have a strong opinion.

[A5rocks]

Aha I see what you mean now! The logic in this PR only runs in except BaseExceptionGroup:, so single exceptions will remain untouched.

And yeah note in docstring is a nice option. I guess reasoning against it is like, you won't know know that you have two GeneratorExits until you do... I see a nonzero amount of raise GeneratorExit in a quick GitHub search so that's probably not enough. (still thinking about it though)

[Zac-HD]

Let's merge, and then ship a new version 🚀