classification
Title: nested try..excepts don't work correctly for generators
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, asvetlov, benjamin.peterson, brett.cannon, emptysquare, larry, martin.panter, ncoghlan, njs, pitrou, scoder, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2015-11-12 20:59 by yselivanov, last changed 2017-10-30 11:37 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
gen_exc_1.patch yselivanov, 2015-11-13 00:16 review
Pull Requests
URL Status Linked Edit
PR 1773 merged python-dev, 2017-05-23 22:08
Messages (31)
msg254557 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-12 20:59
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
msg254558 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-12 21:13
This was originally discovered here: https://github.com/python/asyncio/issues/287
msg254561 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-11-12 22:14
Is this issue related to the issue #23353?
msg254564 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-11-12 22:18
I reverted the change of the issue #23353 but it doesn't fix this example, so it looks like these issues are not related. Cool.
msg254576 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-12 23:42
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__
msg254578 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-13 00:16
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)
msg254809 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-17 17:59
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).
msg254812 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-11-17 18:52
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.
msg254815 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-17 21:36
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.
msg254821 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-11-17 23:27
> 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.
msg254822 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-17 23:36
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.
msg254854 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-18 19:31
> 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.
msg254858 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-18 22:22
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.
msg254870 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-11-19 01:18
I don't plan to hold up 3.5.1 for this.
msg254990 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-20 16:14
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
msg254991 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-11-20 16:16
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>
> _______________________________________
msg254995 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-20 16:30
Anyways, here's a separate issue for the __context__ problem: http://bugs.python.org/issue25683
msg254996 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-11-20 16:31
Guido, Martin, I've just posted to python-dev about this ticket.  Let's see what others think.
msg255039 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-11-21 03:28
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.
msg255079 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-22 00:56
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
msg256622 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-17 23:09
I closed issue #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.
msg287995 - (view) Author: Nathaniel Smith (njs) * Date: 2017-02-17 12:01
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...
msg287996 - (view) Author: Nathaniel Smith (njs) * Date: 2017-02-17 12:05
(Issue 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.)
msg304117 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-11 08:31
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?
msg304760 - (view) Author: Mark Shannon (Mark.Shannon) * Date: 2017-10-22 19:32
Thanks Serhiy for spotting that.

'raise' should raise the same exception as sys.exc_info() returns.
I'll update the PR.
msg304770 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-10-22 21:41
New changeset ae3087c6382011c47db82fea4d05f8bbf514265d by Antoine Pitrou (Mark Shannon) in branch 'master':
Move exc state to generator. Fixes bpo-25612 (#1773)
https://github.com/python/cpython/commit/ae3087c6382011c47db82fea4d05f8bbf514265d
msg304771 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-10-22 21:42
Thank you Mark for the fix!
msg305150 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-28 08:16
Note that removing exc_type, exc_value and exc_traceback from PyThreadState breaks Cython.
msg305151 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-28 08:22
The problem I mentioned in msg304117 has been resolved in backward direction: "raise" outside of an except block don't raise a RuntimeError.
msg305220 - (view) Author: Mark Shannon (Mark.Shannon) * Date: 2017-10-30 10:59
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?
msg305223 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-30 11:37
Thank you for the clarification Mark. I agree that the current behavior is well justified.

Cython was fixed in https://github.com/cython/cython/commit/2d3392463b77eb550bd54a69bd28cc161947acb5.
History
Date User Action Args
2017-10-30 11:37:04serhiy.storchakasetstatus: open -> closed

messages: + msg305223
2017-10-30 10:59:56Mark.Shannonsetmessages: + msg305220
2017-10-28 08:22:18serhiy.storchakasetstatus: closed -> open

messages: + msg305151
2017-10-28 08:16:59serhiy.storchakasetnosy: + scoder
messages: + msg305150
2017-10-22 21:42:57pitrousetmessages: + msg304771
versions: - Python 2.7, Python 3.6
2017-10-22 21:41:53pitrousetstatus: open -> closed
resolution: fixed
messages: + msg304770

stage: resolved
2017-10-22 19:32:33Mark.Shannonsetnosy: + Mark.Shannon
messages: + msg304760
2017-10-11 08:31:28serhiy.storchakasetmessages: + msg304117
versions: + Python 3.7, - Python 3.4, Python 3.5
2017-05-23 22:08:54python-devsetpull_requests: + pull_request1854
2017-02-17 15:18:43gvanrossumsetnosy: - gvanrossum
2017-02-17 12:05:01njssetmessages: + msg287996
2017-02-17 12:01:49njssetnosy: + njs
messages: + msg287995
2016-03-02 19:25:40asvetlovsetnosy: + asvetlov
2015-12-17 23:09:22yselivanovsetmessages: + msg256622
2015-11-22 00:56:14martin.pantersetmessages: + msg255079
2015-11-21 03:28:44ncoghlansetmessages: + msg255039
2015-11-20 16:31:29yselivanovsetmessages: + msg254996
2015-11-20 16:30:08yselivanovsetmessages: + msg254995
2015-11-20 16:16:23gvanrossumsetmessages: + msg254991
2015-11-20 16:14:11yselivanovsetmessages: + msg254990
2015-11-19 01:18:17larrysetpriority: release blocker -> normal

messages: + msg254870
2015-11-18 22:22:02martin.pantersetmessages: + msg254858
2015-11-18 19:31:36yselivanovsetmessages: + msg254854
2015-11-17 23:36:15martin.pantersetmessages: + msg254822
2015-11-17 23:27:24vstinnersetmessages: + msg254821
2015-11-17 21:36:40martin.pantersetnosy: + martin.panter
messages: + msg254815
2015-11-17 18:52:18gvanrossumsetmessages: + msg254812
2015-11-17 17:59:00yselivanovsetnosy: gvanrossum, brett.cannon, ncoghlan, pitrou, vstinner, larry, benjamin.peterson, serhiy.storchaka, yselivanov, emptysquare
messages: + msg254809
2015-11-14 04:37:50emptysquaresetnosy: + emptysquare
2015-11-13 00:16:43yselivanovsetfiles: + gen_exc_1.patch
keywords: + patch
messages: + msg254578
2015-11-12 23:42:43yselivanovsetmessages: + msg254576
2015-11-12 22:19:50vstinnersetnosy: + pitrou
2015-11-12 22:18:57vstinnersetmessages: + msg254564
2015-11-12 22:14:06vstinnersetmessages: + msg254561
2015-11-12 21:24:39yselivanovsetnosy: + serhiy.storchaka
2015-11-12 21:13:53yselivanovsetnosy: + brett.cannon
messages: + msg254558
2015-11-12 21:00:58yselivanovsetversions: + Python 2.7
2015-11-12 20:59:18yselivanovcreate