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

nested try..excepts don't work correctly for generators #69798

Closed
1st1 opened this issue Nov 12, 2015 · 38 comments
Closed

nested try..excepts don't work correctly for generators #69798

1st1 opened this issue Nov 12, 2015 · 38 comments
Labels
3.7 interpreter-core

Comments

@1st1
Copy link
Member

@1st1 1st1 commented Nov 12, 2015

BPO 25612
Nosy @brettcannon, @ncoghlan, @pitrou, @scoder, @vstinner, @larryhastings, @benjaminp, @ned-deily, @njsmith, @asvetlov, @cjerdonek, @markshannon, @vadmium, @serhiy-storchaka, @1st1, @ajdavis
PRs
  • #1773
  • #7069
  • #7074
  • #7656
  • #7658
  • Files
  • gen_exc_1.patch
  • 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 = <Date 2017-10-30.11:37:04.064>
    created_at = <Date 2015-11-12.20:59:18.932>
    labels = ['interpreter-core', '3.7']
    title = "nested try..excepts don't work correctly for generators"
    updated_at = <Date 2018-06-29.12:41:13.832>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2018-06-29.12:41:13.832>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-30.11:37:04.064>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2015-11-12.20:59:18.932>
    creator = 'yselivanov'
    dependencies = []
    files = ['41026']
    hgrepos = []
    issue_num = 25612
    keywords = ['patch']
    message_count = 38.0
    messages = ['254557', '254558', '254561', '254564', '254576', '254578', '254809', '254812', '254815', '254821', '254822', '254854', '254858', '254870', '254990', '254991', '254995', '254996', '255039', '255079', '256622', '287995', '287996', '304117', '304760', '304770', '304771', '305150', '305151', '305220', '305223', '317417', '317442', '319358', '319360', '319361', '319366', '320715']
    nosy_count = 16.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'pitrou', 'scoder', 'vstinner', 'larry', 'benjamin.peterson', 'ned.deily', 'njs', 'asvetlov', 'chris.jerdonek', 'Mark.Shannon', 'martin.panter', 'serhiy.storchaka', 'yselivanov', 'emptysquare']
    pr_nums = ['1773', '7069', '7074', '7656', '7658']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue25612'
    versions = ['Python 3.7']

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 12, 2015

    Nested try..except statements with yields can loose reference to the current exception.

    The following code:

        class MainError(Exception):
            pass
    
        class SubError(Exception):
            pass
    
        def main():
            try:
                raise MainError()
            except MainError:
                try:
                    yield
                except SubError:
                    print('got SubError')
                raise
    
        coro = main()
        coro.send(None)
        coro.throw(SubError())

    prints:

        got SubError
        Traceback (most recent call last):
          File "t.py", line 19, in <module>
            coro.throw(SubError())
          File "t.py", line 15, in main
            raise
        RuntimeError: No active exception to reraise

    @1st1 1st1 added release-blocker interpreter-core labels Nov 12, 2015
    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 12, 2015

    This was originally discovered here: python/asyncio#287

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 12, 2015

    Is this issue related to the issue bpo-23353?

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 12, 2015

    I reverted the change of the issue bpo-23353 but it doesn't fix this example, so it looks like these issues are not related. Cool.

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 12, 2015

    Another bug:

        class MainError(Exception):
            pass
    
        class SubError(Exception):
            pass
    
        def main():
            try:
                raise MainError()
            except MainError:
                yield
    
        coro = main()
        coro.send(None)
        coro.throw(SubError())

    SubError will propagate, but won't have MainError in its __context__

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 13, 2015

    Here's a patch the fixes the first problem (but __context__ bug is still open).

    I'm not sure that the patch is correct :( But at least I've added new unittests (one still failing)

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 17, 2015

    Can someone work with me on fixing this issue? I think it's important to ship 3.5.1 with this resolved (and 3.4.4 FWIW).

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 17, 2015

    You might have to ping python-dev. But in terms of priorities I think
    it's not a blocker -- it's been broken for quite some time now, and
    it's a fairly odd corner of the language.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Nov 17, 2015

    Regarding the second bug, did you consider that the exception thrown to the generator can already have __context__ set?

    try:
    try: raise ValueError("Context outside of generator")
    except ValueError as ex: raise SubError() from ex
    except SubError as ex:
    coro.throw(ex) # ex.__context__ is a ValueError

    I guess either one context could trump the other, or we could to follow the chain of contexts and append the other chain at the end. Both these ideas seem a bit ugly though.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 17, 2015

    Can someone work with me on fixing this issue? I think it's important to ship 3.5.1 with this resolved (and 3.4.4 FWIW).

    It don't consider this issue as a blocker for Python 3.5.1. This
    release already contains a *lot* of bugfixes! It's important to get it
    as soon as possible.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Nov 17, 2015

    Thinking about the __context__ thing some more, I guess it might make sense for __context__ to be overwritten by the generator context. The same way it gets overwritten when you execute “raise” inside an exception handler.

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 18, 2015

    Thinking about the __context__ thing some more, I guess it might make sense for __context__ to be overwritten by the generator context. The same way it gets overwritten when you execute “raise” inside an exception handler.

    Not sure I understand what you're saying here.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Nov 18, 2015

    I was making an analogy between how the “raise” statement works, and how the throw() method could work. In this example, there are three exception objects (MainError, SubError, and ValueError). I was suggesting that it is okay for the context to be set to the MainError instance, because that is how the analogous version using a “raise” statement works.

    def main():
        try:
            raise MainError("Context inside generator")
        except MainError:
            yield  # __context__ could be changed to MainError
    
    coro = main()
    coro.send(None)
    try:
        try: raise ValueError("Context outside of generator")
        except ValueError: raise SubError()
    except SubError as ex:
        coro.throw(ex)  # __context__ is ValueError

    # raise analogy:

    try:
    try: raise ValueError("Context outside of generator")
    except ValueError as ex: raise SubError()
    except SubError as ex:
    saved = ex # __context__ is ValueError

    try:
    raise MainError("Context inside generator")
    except MainError:
    raise saved # Changes __context__ to MainError

    ===

    Sorry I’m not really familiar with the code to quickly propose a patch or review your change though.

    @larryhastings
    Copy link
    Contributor

    @larryhastings larryhastings commented Nov 19, 2015

    I don't plan to hold up 3.5.1 for this.

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 20, 2015

    Martin, you might be right here. Guido, do you think it's OK that SubError doesn't have MainError in its __context__? http://bugs.python.org/issue25612#msg254576

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 20, 2015

    I'm sorry, I'm no help here -- I don't know how __context__ is
    supposed to work. :-(

    On Fri, Nov 20, 2015 at 8:14 AM, Yury Selivanov <report@bugs.python.org> wrote:

    Yury Selivanov added the comment:

    Martin, you might be right here. Guido, do you think it's OK that SubError doesn't have MainError in its __context__? http://bugs.python.org/issue25612#msg254576

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue25612\>


    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 20, 2015

    Anyways, here's a separate issue for the __context__ problem: http://bugs.python.org/issue25683

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Nov 20, 2015

    Guido, Martin, I've just posted to python-dev about this ticket. Let's see what others think.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Nov 21, 2015

    Interrogating the thread state like that makes me wonder how this patch behaves in the following scenario:

        class MainError(Exception): pass
        class SubError(Exception): pass
    
        def yield_coro():
            yield
        coro = yield_coro()
        coro.send(None)
    try:
        raise MainError
    except MainError:
        try:
            coro.throw(SubError)
        except SubError:
            pass
        raise
    

    Also potentially worth exploring is this proposed architectural change from Mark Shannon to move all exception related state out of frame objects: http://bugs.python.org/issue13897

    While we couldn't do the latter in a maintenance release, I'd be interested to know what effect it has on this edge case.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Nov 22, 2015

    Nick, your scenario seems to behave no differently with and without the patch:

    Traceback (most recent call last):
      File "<stdin>", line 2, in <module>
    __main__.MainError

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Dec 17, 2015

    I closed issue bpo-25683 as "not a bug". So it's just this issue that we need to fix.

    Anyone wants to review the patch? :) Since the code involved here is quite complex, I don't want to commit this patch without a thorough review.

    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Feb 17, 2017

    I read the patch but I don't understand the logic behind it :-). Why should the value of tstate->exc_type affect whether we save/restore exc_info? Won't this mean that things are still broken for code like:

    -----

    def f():
        try:
            raise KeyError
        except Exception:
            try:
                yield
            except Exception:
                pass
            raise

    try:
    raise NameError
    except Exception:
    gen = f()
    gen.send(None)
    gen.throw(ValueError)
    -----

    ?

    Conceptually I would have thought the fix would be to remove the '!throwflag' check, but I haven't actually tried it...

    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Feb 17, 2017

    (bpo-29587 is a duplicate of this one, but it has some more information on where the !throwflag check came from and why it might be possible to remove it now.)

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 11, 2017

    I tried the following code:

    def g():
        yield 1
        raise
        yield 2
    
    i = g()
    try:
        1/0
    except:
        next(i)
        next(i)

    Currently it raises:

    Traceback (most recent call last):
      File "<stdin>", line 5, in <module>
      File "<stdin>", line 2, in <module>
    ZeroDivisionError: division by zero

    With PR 1773 applied it raises:

    Traceback (most recent call last):
      File "<stdin>", line 2, in <module>
    ZeroDivisionError: division by zero
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<stdin>", line 5, in <module>
      File "<stdin>", line 3, in g
    RuntimeError: No active exception to reraise

    And this looks more correct.

    But if replace raise with print(sys.exc_info()) the patched and unpatched interpreters both print:

    (<class 'ZeroDivisionError'>, ZeroDivisionError('division by zero',), <traceback object at 0x7f61d9ed1448>)

    Is it correct that sys.exc_info() return an exception while raise can't reraise it?

    @markshannon
    Copy link
    Member

    @markshannon markshannon commented Oct 22, 2017

    Thanks Serhiy for spotting that.

    'raise' should raise the same exception as sys.exc_info() returns.
    I'll update the PR.

    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Oct 22, 2017

    New changeset ae3087c by Antoine Pitrou (Mark Shannon) in branch 'master':
    Move exc state to generator. Fixes bpo-25612 (bpo-1773)
    ae3087c

    @pitrou pitrou closed this as completed Oct 22, 2017
    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Oct 22, 2017

    Thank you Mark for the fix!

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 28, 2017

    Note that removing exc_type, exc_value and exc_traceback from PyThreadState breaks Cython.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 28, 2017

    The problem I mentioned in msg304117 has been resolved in backward direction: "raise" outside of an except block don't raise a RuntimeError.

    @markshannon
    Copy link
    Member

    @markshannon markshannon commented Oct 30, 2017

    Looking at the docs:

    https://docs.python.org/3.6/library/sys.html#sys.exc_info states:
    If the current stack frame is not handling an exception, the information is taken from the calling stack frame, or its caller, and so on until a stack frame is found that is handling an exception.

    And https://docs.python.org/3/reference/simple_stmts.html#the-raise-statement

    If no expressions are present, raise re-raises the last exception that was active in the current scope. If no exception is active in the current scope, a RuntimeError exception is raised indicating that this is an error.

    Note that sys.exc_info() explicitly mentions scanning the stack, but raise just says "active in the current scope". Testing on 3.5 shows that "active in the current scope" does scan the stack (for simple calls at least).

    Which means that the newly implemented behaviour is correct.

    Note that removing exc_type, exc_value and exc_traceback from PyThreadState breaks Cython.

    Is there a matching Cython issue?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 30, 2017

    Thank you for the clarification Mark. I agree that the current behavior is well justified.

    Cython was fixed in cython/cython@2d33924.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 23, 2018

    New changeset b6dccf5 by Victor Stinner in branch 'master':
    bpo-33612: Remove PyThreadState_Clear() assertion (bpo-7069)
    b6dccf5

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented May 23, 2018

    New changeset 508d769 by Ned Deily (Miss Islington (bot)) in branch '3.7':
    bpo-33612: Remove PyThreadState_Clear() assertion (GH-7069) (GH-7074)
    508d769

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jun 12, 2018

    New changeset 04290cb by Ned Deily in branch 'master':
    bpo-25612: Add minimal What's New in 3.7 entry (GH-7656)
    04290cb

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jun 12, 2018

    New changeset ef5f4ba by Ned Deily (Miss Islington (bot)) in branch '3.7':
    bpo-25612: Add minimal What's New in 3.7 entry (GH-7656) (GH-7658)
    ef5f4ba

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jun 12, 2018

    P.S. I added a very minimal What's New in 3.7 entry for this change since it has affected some downline projects. I just copied the NEWS entry. Feel free to expand it and/or move it to a better location in the file.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jun 12, 2018

    This change was in top5 breaking changes for 3.7. It broke many projects that use Cython until they upgraded Cython to the version that supports 3.7.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 29, 2018

    It seems like this issue introduced a regression in Python 3.7.0: bpo-33996 "Crash in gen_send_ex(): _PyErr_GetTopmostException() returns freed memory".

    @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
    Labels
    3.7 interpreter-core
    Projects
    None yet
    Development

    No branches or pull requests