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

Created on 2015-11-12 20:59 by yselivanov, last changed 2017-10-11 08:31 by serhiy.storchaka.

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 open python-dev, 2017-05-23 22:08
Messages (24)
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 (haypo) * (Python committer) Date: 2015-11-12 22:14
Is this issue related to the issue #23353?
msg254564 - (view) Author: STINNER Victor (haypo) * (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 (haypo) * (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?
History
Date User Action Args
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:24hayposetmessages: + 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, haypo, 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:50hayposetnosy: + pitrou
2015-11-12 22:18:57hayposetmessages: + msg254564
2015-11-12 22:14:06hayposetmessages: + 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