classification
Title: Issues with "async for"
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: 17288 33026 Superseder:
Assigned To: serhiy.storchaka Nosy List: miss-islington, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2018-03-10 13:10 by serhiy.storchaka, last changed 2018-04-27 12:38 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 6052 merged serhiy.storchaka, 2018-03-10 13:39
PR 6053 merged serhiy.storchaka, 2018-03-10 13:40
PR 6060 merged serhiy.storchaka, 2018-03-10 17:34
PR 6062 merged miss-islington, 2018-03-10 18:46
PR 6110 merged serhiy.storchaka, 2018-03-13 20:13
PR 6140 merged serhiy.storchaka, 2018-03-18 09:23
PR 6141 merged serhiy.storchaka, 2018-03-18 09:29
PR 6142 merged serhiy.storchaka, 2018-03-18 12:14
PR 6154 merged serhiy.storchaka, 2018-03-20 06:39
PR 6199 merged serhiy.storchaka, 2018-03-23 12:48
PR 6595 merged vstinner, 2018-04-25 13:14
Messages (18)
msg313526 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-10 13:10
There is a number of issues with "async for".

1. When assigning to the target raises StopAsyncIteration (in custom __setitem__, __setattr__ or __iter__) it will be silenced and will cause to stop iteration.

2. StopAsyncIteration is dynamically looked up in globals. If set the global StopAsyncIteration or delete it from builtins (for example at the shutdown stage), this will break any "async for".

3. The f_lineno setter doesn't handle jumping into or out of the "async for" block. Jumping into is not forbidden, and jumping out doesn't update the stack correctly. This can cause a crash or incorrect behavior (like iterating wrong loop).

4. The compiler doesn't check all errors when creating new blocks. Some blocks are not used. And the resulting bytecode is suboptimal.

I'll create a series of pull request for fixing all this issue. Some of them can be backported. Others require changes in bytecode or are too hard for implementing in 3.7 and earlier versions (the related code was changed in issue17611). Some of them depend on other PRs or other issues (like issue33026) and need to wait until their be merged.
msg313527 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-10 13:46
PR 6052 fixes issue 1. I don't know what is the best place for tests. There are two files with tests for "async for": test_coroutines.py and test_asyncgen.py. I'm not sure that new tests use the simplest way for testing this behavior. Could you please look at them Yury?

PR 6053 fixes issue 4. Adds missed error checks, removes unused variables and removes generating redundant bytecode.
msg313538 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-10 15:56
Thanks so much for looking into this, Serhiy!

> 2. StopAsyncIteration is dynamically looked up in globals. If set the global StopAsyncIteration or delete it from builtins (for example at the shutdown stage), this will break any "async for".

IIRC I adapted the approach from some other place in compile.c. Quick looking at it reveals that the `assert` statement is compiled in a similar way w.r.t. how AssertionError is looked up at runtime.  You might want to check if there are other places in compile.c that need to be fixed.

> PR 6052 fixes issue 1. I don't know what is the best place for tests. There are two files with tests for "async for": test_coroutines.py and test_asyncgen.py. I'm not sure that new tests use the simplest way for testing this behavior. Could you please look at them Yury?

I think the new tests are fine.
msg313539 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-10 16:22
New changeset 24d3201eb7f0b39a7eaf2a5b2a2ceca10ad1f8eb by Serhiy Storchaka in branch 'master':
bpo-33041: Fixed bytecode generation for "async for" with a complex target. (#6052)
https://github.com/python/cpython/commit/24d3201eb7f0b39a7eaf2a5b2a2ceca10ad1f8eb
msg313543 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-10 16:49
New changeset 67ee07795bcd84b679c000780212d4d81a1490a3 by Serhiy Storchaka in branch 'master':
bpo-33041: Add missed error checks when compile "async for" (#6053)
https://github.com/python/cpython/commit/67ee07795bcd84b679c000780212d4d81a1490a3
msg313553 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-10 18:45
New changeset 9e94c0d3c78d1bc582c865240ed9353fe9689b2a by Serhiy Storchaka in branch '3.7':
[3.7] bpo-33041: Add missed error checks when compile "async for" (GH-6053) (GH-6060)
https://github.com/python/cpython/commit/9e94c0d3c78d1bc582c865240ed9353fe9689b2a
msg313555 - (view) Author: miss-islington (miss-islington) Date: 2018-03-10 19:32
New changeset d0826340d96e0953793b86d0b8475d2f43a280b6 by Miss Islington (bot) in branch '3.6':
[3.7] bpo-33041: Add missed error checks when compile "async for" (GH-6053) (GH-6060)
https://github.com/python/cpython/commit/d0826340d96e0953793b86d0b8475d2f43a280b6
msg313772 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-13 20:15
PR 6110 adds tests for jumping in/out of 'async with' blocks. It isn't directly related to 'async for', but it adds helpers that will be used also in tests for 'async for'.

I'm not sure this is a correct and good way of writing such tests.
msg314029 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 07:53
New changeset bc300ce205f99acb1ef92c37de06dc76147e073b by Serhiy Storchaka in branch 'master':
bpo-33041: Add tests for jumps in/out of 'async with' blocks. (#6110)
https://github.com/python/cpython/commit/bc300ce205f99acb1ef92c37de06dc76147e073b
msg314034 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 10:31
New changeset 773573e9ac654d4b5c6682e70360f75864acd80e by Serhiy Storchaka in branch '3.7':
[3.7] bpo-33041: Add tests for jumps in/out of 'async with' blocks. (GH-6110). (GH-6140)
https://github.com/python/cpython/commit/773573e9ac654d4b5c6682e70360f75864acd80e
msg314035 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 10:32
New changeset 193760f00aacb122698674ed15327dba412653a5 by Serhiy Storchaka in branch '3.6':
[3.6] bpo-33041: Add tests for jumps in/out of 'async with' blocks. (GH-6110). (GH-6141)
https://github.com/python/cpython/commit/193760f00aacb122698674ed15327dba412653a5
msg314038 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 12:29
PR 6142 fixes issues 2 and 4. It adds a new opcode END_ASYNC_FOR and therefore can't be backported. END_ASYNC_FOR combines a number of other opcodes and guaranties using the true StopAsyncIteration. The new opcode is neccessary also for detecting an "async for" loop when jump. Besides introducing the new opcode the code of an "async for" loop was changed in a way that allowed to disallow jumping into an "async for" loop (this is the only part that can be backported). The final bytecode is much simpler. The compiler has been cleaned up and its code is now much simpler too. I expect also a performance boost, but don't know how to benchmark this.

Perhaps only the half of issue 4 (disallowing jumps into an "async for" loop) can be solved in 3.7 and 3.6.
msg314134 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-20 06:46
I'm surprised, but seems PR 6154 fixes the whole issue 3 in 3.7.

It keeps only one POP_BLOCK corresponding to SETUP_LOOP. It also make the generated bytecode a tiny bit more efficient (less jumps).
msg314305 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-23 12:34
New changeset 702f8f3611bc49b73772cce2b9b041bd11ff9b35 by Serhiy Storchaka in branch 'master':
bpo-33041: Rework compiling an "async for" loop. (#6142)
https://github.com/python/cpython/commit/702f8f3611bc49b73772cce2b9b041bd11ff9b35
msg314306 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-23 12:35
New changeset b9744e924ca07ba7db977e5958b91cd8db565632 by Serhiy Storchaka in branch '3.7':
bpo-33041: Fixed jumping if the function contains an "async for" loop. (GH-6154)
https://github.com/python/cpython/commit/b9744e924ca07ba7db977e5958b91cd8db565632
msg314310 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-23 13:45
New changeset 18d7edf32e6832a818621cb8cb3d144928eca232 by Serhiy Storchaka in branch '3.6':
[3.6] bpo-33041: Fixed jumping if the function contains an "async for" loop. (GH-6154). (GH-6199)
https://github.com/python/cpython/commit/18d7edf32e6832a818621cb8cb3d144928eca232
msg315837 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-04-27 12:30
New changeset 078c4e3519deeef8014541925da057bb064eb5a8 by Victor Stinner in branch 'master':
bpo-33041: Fix downcast warning on Windows (#6595)
https://github.com/python/cpython/commit/078c4e3519deeef8014541925da057bb064eb5a8
msg315838 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-04-27 12:38
I didn't check if the new warning, fixed by my PR-6595 in the master branch, exists on Windows in 3.6 and 3.7. If it does, you might want to request a backport of my PR. Usually, I only fix compiler warnings in the master branch.
History
Date User Action Args
2018-04-27 12:38:23vstinnersetmessages: + msg315838
2018-04-27 12:30:04vstinnersetnosy: + vstinner
messages: + msg315837
2018-04-25 13:14:50vstinnersetpull_requests: + pull_request6293
2018-03-23 13:47:54serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-03-23 13:45:40serhiy.storchakasetmessages: + msg314310
2018-03-23 12:48:12serhiy.storchakasetpull_requests: + pull_request5946
2018-03-23 12:35:35serhiy.storchakasetmessages: + msg314306
2018-03-23 12:34:43serhiy.storchakasetmessages: + msg314305
2018-03-20 06:46:02serhiy.storchakasetmessages: + msg314134
2018-03-20 06:39:47serhiy.storchakasetpull_requests: + pull_request5912
2018-03-18 12:29:13serhiy.storchakasetmessages: + msg314038
2018-03-18 12:14:15serhiy.storchakasetpull_requests: + pull_request5899
2018-03-18 10:32:34serhiy.storchakasetmessages: + msg314035
2018-03-18 10:31:41serhiy.storchakasetmessages: + msg314034
2018-03-18 09:29:37serhiy.storchakasetpull_requests: + pull_request5898
2018-03-18 09:23:34serhiy.storchakasetpull_requests: + pull_request5897
2018-03-18 07:53:17serhiy.storchakasetmessages: + msg314029
2018-03-13 20:16:00serhiy.storchakasetmessages: + msg313772
2018-03-13 20:13:16serhiy.storchakasetpull_requests: + pull_request5872
2018-03-13 08:56:31serhiy.storchakasetdependencies: + cannot jump from a 'return' or 'exception' trace event, Fix jumping out of "with" block
2018-03-10 19:32:52miss-islingtonsetnosy: + miss-islington
messages: + msg313555
2018-03-10 18:46:16miss-islingtonsetpull_requests: + pull_request5824
2018-03-10 18:45:07serhiy.storchakasetmessages: + msg313553
2018-03-10 17:34:22serhiy.storchakasetpull_requests: + pull_request5822
2018-03-10 16:49:28serhiy.storchakasetmessages: + msg313543
2018-03-10 16:22:36serhiy.storchakasetmessages: + msg313539
2018-03-10 15:56:11yselivanovsetmessages: + msg313538
2018-03-10 13:46:31serhiy.storchakasetmessages: + msg313527
2018-03-10 13:40:38serhiy.storchakasetpull_requests: + pull_request5814
2018-03-10 13:39:51serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request5813
2018-03-10 13:10:41serhiy.storchakacreate