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 7 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.
Hmm, looking at this again - we want to be sure that we close the channels even if we get an error while starting the generator, which suggests pulling the send channel to the outside:
I also spent a while looking at the spec for
await agen.aclose()
too, but in my experiments I couldn't construct a version which misbehaved with our decorator but was OK without it. (although it's easy enough to get errors both ways!)The tricky part is that I run into basically the same questions as #1559 (comment). I think here my solution gets around that: we
aclose_forcefully()
if there's any error propagating (and if there's an error during that, it should propagate as usual I think?), while if there's no exception yet we do a bareawait ait.aclose()
and allow whatever cleanup logic we might have to run.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.
I don't think there's any problems with not closing the receive channel on starting the generator - if the
nursery.start
call errors then we'll never yield the receive channel to the user, so there's nobody that can get stuck waiting on itThere 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.
oh and also the send channel will be closed in current implementation, so there's really no problem with leaving the receive channel unclosed.
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.
The loop in my prototype evolved from this one by @oremanj, via @belm0 here.
If we could trigger it, it'd have to be a non-
Cancelled
non-BrokenResourceError
, which occurred while waiting inawait send_chan.send(value)
. I think we could in principle get a few things here (e.g.KeyboardInterrupt
, GC-related errors, etc), but in each case calling.aclose()
on the generator and raising from this function without ever throwing the error into the generator seems like a reasonable response to me.So... I might be wrong, but this simpler code looks good to me.
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.
yeah since your version had
except
s forBrokenResourceError
andCancelled
it's only niche stuff that could get sent. And I don't see any reason why the generator should generate another value after thesend
gets cancelled or broken since it'll just fail again.Given that
send
has KI protection I don't even think it can raiseKeyboardInterrupt
(unless that gets raised just as the decorator exits? idk details how that works)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)