bpo-39606: allow closing async generators that are already closed #18475

njsmith · 2020-02-12T08:04:57Z

The fix for bpo-39386 attempted to make it so you couldn't reuse a
agen.aclose() coroutine object. It accidentally also prevented you
from calling aclose() at all on an async generator that was already
closed or exhausted. This commit fixes it so we're only blocking the
actually illegal cases, while allowing the legal cases.

The new tests failed before this patch. Also confirmed that this fixes
the test failures we were seeing in Trio with Python dev builds:
python-trio/trio#1396

https://bugs.python.org/issue39606

Automerge-Triggered-By: @njsmith

The fix for bpo-39386 attempted to make it so you couldn't reuse a agen.aclose() coroutine object. It accidentally also prevented you from calling aclose() at all on an async generator that was already closed or exhausted. This commit fixes it so we're only blocking the actually illegal cases, while allowing the legal cases. The new tests failed before this patch. Also confirmed that this fixes the test failures we were seeing in Trio with Python dev builds: python-trio/trio#1396

njsmith · 2020-02-12T08:07:04Z

I think this needs to get in before 3.8.2 is released, which is currently scheduled for Monday Feb 17.

njsmith

Some notes to make review easier.

njsmith · 2020-02-12T08:20:27Z

Objects/genobject.c

+        PyErr_SetNone(PyExc_StopIteration);
+        return NULL;
+    }
+


These two hunks are the main change. Originally, this code raised StopIteration if the generator had no frame. The fix for bpo-39386 added the check for the athrow coroutine being in AWAITABLE_STATE_CLOSED to the same if statement, and switched it to raise RuntimeError in both cases. But in fact, the generator having no frame should raise StopIteration; it's only the AWAITABLE_STATE_CLOSED case that should raise RuntimeError. So I split the AWAITABLE_STATE_CLOSED logic out into its own block, and restored the old if (f == NULL || f->f_stacktop) logic.

Everything else is just going through to make sure that we're setting AWAITABLE_STATE_CLOSED at the right times.

njsmith · 2020-02-12T08:20:27Z

Objects/genobject.c

    if (o->agt_state == AWAITABLE_STATE_INIT) {
        if (o->agt_gen->ag_running_async) {
+            o->agt_state = AWAITABLE_STATE_CLOSED;


If the athrow/aclose is being aborted because there's already another task advancing the underlying async generator, then that's a final result, so we should mark the coroutine as complete.

njsmith · 2020-02-12T08:20:27Z

Objects/genobject.c

@@ -1878,7 +1884,6 @@ async_gen_athrow_send(PyAsyncGenAThrow *o, PyObject *arg)
        /* aclose() mode */
        if (retval) {
            if (_PyAsyncGenWrappedValue_CheckExact(retval)) {
-                o->agt_gen->ag_running_async = 0;
                Py_DECREF(retval);
                goto yield_close;


yield_close already sets ag_running_async = 0, so the deleted line was already a no-op.

njsmith · 2020-02-12T08:20:27Z

Objects/genobject.c

@@ -1893,16 +1898,17 @@ async_gen_athrow_send(PyAsyncGenAThrow *o, PyObject *arg)

 yield_close:
    o->agt_gen->ag_running_async = 0;
+    o->agt_state = AWAITABLE_STATE_CLOSED;


ag_running_async tracks whether there's some call to asend/athrow in progress. agt_state tracks whether this call to asend/athrow is in progress. So every time we set ag_running_async = 0, we should also set agt_state = AWAITABLE_STATE_CLOSED.

codecov · 2020-02-12T08:34:43Z

Codecov Report

Merging #18475 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #18475     +/-   ##
=========================================
  Coverage   82.11%   82.12%             
=========================================
  Files        1956     1955      -1     
  Lines      589067   583746   -5321     
  Branches    44436    44437      +1     
=========================================
- Hits       483723   479380   -4343     
+ Misses      95690    94721    -969     
+ Partials     9654     9645      -9

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
Lib/test/test_asyncio/test_base_events.py	`91.84% <0.00%> (-3.30%)`	⬇️
... and 328 more

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 95905ce...38a68f2. Read the comment docs.

asvetlov

LGTM!
Thanks for catching up!

1st1

LGTM. Thanks for your comments, Nathaniel. They really helped to follow the changes (as I'm not a bit rusty on this implementaion). Would be great if you could add them directly to the code.

njsmith · 2020-02-13T08:15:18Z

Would be great if you could add them directly to the code.

Yeah, I thought about that when writing them, but I couldn't figure out how to write them in a way that would make sense when looking at a single version of the file, instead of a diff. The full state machine management here is complicated and spread about over a bunch of different places, and there are lots of bits of code that are the same as the ones I modified here and the comments would apply the same way, just I didn't touch them because they were already correct...

Since there's some urgency here and everyone agrees the code looks good, I'm going to go ahead and merge. If you have suggestions on how to make the comments work better we can always add those later :-)

miss-islington · 2020-02-13T08:15:42Z

Thanks @njsmith for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2020-02-13T08:15:42Z

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

miss-islington · 2020-02-13T08:15:45Z

Sorry @njsmith, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 925dc7fb1d0db85dc137afa4cd14211bf0d67414 3.7

miss-islington · 2020-02-13T08:19:42Z

Thanks @njsmith for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2020-02-13T08:19:42Z

Thanks @njsmith for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2020-02-13T08:19:51Z

GH-18501 is a backport of this pull request to the 3.8 branch.

miss-islington · 2020-02-13T08:19:51Z

Sorry, @njsmith, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 925dc7fb1d0db85dc137afa4cd14211bf0d67414 3.7

bedevere-bot · 2020-02-13T08:42:57Z

GH-18502 is a backport of this pull request to the 3.7 branch.

@njsmith

…ed (GH-18475) (GH-18501) The fix for [bpo-39386](https://bugs.python.org/issue39386) attempted to make it so you couldn't reuse a agen.aclose() coroutine object. It accidentally also prevented you from calling aclose() at all on an async generator that was already closed or exhausted. This commit fixes it so we're only blocking the actually illegal cases, while allowing the legal cases. The new tests failed before this patch. Also confirmed that this fixes the test failures we were seeing in Trio with Python dev builds: python-trio/trio#1396 https://bugs.python.org/issue39606 (cherry picked from commit 925dc7f) Co-authored-by: Nathaniel J. Smith <njs@pobox.com> https://bugs.python.org/issue39606 Automerge-Triggered-By: @njsmith

…-18475) (GH-18502) The fix for [bpo-39386](https://bugs.python.org/issue39386) attempted to make it so you couldn't reuse a agen.aclose() coroutine object. It accidentally also prevented you from calling aclose() at all on an async generator that was already closed or exhausted. This commit fixes it so we're only blocking the actually illegal cases, while allowing the legal cases. The new tests failed before this patch. Also confirmed that this fixes the test failures we were seeing in Trio with Python dev builds: python-trio/trio#1396 https://bugs.python.org/issue39606 (cherry picked from commit 925dc7f)

njsmith requested a review from 1st1 as a code owner Feb 12, 2020

the-knights-who-say-ni added the CLA signed label Feb 12, 2020

bedevere-bot added the awaiting core review label Feb 12, 2020

njsmith requested review from asvetlov and ambv Feb 12, 2020

njsmith mentioned this pull request Feb 12, 2020

Drop python3.5 python-trio/trio#1396

Closed

njsmith commented Feb 12, 2020

View changes

asvetlov approved these changes Feb 12, 2020

View changes

bedevere-bot added awaiting merge and removed awaiting core review labels Feb 12, 2020

njsmith added needs backport to 3.7 needs backport to 3.8 type-bug An unexpected behavior, bug, or error labels Feb 12, 2020

njsmith mentioned this pull request Feb 12, 2020

Fix python 3.8 compatibility vxgmichel/aiostream#59

Merged

1st1 approved these changes Feb 12, 2020

View changes

njsmith added the 🤖 automerge PR will be merged once it's been approved and all CI passed label Feb 13, 2020

miss-islington merged commit 925dc7f into python:master Feb 13, 2020

bedevere-bot removed the awaiting merge label Feb 13, 2020

njsmith deleted the bpo-39606 branch Feb 13, 2020

miss-islington self-assigned this Feb 13, 2020

njsmith added needs backport to 3.7 and removed needs backport to 3.7 needs backport to 3.8 labels Feb 13, 2020

njsmith added the needs backport to 3.8 label Feb 13, 2020

bedevere-bot removed the needs backport to 3.8 label Feb 13, 2020

bedevere-bot removed the needs backport to 3.7 label Feb 13, 2020

bpo-39606: allow closing async generators that are already closed #18475

bpo-39606: allow closing async generators that are already closed #18475

njsmith commented Feb 12, 2020 •

edited by miss-islington

njsmith commented Feb 12, 2020

njsmith left a comment

njsmith Feb 12, 2020 •

edited

njsmith Feb 12, 2020

njsmith Feb 12, 2020

njsmith Feb 12, 2020

codecov bot commented Feb 12, 2020 •

edited

asvetlov left a comment

1st1 left a comment

njsmith commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

miss-islington commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

bpo-39606: allow closing async generators that are already closed #18475

bpo-39606: allow closing async generators that are already closed #18475

Conversation

njsmith commented Feb 12, 2020 • edited by miss-islington

njsmith commented Feb 12, 2020

njsmith left a comment

njsmith Feb 12, 2020 • edited

Choose a reason for hiding this comment

njsmith Feb 12, 2020

Choose a reason for hiding this comment

njsmith Feb 12, 2020

Choose a reason for hiding this comment

njsmith Feb 12, 2020

Choose a reason for hiding this comment

codecov bot commented Feb 12, 2020 • edited

Codecov Report

asvetlov left a comment

1st1 left a comment

njsmith commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

miss-islington commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

njsmith commented Feb 12, 2020 •

edited by miss-islington

njsmith Feb 12, 2020 •

edited

codecov bot commented Feb 12, 2020 •

edited