-
Notifications
You must be signed in to change notification settings - Fork 184
Use asyncgen hooks to call aclose() #1645
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
Open
agrif
wants to merge
3
commits into
amaranth-lang:main
Choose a base branch
from
agrif:aclose-fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
This will completely replace the hooks for the entire process, right? I'm not comfortable merging it like this. Amaranth should never assumed to be the only thing running in the process.
Can we somehow check if this is "our" async generator and pass the generator along if it's not? Glasgow does mix Amaranth asyncgens and asyncio asyncgens, for example.
Barring that, would it be possible to at least detect if the hooks weren't installed before?
What does asyncio do? Does it just blindly replace the hooks?
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.
asynciosets new hooks while it's running, and restores the old hooks when it stops. This is the behavior I replicated here.I think this is fine -- the hooks are thread-local, so as long as each event loop saves/restores the old hooks as execution enters/leaves their control, I think it all works. It's effectively a stack of hooks that corresponds with the call stack of the thread. If Amaranth and
asyncioare both used, then which hooks are active depends on which one is closest in the call stack.That makes sense to me, but I'd be very interested to know if that meshes with how Glasgow mixes the two. I'm trying to be careful, because everything about these hooks feels messy.
Part of why I implemented
firstiteris specifically to preventasynciofrom claiming Simulator asyncgens. Theasynciohooks store all generators it sees on the loop itself, and then tries to close them when the loop closes. If any of those are in use in the simulator at the time, that will raise a RuntimeError like the one I saw.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 agree this should work.
I will test this.