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

AsyncIterator on 3.7: __aiter__ no longer honors finally blocks #80584

Closed
asksol mannequin opened this issue Mar 23, 2019 · 8 comments
Closed

AsyncIterator on 3.7: __aiter__ no longer honors finally blocks #80584

asksol mannequin opened this issue Mar 23, 2019 · 8 comments
Labels
pending The issue will be closed if no feedback is provided topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@asksol
Copy link
Mannequin

asksol mannequin commented Mar 23, 2019

BPO 36403
Nosy @ned-deily, @asvetlov, @1st1, @twisteroidambassador, @tirkarthi
Files
  • test_iterators_and_finally_blocks.py
  • 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 = None
    closed_at = None
    created_at = <Date 2019-03-23.00:15:07.175>
    labels = ['3.8', 'type-bug', '3.7', 'expert-asyncio']
    title = 'AsyncIterator on 3.7: __aiter__ no longer honors finally blocks'
    updated_at = <Date 2020-06-12.08:32:55.224>
    user = 'https://bugs.python.org/asksol'

    bugs.python.org fields:

    activity = <Date 2020-06-12.08:32:55.224>
    actor = 'ned.deily'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['asyncio']
    creation = <Date 2019-03-23.00:15:07.175>
    creator = 'asksol'
    dependencies = []
    files = ['48229']
    hgrepos = []
    issue_num = 36403
    keywords = ['3.7regression']
    message_count = 7.0
    messages = ['338630', '338638', '338642', '338647', '345805', '345808', '371341']
    nosy_count = 6.0
    nosy_names = ['ned.deily', 'asvetlov', 'asksol', 'yselivanov', 'twisteroid ambassador', 'xtreak']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue36403'
    versions = ['Python 3.7', 'Python 3.8']

    @asksol
    Copy link
    Mannequin Author

    asksol mannequin commented Mar 23, 2019

    We use finally blocks in __aiter__ like this:

    class AsyncFinallyIterator:
    
       def __aiter__(self):
           for i in range(10):
               try:
                   yield i
               finally:
                   print('FINALLY')
    

    Attached is a test for both iterators and async iterators.
    The tests pass on Python 3.6, but only the non-async iterator test pass under Python 3.7.

    Thanks for your attention!

    This worked perfectly well in Python 3.6, but stopped working in Python 3.7.

    I also verified that Iterator supports the same construct (and this works in both Python 3.6 and 3.7):

    
    class FinallyIterator:
    
        def __iter__(self):
            for i in range(10):
                try:
                    yield i
                finally:
                    print('FINALLY')
    

    @asksol asksol mannequin added the 3.7 (EOL) end of life label Mar 23, 2019
    @tirkarthi
    Copy link
    Member

    The attached script fails on master too. On bisecting could this be possibly caused due to 41e5ec3 (bpo-34769) ? Tagging Ned since it was introduced from 3.7.1rc2 and also backported to 3.6 .

    ➜ cpython git:(41e5ec3) git checkout 41e5ec3 && make -s -j4 > /dev/null
    HEAD is now at 41e5ec3 bpo-34769: Thread safety for _asyncgen_finalizer_hook(). (GH-9716)
    ➜ cpython git:(41e5ec3) ./python.exe ../backups/bpo36403.py
    F.
    ======================================================================
    FAIL: test_main (main.TestAsyncIteratorFinally)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "../backups/bpo36403.py", line 30, in test_main
        self.assertTrue(it.finally_executed)
    AssertionError: False is not true

    Ran 2 tests in 0.002s

    FAILED (failures=1)
    ➜ cpython git:(41e5ec3) git checkout 41e5ec3~1 && make -s -j4 > /dev/null
    Previous HEAD position was 41e5ec3 bpo-34769: Thread safety for _asyncgen_finalizer_hook(). (GH-9716)
    HEAD is now at 0ce31d3 bpo-32962: Fix test_gdb failure in debug build with -mcet -fcf-protection -O0 (GH-9656)
    ➜ cpython git:(0ce31d3) ./python.exe ../backups/bpo36403.py
    ..
    ----------------------------------------------------------------------
    Ran 2 tests in 0.003s

    OK

    @tirkarthi tirkarthi added topic-asyncio 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels Mar 23, 2019
    @asksol
    Copy link
    Mannequin Author

    asksol mannequin commented Mar 23, 2019

    Ah, so the extra call_soon means it needs a:

    [code]
    loop.run_until_complete(asyncio.sleep(0))```
    [/code]

    before the self.assertTrue(it.finally_executed)

    to finish executing agen.close().

    Why is create_task different? Does it execute an iteration of the generator immediately?

    Seems good for this behavior to be consistent, but not sure how difficult that would be.

    @asksol
    Copy link
    Mannequin Author

    asksol mannequin commented Mar 23, 2019

    Perhaps we could add a self._finally to the event loop itself?
    Like loop._ready, but a list of callbacks run_until_complete will call before returning?

    @ned-deily
    Copy link
    Member

    Ping. This was marked as a 3.7regression. Is a change needed?

    @asvetlov
    Copy link
    Contributor

    From my understanding yield inside __aiter__ is a forbidden construction. Even if parser allows it the statement doesn't make any sense.

    I'm very interested to hear Yuri opinion.

    @ned-deily
    Copy link
    Member

    I note this is marked as a 3.7regression and still open. Since the cutoff for the final 3.7 bugfix mode release is in a few days, I'm assuming this means that 3.7 users will have to live with this regression. If you feel that is a problem, speak up now.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @kumaraditya303 kumaraditya303 removed 3.8 only security fixes 3.7 (EOL) end of life labels Sep 22, 2022
    @kumaraditya303
    Copy link
    Contributor

    I don't see how this is a bug or regression. There is no guarantee when generators are finalized, code relying on it is already subtly broken. I'll close this unless anyone objects, marking pending.

    @kumaraditya303 kumaraditya303 added the pending The issue will be closed if no feedback is provided label Sep 22, 2022
    @kumaraditya303 kumaraditya303 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 23, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    pending The issue will be closed if no feedback is provided topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    Status: Done
    Development

    No branches or pull requests

    4 participants