-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix interoperability with native async generators #14
Conversation
95b5d14
to
6aa8b84
Compare
Codecov Report
@@ Coverage Diff @@
## master #14 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 7 7
Lines 776 817 +41
Branches 59 70 +11
=====================================
+ Hits 776 817 +41
Continue to review full report at Codecov.
|
gentle ping |
Sorry for the delay! This is some impressive hacking. But I have to admit, I'm not as impressed with the whole idea as I was when I first wrote it... it's a lot of complexity for a cute gimmick that only works on CPython :-). So while the code looks fine on a quick once-over, I'm not sure if we actually want the functionality or not. (Sorry if the comments misled you here, I had sort of forgotten about those...) I'd be interested in any thoughts on this -- for example, do you have a use case? I assume you had some reason for digging into this :-) |
I didn't have a specific reason at first - I came across the commented-out code and thought, "this sounds fun to work out!", so I did. Thinking about it some more... I think the only real utility is if we could use this to make @async_generator automatically produce a native async generator on 3.6+, which would have both interoperability and performance improvements. Interoperability is of limited use given that native async generators can't use "yield from", and we may or may not care about the performance - I know your feelings about microbenchmarks. :-) Since some numbers are better than none, though, I extended the microbenchmark from PEP 525 to compare the analogous @async_generator and two ways I thought of for making a looks-like-an-@async_generator-but-is-actually-native:
results:
Would you be receptive to a patch that makes @async_generator work like agen4 in this example on CPython 3.6+? I know the "transmute an async generator to a regular generator" operation is sketchy, but I looked through genobject.c and I'm pretty well convinced it's safe -- an async generator object is a generator object with a few extra fields tacked on at the end, only one of which is an object pointer (ag_finalizer) and that's NULL until the first call to asend()/athrow()/etc (which we never invoke on our wrapper_maker). |
Wow, that's super clever. I like that it's much simpler than the current version of this hack, and that it can be motivated as an invisible optimization. (Maybe we wouldn't even want to document that I suspect you'll run into a bit of trouble getting that to pass the current test suite, because we have some tests to check that we don't suffer from bpo-32526, and native async generators do suffer from that bug. Someone should probably fix that at some point... But I guess merely working as well as native async generators would be OK, and we could mark that test as I think this will also cause behavioral differences with respect to the async generator GC hooks. Currently |
I've started working on implementing this for async_generator, and the biggest wrinkle I've run into is that native async generators don't support returning values other than None. Combined with the conversion of explicitly raised StopAsyncIteration exceptions into RuntimeErrors, this means there's (AFAICT) no way to make an @async_generator, implemented as above as a native async generator awaiting an async function that yields magic wrapper objects, return something. This breaks a few of the tests, mostly of |
@oremanj Oh, I see, that's unfortunate.
Historically, it's been both a handy backport library, and also a laboratory for future technology... in particular, 3.6's native async generators were actually inspired by this library rather than vice versa (which is why the implementation is sufficiently similar to allow for these wacky hacks!), and the I hate saying this, but: we need to keep the old implementation around anyway for 3.5 and for PyPy. Maybe we should expose both and let users pick? |
- Revigorate support for using the native AsyncGenWrappedValue type as our wrapper type on CPython 3.6+. Instead of trying to manually construct the object using ctypes, trick the interpreter into doing the wrapping/unwrapping for us. - Add support for PEP 525 async generator hooks (like `sys.set_asyncgen_hooks()`); on 3.6+, our hooks are the same as the system ones, and regardless of version they can be accessed via `async_generator.set_asyncgen_hooks()` and so on - Add the ability for `@async_generator` to produce a native async generator. Automatically detect whether this is possible (we need to be on CPython 3.6+ and the async function we were given needs to only ever return None). Since there are a few minor semantic differences, native mode currently must be requested using `@async_generator_native`, and plain `@async_generator` warns on code whose behavior would differ with _native. In a future release we can make _native the default, and require people who really need ag_running to work correctly (for example) to use `@async_generator_legacy`.
Codecov Report
@@ Coverage Diff @@
## master #14 +/- ##
======================================
Coverage 100% 100%
======================================
Files 7 7
Lines 776 1064 +288
Branches 59 98 +39
======================================
+ Hits 776 1064 +288
Continue to review full report at Codecov.
|
Here's a substantial rewrite of the patch that I think accomplishes all of our goals:
|
- refcount is a size_t, not a long (they're different sizes on Windows x64) - in AsyncGenerator.__del__, ensure the athrow awaitable is closed; otherwise pypy might complain about reentering the generator - disable test_gc_hooks_behavior on pypy for now; it's segfaulting for as-yet-unclear reasons
I reported the pypy segfault: https://bitbucket.org/pypy/pypy/issues/2786/pypy3-interpreter-segfault-somewhere |
…n blindly skipif'ing
This is kind of an unholy amalgamation of three separate changes at this point, so I'm going to close it and make new PRs for the separate changes. |
sys.set_asyncgen_hooks()
); on 3.6+, our hooks are the same as the system ones, and regardless of version they can be accessed viaasync_generator.set_asyncgen_hooks()
and so on@async_generator
to produce a native async generator. Automatically detect whether this is possible (we need to be on CPython 3.6+ and the async function we were given needs to only ever return None). Since there are a few minor semantic differences, native mode currently must be requested using@async_generator_native
, and plain@async_generator
warns on code whose behavior would differ with _native. In a future release we can make _native the default, and require people who really need ag_running to work correctly (for example) to use@async_generator_legacy
.