-
Notifications
You must be signed in to change notification settings - Fork 8
Improvements to loop lifecycle #152
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
base: main
Are you sure you want to change the base?
Conversation
|
Oh, this also fixes a bug in our |
|
Current CI failures show that the current implementation suffers from the problem that I must say that the issue seems pretty niche: it applies to an async generator that is unfinished, and that has a ref somewhere, preventing the gc from cleaning it up. But fair enough. |
|
For some extra context how this relates to wgpu-py loop integration. The Something like this: def sniff_io_lib():
ob, ob2 = sys.get_asyncgen_hooks()
if ob is None:
ob = ob2
if ob is not None:
try:
return ob.__module__.split(".", 1)[0]
except AttributeError:
pass |
|
Another rabbithole ... in order to use So I spend some time to make major improvements to the lifecycle management. Adding multiple tests, and fixing many subtle issues. Should be about done, but I will do a bit of testing on Windows and Linux on Monday. |
| def __setup_asyncgen_hooks(self): | ||
| # We employ a simple strategy to deal with lingering async generators, | ||
| # in which we attempt to sync-close them. This fails (only) when the | ||
| # finalizer of the agen has an await in it. Technically this is allowed, | ||
| # but it's probably not a good idea, and it would make it hard for us, | ||
| # because we want to be able to stop synchronously. So when this happens | ||
| # we log an error with a hint on how to cleanly (asynchronously) close | ||
| # the generator in the user's code. Note that when a proper async | ||
| # framework (asyncio or trio) is used, all of this does not apply; only | ||
| # for the qt/wx/raw loop do we do this, an in these cases we don't | ||
| # expect fancy async stuff. | ||
|
|
||
| current_asyncgen_hooks = sys.get_asyncgen_hooks() | ||
| if ( | ||
| current_asyncgen_hooks.firstiter is None | ||
| and current_asyncgen_hooks.finalizer is None | ||
| ): | ||
| sys.set_asyncgen_hooks( | ||
| firstiter=self._asyncgen_firstiter_hook, | ||
| finalizer=self._asyncgen_finalizer_hook, | ||
| ) | ||
| else: | ||
| # Assume that the hooks are from asyncio/trio on which this loop is running. | ||
| pass | ||
|
|
||
| def __finish_asyncgen_hooks(self): | ||
| sys.set_asyncgen_hooks(None, None) | ||
|
|
||
| if len(self._asyncgens): | ||
| closing_agens = list(self._asyncgens) | ||
| self._asyncgens.clear() | ||
| for agen in closing_agens: | ||
| close_agen(agen) | ||
|
|
||
| def _asyncgen_firstiter_hook(self, agen): | ||
| self._asyncgens.add(agen) | ||
|
|
||
| def _asyncgen_finalizer_hook(self, agen): | ||
| self._asyncgens.discard(agen) | ||
| close_agen(agen) |
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 is the relevant code specific to the async-generators. The rest is improvements to lifecycle so it's wel-defined when setup/finish is called.
Summary
set_asyncgen_hooks()Context
In looking for an alternative approach to sniffio, I ran into
sys.set_asyncgen_hooks()andsys.get_asyncgen_hooks()part of pep-0525. From what I understand, our async-adapter should use those to make sure that any unfinished async adapters get cleaned up properly.So I wrote some tests ... but they already pass, and I have not (yet) been able to come up with a test that fails with the current implementation (which does not use
get_asyncgen_hooks()).Nevertheless good to have these tests in place 🤷