add @as_safe_channel

[jakkdl]

Mostly just shepherding @Zac-HD's implementation from #638 (comment)

I don't understand all the details of it, esp some of the code paths I'm completely failing how to cover, so gonna need some help. And I'm getting an exception that sometimes disappear.. which seems bad?
Feel free to add code suggestions and/or commit directly to the branch.

I don't know if this fully resolves #638, or if there's docs and/or other stuff that should be added.

I will make https://flake8-async.readthedocs.io/en/latest/rules.html#async900 suggest using this, and perhaps make it enabled by default, once released.

[A5rocks]

I'm not too sold on how some of this is done, but at least now it shouldn't fail CI.

edit: I'm also pretty sure the more correct way to fix the race condition would be using nursery.start instead of nursery.start_soon.

[jakkdl]

I'm not too sold on how some of this is done, but at least now it shouldn't fail CI.

edit: I'm also pretty sure the more correct way to fix the race condition would be using nursery.start instead of nursery.start_soon.

I tracked down the race condition to send_chan.aclose() getting cancelled, swallowing the exception, i.e. #1559

We still want to use start instead of start_soon to make sure the iterator gets properly closed.

The reason it works to move the send_chan closure to across the yield is simply because that allows the exception in _move_elems_to_channel to bubble up into the nursery without getting eaten (except if aclosing(aiterable) eats it), and the send channel can raise as much Cancelled as it wants in context_manager.
We do need to manually call send_chan.close() in _move_elems_to_channel though to halt any iterations over recv_chan.

[codecov]

Codecov Report

Attention: Patch coverage is 97.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 99.97897%. Comparing base (f500503) to head (0030ac8).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/trio/_tests/test_channel.py	96.82540%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@                 Coverage Diff                  @@
##                 main       #3197         +/-   ##
====================================================
- Coverage   100.00000%   99.97897%   -0.02103%     
====================================================
  Files             124         124                 
  Lines           18844       19017        +173     
  Branches         1277        1287         +10     
====================================================
+ Hits            18844       19013        +169     
- Misses              0           3          +3     
- Partials            0           1          +1     

Files with missing lines	Coverage Δ
src/trio/__init__.py	100.00000% <ø> (ø)
src/trio/_channel.py	100.00000% <100.00000%> (ø)
src/trio/_util.py	100.00000% <100.00000%> (ø)
src/trio/_tests/test_channel.py	99.02676% <96.82540%> (-0.97325%)	⬇️

🚀 New features to boost your workflow:

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

[jakkdl]

@Zac-HD if you can show why the inner loop is necessary it'd be great, but I'm kinda suspecting it's a remnant of previous implementations or something - because I can't come up with anything that would hit that code path.

[Zac-HD]

I think this'll help? But haven't run many tests yet...

[jakkdl]

I have no clue why sphinx fails to link the AbstractAsyncContextManager in the type hint, when it works perfectly fine for https://trio.readthedocs.io/en/stable/reference-core.html#trio.open_nursery

[TeamSpen210]

Looks good and useful.

This is unrelated, but while we're editing the async-gen docs, we might want to swap out the async_generator.aclosing reference to point to contextlib first, with async_generator as a backport.

[A5rocks]

The code itself looks good!

[jakkdl]

does anybody have any idea about the RTD fail?

I have no clue why sphinx fails to link the AbstractAsyncContextManager in the type hint, when it works perfectly fine for https://trio.readthedocs.io/en/stable/reference-core.html#trio.open_nursery

[jakkdl]

It's not clear to me whether this is intended to be a drop-in replacement for iterating over an async generator that replicates behavior. The documentation makes it sound that way, but:

• this raises ExceptionGroup? I get this makes sense in the (hopefully rare) case that both the async for body and the async iterator raise errors, but maybe we could raise from_iter from from_body and hope that makes sense? I've kind of talked myself into thinking that raising ExceptionGroup is a good thing but that would require documenting this.

With a buffer size > 0 I don't even think it's all that rare to get multiple errors. But unless we want to remove that functionality I think it's correct to stick to groups. This might make people hesitant to use it as a drop-in-replacement and if that's explicitly what we're targeting we could maybe add a kwarg or an additional decorator that handles it. (and, uh, given how messy it is to convert exceptiongroups into single exceptions maybe that's a thing we want separately?)

(and hopefully explicitly passing strict_exception_groups=True to the open_nursery within.)

good catch, done!

• add @background_with_channel #3197 (comment)

I thought this was gonna be complicated, but turned out not too bad with the addition of a wrapper + semaphore.

I would rather this is drop-in (at least for the above) even if it needs to be made more complicated. Though I don't care for less visible things like "does it run in the task or in another task if there's no buffer".

For example: [...]

I'm not even sure if it's possible to run in the same task and do it correctly? We could in theory make it look like it from the traceback, but not sure that's worth it

[Zac-HD]

I'd be happy to merge this now, or after making one medium-sized design change.

Specifically, I think @A5rocks has a good point about raising ExceptionGroup. If we've got max_buffer_size=0 (which is the default!), then our semaphore ensures that we can't be running user code in the generator concurrently with that in the body of the context manager. I think this is a rare good reason to unwrap the knowably-just-one exception in a group raised by the inner nursery - and that the smaller change relative to raw generators in the default case is a larger benefit than the cost of raising groups or not based on the max_buffer_size argument.

Thoughts?

[jakkdl]

I think if we're going towards single exceptions we should maybe never raise exceptiongroups, and either do raise RuntimeError("multiple errors") from exc_group or stuff the exception from the generator in __context__ (or the other way round?). I'm not sure, all of them sound kinda iffy.

Actually, I think the cleanest is to expose two different decorators:

def background_with_channel():
  ...
def background_with_buffer_channel(buffer: int|None):
  """[...] raises ExceptionGroup [...]"""
  ...

This also allows us to make interleave vs not much more explicit, as opposed to suggesting that anybody can start buffering any generator without any change in behavior around .throw() etc

[Zac-HD]

Let's merge as-is then.

I don't want to split into multiple functions (linter recs etc get harder), we've already disabled .throw() by handing the user a channel instead of a generator object, and imo consistency with the obvious way to do similar things downstream is pretty nice.

[jakkdl]

sorry, with throwing I meant #3197 (comment)

for linter recs we'd just always recommend background_with_channel - and then in its docstring we'd mention the existence of the buffered version in case you want that

[Zac-HD]

#3197 (comment) doesn't bother me, because cancellation is always a possibility you have to handle. It "just so happens" that we always cancel "right before" the GeneratorExit would show up 😉

In practice most users are never going to touch the default, so maybe we should only supply the unbuffered version, and let power-users add their own buffering if desired? We could even share a recipe for the "await send_chan.send(x) instead of yield x" version, which is quite simple and avoids all the complexity of having a generator object in there...

[jakkdl]

In practice most users are never going to touch the default, so maybe we should only supply the unbuffered version, and let power-users add their own buffering if desired? We could even share a recipe for the "await send_chan.send(x) instead of yield x" version, which is quite simple and avoids all the complexity of having a generator object in there...

Okay yeah this seems fine. The only time this would be insufficient is if they're relying on an external asyncgen they can't modify and they want buffering, but if so they can still just write an equivalent version of buffer_it.

But god I am not looking forward to another go at the "oh just unwrap the exception from inside the group"; I'm tempted to go create a helper for that in a new PR, which would be pretty great for trio-websocket and probably others as well: https://github.com/python-trio/trio-websocket/blob/bec4232178700e53dccb887d028997f6746e91de/trio_websocket/_impl.py#L218

[Zac-HD]

OK, I think this is the last round of comments from me:

• Yeah, let's make the RecvChanWrapper clone-able. I don't have a use-case in mind but would prefer to avoid gaps vs the memory-channel interface. changed my mind on this; let's release without .clone() and follow up later if we get a feature request.
• Needs changelog entry etc., since I think it's worth releasing this after we merge (and then updating linters etc.)
• Now that there's no buffering, I think @trio.background_with_channel makes less sense as a name. Thoughts?
  • Current ideas: @trio.safe_(async_)?generator, @trio.generator(_to_|2|_as_)channel, variations. Prior art is named trio_async_generator, and includes buffering (size-zero channel, no semaphore).
  • "background" is just inaccurate now and so I'd fairly strongly prefer to rename it.

[Zac-HD]

false alarm, the simple idea leaks cancellations from the generator into the caller (cf PEP-789) 😞

[A5rocks]

Could we add a test for that in this PR too at least

[Zac-HD]

@generator_as_channel
async def agenfn():
    with trio.CancelScope() as cscope:
        cscope.cancel()
        yield
        
async def test_doesn't_leak_cancellation():
    with pytest.raises(AssertionError):
        async with agenfn() as recv_chan:
            async for _ in recv_chan:
                pass
        raise AssertionError("should be reachable")

[Zac-HD]

Three things below - I'm very keen to merge and thus deferred .clone(), but I also think we need to get the core interface right the first time 😅

• The big-but-easy one is renaming the function @as_safe_channel; see below for why
• We can drop some docs now that the interface is simpler 🎉
• Two small but important correctness changes.

(once more into the diff, dear friends!)

[Zac-HD]

I suggest we ship an initial version without .clone() support, and come back later if anyone asks for it. This will already be super valuable, and I'd rather not wait any longer!

[jakkdl]

I was mostly thinking if we have any strong reasons not to offer it, implementing it should be straightforward.
Though looking at the interface of MemoryReceiveChannel we also don't have receive_nowait on the wrapper and I'm not seeing how that one would make sense to offer - so let's stick to the interface of ReceiveChannel

[Zac-HD]

This looks great to me!

• tiny tweaks for changelog and coverage below
• ✅ Switch from PurePath#as_uri to PurePath#joinpath for our test that PurePath methods get inherited #3249 has fixed the unrelated test failures

then let's ship it!