Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
@background_with_channel
#3197base: main
Are you sure you want to change the base?
add
@background_with_channel
#3197Changes from 3 commits
317cb1c
1a7714e
b0b8b02
8584cff
274755f
428dd4b
7542973
86d3b0f
2d11ea2
a5734f6
0b461d2
b86eb54
7936fd2
6e71d4e
1670674
7acf3a0
69a95dc
efe2d00
f78f641
5bfb0c5
54800e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we test a few more cases:
try: ... finally: ...
work in the async generator? What if I throw an error in thefinally
? I expect it'll add the raised error to theExceptionGroup
but would be good to have guarantees in the tests.agen
? (I don't think so?)(To be honest it's a bit weird that things get raised in a
ExceptionGroup
even if I get why.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, messing around with this made me notice that this implementation does not throw
GeneratorExit()
into the generator when exiting the consumer side early - something that a normal async gen would do.I'm not sure if this really matters, but it would break agens that do
will look if possible to restore, or if it should be noted as something to be vary of
edit: or something else weird is going on, that makes no sense, will have to come back on this
edit2: ah this is the interleaved-buffer-thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so yeah what's going on here is the extra buffer from interleaved execution - the
agen
will always be one yield ahead, and theGeneratorExit
will be thrown into it at the second yield if the consumer exits while handling the firstyield
.I'm pretty sure we need a wrapper around the
receive_channel
for this, so_move_elems_to_channel
can block until it's possible to send.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. That makes sense as a root cause and the work around would only work for buffer-less channels.
We could make it so the memory channel always has no buffer, or we could document that increasing buffer size means that behavior will change with regard to what code will run.
Are there even use cases for buffered versions of this? I can obviously think of contrived examples where it would deadlock, but I think if someone explicitly wanted concurrency between the generator and the for loop they could easily implement that themselves using this -- just add another memory channel: (I haven't tested this but just to get the point across)