Skip to content
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

Jump to bottom

Issues with "async for" #77222

Closed
serhiy-storchaka opened this issue Mar 10, 2018 · 18 comments
Closed

Issues with "async for" #77222

serhiy-storchaka opened this issue Mar 10, 2018 · 18 comments
Assignees
@serhiy-storchaka
Labels
3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@serhiy-storchaka
Copy link
Member

BPO 33041
Nosy @vstinner, @serhiy-storchaka, @1st1, @miss-islington
PRs
  • bpo-33041: Fixed bytecode generation for "async for" with a complex target. #6052
  • bpo-33041: Add missed error checks when compile "async for" #6053
  • [3.7] bpo-33041: Add missed error checks when compile "async for" (GH-6053) #6060
  • [3.6] [3.7] bpo-33041: Add missed error checks when compile "async for" (GH-6053) (GH-6060) #6062
  • bpo-33041: Add tests for jumps in/out of 'async with' blocks. #6110
  • [3.7] bpo-33041: Add tests for jumps in/out of 'async with' blocks. (GH-6110). #6140
  • [3.6] bpo-33041: Add tests for jumps in/out of 'async with' blocks. (GH-6110). #6141
  • bpo-33041: Rework compiling an "async for" loop. #6142
  • [3.7] bpo-33041: Fixed jumping if the function contains an "async for" loop. #6154
  • [3.6] bpo-33041: Fixed jumping if the function contains an "async for" loop. (GH-6154). #6199
  • bpo-33041: Fix downcast warning on Windows #6595
    		•
    Dependencies
  • bpo-17288: cannot jump from a 'return' or 'exception' trace event
  • bpo-33026: Fix jumping out of "with" block
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2018-03-23.13:47:54.971>
created_at = <Date 2018-03-10.13:10:41.780>
labels = ['interpreter-core', '3.7', '3.8', 'type-crash']
title = 'Issues with "async for"'
updated_at = <Date 2018-04-27.12:38:23.735>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2018-04-27.12:38:23.735>
actor = 'vstinner'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2018-03-23.13:47:54.971>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2018-03-10.13:10:41.780>
creator = 'serhiy.storchaka'
dependencies = ['17288', '33026']
files = []
hgrepos = []
issue_num = 33041
keywords = ['patch']
message_count = 18.0
messages = ['313526', '313527', '313538', '313539', '313543', '313553', '313555', '313772', '314029', '314034', '314035', '314038', '314134', '314305', '314306', '314310', '315837', '315838']
nosy_count = 4.0
nosy_names = ['vstinner', 'serhiy.storchaka', 'yselivanov', 'miss-islington']
pr_nums = ['6052', '6053', '6060', '6062', '6110', '6140', '6141', '6142', '6154', '6199', '6595']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue33041'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']
    @serhiy-storchaka
    Copy link
    Member Author

    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 bpo-17611). Some of them depend on other PRs or other issues (like bpo-33026) and need to wait until their be merged.

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes labels Mar 10, 2018
    @serhiy-storchaka serhiy-storchaka self-assigned this Mar 10, 2018
    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 10, 2018
    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @1st1
    Copy link
    Member

    1st1 commented Mar 10, 2018

    Thanks so much for looking into this, Serhiy!

    1. 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.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 24d3201 by Serhiy Storchaka in branch 'master':
    bpo-33041: Fixed bytecode generation for "async for" with a complex target. (bpo-6052)
    24d3201

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 67ee077 by Serhiy Storchaka in branch 'master':
    bpo-33041: Add missed error checks when compile "async for" (bpo-6053)
    67ee077

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 9e94c0d by Serhiy Storchaka in branch '3.7':
    [3.7] bpo-33041: Add missed error checks when compile "async for" (GH-6053) (GH-6060)
    9e94c0d

    @miss-islington
    Copy link
    Contributor

    New changeset d082634 by Miss Islington (bot) in branch '3.6':
    [3.7] bpo-33041: Add missed error checks when compile "async for" (GH-6053) (GH-6060)
    d082634

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset bc300ce by Serhiy Storchaka in branch 'master':
    bpo-33041: Add tests for jumps in/out of 'async with' blocks. (bpo-6110)
    bc300ce

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 773573e 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)
    773573e

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 193760f 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)
    193760f

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @serhiy-storchaka
    Copy link
    Member Author

    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).

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 702f8f3 by Serhiy Storchaka in branch 'master':
    bpo-33041: Rework compiling an "async for" loop. (bpo-6142)
    702f8f3

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset b9744e9 by Serhiy Storchaka in branch '3.7':
    bpo-33041: Fixed jumping if the function contains an "async for" loop. (GH-6154)
    b9744e9

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 18d7edf 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)
    18d7edf

    @vstinner
    Copy link
    Member

    New changeset 078c4e3 by Victor Stinner in branch 'master':
    bpo-33041: Fix downcast warning on Windows (bpo-6595)
    078c4e3

    @vstinner
    Copy link
    Member

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @1st1 @serhiy-storchaka @miss-islington