Add support for PEP 525-style garbage collection hooks by oremanj · Pull Request #15 · python-trio/async_generator

oremanj · 2018-04-05T20:25:49Z

Add async_generator.get_asyncgen_hooks() and async_generator.set_asyncgen_hooks(), and use them under the same circumstances the equivalents in sys would be used for native generators. On 3.6+, the async_generator names refer to the same functions as the sys names, so there's only one set of hooks, and frameworks that use sys.set_asyncgen_hooks() should work seamlessly with async_generator functions as well.

There's an issue with GC ordering or something in pypy, which causes the new logic to make the interpreter segfault under some circumstances, so keep the old logic (which complains if any async_generator is GC'ed before its execution has completed) if we detect that we're running under pypy. The bug has been reported upstream: https://bitbucket.org/pypy/pypy/issues/2786/

codecov · 2018-04-05T20:29:11Z

Codecov Report

Merging #15 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master    #15    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files           7      7            
  Lines         776    972   +196     
  Branches       59     77    +18     
======================================
+ Hits          776    972   +196

Impacted Files	Coverage Δ
async_generator/__init__.py	`100% <ø> (ø)`	⬆️
async_generator/_tests/test_async_generator.py	`100% <100%> (ø)`	⬆️
async_generator/_impl.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17521f8...c9168c1. Read the comment docs.

njsmith · 2018-04-06T03:33:04Z

async_generator/_impl.py

+        if coro_state is CORO_CREATED:
+            (firstiter, self._finalizer) = get_asyncgen_hooks()
+            if firstiter is not None:
+                firstiter(self)


This could call the firstiter hook twice:

agen = agen_function() agen.asend(None).close() agen.asend(None).close()

Pretty obscure, but native agens don't seem to call it twice in this case. We should add a test case, and I guess an explicit have-we-done-hook-setup variable.

Thanks for spotting! Fixed and added a test.

njsmith · 2018-04-06T03:35:57Z

async_generator/_impl.py

+
+        async def step():
+            if self.ag_running:
+                raise ValueError("async generator already executing")


What do you think of moving this check, and the self.ag_running = True, up out of the async def?

There would be one tricky thing to worry about: if a coroutine object gets closed or GCed before it even starts, then python doesn't enforce its normal guarantee that finally blocks will be executed. So if we did do this I guess we'd need to add another yield at the top of step, and immediately iterate it that far when we created the coroutine object, in order to guarantee that eventually ag_running = False would happen. Maybe this is more work than it's worth.

I guess the intent would be to guard against two asend() coroutines existing simultaneously, even if one is exhausted before the second is started? Given the implementation difficulties, I don't think it's worth it, since it seems impossible to actually interleave the underlying send() calls in a way that would create confusion. (I originally put the ag_running test outside of step() with the setting-to-True still inside, and that did create potential problems, which is why I added test_reentrance_forbidden_simultaneous_asends.)

njsmith · 2018-04-06T03:37:32Z

async_generator/_impl.py

        state = getcoroutinestate(self._coroutine)
+        if state is CORO_CLOSED or self._closed:
+            return
+        self._closed = True


What's this code for?

It ensures that if aclose() fails due to a yield during stack unwinding, we still consider the generator "as closed as it's going to get", and don't try again to unwind it during __del__. I added a comment to clarify.

ah, nice catch!

njsmith · 2018-04-06T03:40:28Z

async_generator/_tests/test_async_generator.py

-    gen.__del__()
+    for turns in (1, 2, 3):
+        gen = awaits_when_unwinding()
+        for turn in range(1, turns + 1):


If you make it yield 0, 1, 2 then this will be a bit simpler :-)

njsmith · 2018-04-06T03:41:14Z

async_generator/_tests/test_async_generator.py

-    await gen.aclose()
-    # Closed, so no problem
-    gen.__del__()
+    for turns in (1, 2, 3):


Can you rename turns → stop_after_turn or similar, to make it more immediately obvious what this is doing?

njsmith · 2018-04-06T03:43:10Z

async_generator/_tests/test_async_generator.py

+    with pytest.raises(TypeError):
+        set_asyncgen_hooks(finalizer=False)
+
+    def in_thread(results=[]):


nitpick: we always pass results to in_thread, so it should be a mandatory argument (no default)

done (and using a list as a default argument, too - what was I even thinking?)

njsmith · 2018-04-06T03:57:56Z

docs/source/reference.rst

+like you would use ``sys.set_asyncgen_hooks()`` with native
+generators. On Python 3.6+, the former is an alias for the latter,
+so libraries that use the native mechanism should work seamlessly
+with ``@async_generator`` functions.


We should probably include a warning here that not all async libraries will use these hooks. For example presumably trio will use these if it uses any, but if you're using asyncio on 3.5 then it won't do the same async generator cleanup that it does on 3.6+.

good thought - I added a sentence about this. (When we teach trio about these, we can go back and say "but if you're using trio, it will just work!" if we want. :P)

…onality

oremanj · 2018-04-17T03:03:59Z

ping on this?

njsmith · 2018-04-17T05:37:16Z

Thanks!

Add support for PEP 525-style garbage collection hooks

b5c50ce

Add newsfragment for GC hooks

0a49fd9

oremanj requested a review from njsmith April 5, 2018 20:53

njsmith reviewed Apr 6, 2018

View reviewed changes

oremanj added 3 commits April 6, 2018 11:53

Cleanups after code review

cca57d9

Proper workaround for PyPy issue 2786, restoring full GC hooks functi…

c9aa69a

…onality

yapfify

c9168c1

njsmith merged commit 1272b9c into python-trio:master Apr 17, 2018

oremanj mentioned this pull request Apr 17, 2018

Unexpected RuntimeError: partially-exhausted async_generator garbage collected #13

Closed

oremanj mentioned this pull request May 2, 2018

Make yield_ and yield_from_ interoperate with native async generators again #16

Open

Uh oh!

Conversation

oremanj commented Apr 5, 2018

Uh oh!

codecov bot commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oremanj commented Apr 17, 2018

Uh oh!

njsmith commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Apr 5, 2018 •

edited

Loading