fix broken-channel bug in @trio.as_safe_channel

[Zac-HD]

We dropped this handler from #3197, presumably because the tests (...or production patterns) which trigger it are fairly esoteric. The if not send_chan._state.open_receive_channels: break clause is actually new, but lets us skip a pointless __anext__ call in many cases which is a nice perf win.

I just wish I'd noticed this in time for #3325, rather than when pulling it in the new version 🤦‍♂️

[A5rocks]

Just realized the "Encountered exception during cleanup of generator object ..." exception group... contains the exception group with multiple exceptions!? That looks weird when printed! I guess I should make a new issue about that. (unless that behavior makes perfect sense to you?)

[A5rocks]

I have a question and a comment about the tests, but this makes sense.

(I have the possibly strange opinion that we should one day have service nurseries, meaning that trio.as_safe_channel would be implemented s.t. the task calling the async generator should never raise... which actually runs counter to "I can close the channel" now that I think about it. Well, that's for another day.)

[A5rocks]

Huh, it's a bit unintuitive to me that it's possible to not run any code within the async generator. But that can happen in sync generators too so I guess it's fine.

[Zac-HD]

Just realized the "Encountered exception during cleanup of generator object ..." exception group... contains the exception group with multiple exceptions!? That looks weird when printed! I guess I should make a new issue about that. (unless that behavior makes perfect sense to you?)

hmm, we could instead use .add_note(""Encountered exception during cleanup ...") on the group with multiple exceptions? The status quo is not ideal IMO, but since we're hopefully not going to see that outside of a bug or keyboard-interrupt etc I don't think it's a big deal.

[A5rocks]

The status quo is not ideal IMO, but since we're hopefully not going to see that outside of a bug or keyboard-interrupt etc I don't think it's a big deal.

Or

async def test8() -> None:
    @trio.as_safe_channel
    async def agen():
        await chan.aclose()
        raise ValueError()
        yield

    async with agen() as chan:
        async for _ in chan:
            pass

(I think. When I run that on main we drop the ValueError sometimes (I guess this is the "missed-alarm" thing you mentioned?) which is uh, awful.)

Anyways, moved that to #3332.

[Zac-HD]

When I run that on main we drop the ValueError sometimes (I guess this is the "missed-alarm" thing you mentioned?) which is uh, awful.

yeah, we should probably have an open issue for that in fact.

[A5rocks]

Yeah turns out this PR doesn't fix that case so I've made #3333. I'm fine with this PR not fixing that.

[Zac-HD]

OK, merging this as an improvement on the status quo; I'd rather not block on a general fix to the cancellation problem.

[codecov]

Codecov Report

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

Additional details and impacted files

@@               Coverage Diff               @@
##                 main        #3331   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             125          125           
  Lines           19547        19582   +35     
  Branches         1336         1341    +5     
===============================================
+ Hits            19547        19582   +35     

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.