Skip to content
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.

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

add @wrapped_async_generator #293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

belm0
Copy link
Contributor

@belm0 belm0 commented May 16, 2021

TODO: add to documentation

cc: @smurfix

@agronholm
Copy link
Owner

This still has a task group straddling a yield which invalidates it in situations where cancel scope stack corruption can happen (e.g. pytest fixtures). And in other situations it's not necessary. So, please elaborate?

@belm0
Copy link
Contributor Author

belm0 commented May 16, 2021

This still has a task group straddling a yield which invalidates it in situations where cancel scope stack corruption can happen (e.g. pytest fixtures). And in other situations it's not necessary. So, please elaborate?

I'm not an expert on this code (it's based on a prototype by Joshua Oreman), but I think the answer is this:

This is still technically suspending an async generator while it has nurseries/cancel scopes open, but the task that's iterating the async generator doesn't exit or enter any nurseries or cancel scopes while the async generator is suspended, and the exception forwarding effectively puts send_channel.send() into the same context as the async generator's yield.

python-trio/trio#638 (comment)

This wrapper absolutely has real world use cases, and the application at my workplace relies on it.

@agronholm
Copy link
Owner

This wrapper absolutely has real world use cases, and the application at my workplace relies on it.

I would be interested in seeing this use case if you don't mind. If it makes a strong case then I will think about merging this PR.

@belm0
Copy link
Contributor Author

belm0 commented May 16, 2021

This wrapper absolutely has real world use cases, and the application at my workplace relies on it.

I would be interested in seeing this use case if you don't mind. If it makes a strong case then I will think about merging this PR.

The problem and need is clearly stated in the Trio documentation.

https://trio.readthedocs.io/en/stable/reference-core.html#cancel-scopes-and-nurseries

@agronholm
Copy link
Owner

I am quite aware of the problem (as I am currently writing an expansion of the relevant AnyIO documentation) but I am interested in whatever real world uses cases this PR solves, and whether other workarounds could be better.

@belm0
Copy link
Contributor Author

belm0 commented May 16, 2021

It's not hard to imagine cases where you want to use a nursery or cancel scope within an async generator, and people are hitting these cases in real life (we did, and Michael Elsdörfer who filed the trio bug did).

The case in our codebase is basically "async generator needs to call out to an external service for each item, and needs to apply a timeout to the individual or aggregate calls".

@agronholm
Copy link
Owner

It's not hard to imagine cases where you want to use a nursery or cancel scope within an async generator, and people are hitting these cases in real life (we did, and Michael Elsdörfer who filed the trio bug did).

Maybe for you it isn't, but I have never written code like that which is why I'm asking to see a real world example. Do I have to ask a third time?

@o-fedorov
Copy link

o-fedorov commented Jan 18, 2022

Hi. I suppose this code would be a solution for my question asked in StackOwerflow here: https://stackoverflow.com/q/70037576/2069071

The example provided there is quite simplified, in reality there are multiple calls to other APIs that provide paginated responses, so the responses are wrapped in async generators. Though, without task groups the responses are currently processed and provided to the user sequentially.

CC @agronholm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants