Use asyncgen hooks to call aclose() #1645
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1645 +/- ##
==========================================
+ Coverage 91.28% 91.29% +0.01%
==========================================
Files 44 44
Lines 11471 11486 +15
Branches 2236 2237 +1
==========================================
+ Hits 10471 10486 +15
Misses 837 837
Partials 163 163 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return | ||
|
|
||
| old_hooks = sys.get_asyncgen_hooks() | ||
| sys.set_asyncgen_hooks(firstiter=firstiter, finalizer=finalizer) |
There was a problem hiding this comment.
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.
asyncio sets 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 asyncio are 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 firstiter is specifically to prevent asyncio from claiming Simulator asyncgens. The asyncio hooks 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.
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.
I agree this should work.
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.
I will test this.
I'll do my best to reproduce and test for this issue before merging it. |
|
The easiest way to find these tests is to add a warning (or exception, or ...) to the very end of |
Async event loops are responsible for cleaning up async generators. This is a best-effort implementation to call aclose() when possible. Reverts amaranth-lang#1590 and closes amaranth-lang#1638.
2 participants
Closes #1638.
I originally had
finalizer()cause a warning if it failed to runaclose(), but this ended up making the tests fairly noisy on Python <= 3.12. Instead, I added a note to the documentation.I don't understand why those same tests did not cause the RuntimeError I saw. I'm worried that there is some subtle issue with the order simulator coroutines are being cleaned up. In the meantime, this fix should at least be an improvement.
Let me know if you'd like any changes.