classification
Title: CPython hangs on error __context__ set to the error itself
Type: behavior Stage: patch review
Components: Interpreter Core Versions: Python 3.6, Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Rotem Yaari, Yury.Selivanov, georg.brandl, gregory.p.smith, gvanrossum, haypo, larry, ncoghlan, oconnor663, python-dev, serhiy.storchaka, terry.reedy, yselivanov
Priority: high Keywords: patch

Created on 2015-12-02 17:14 by yselivanov, last changed 2016-12-17 00:46 by terry.reedy.

Files
File name Uploaded Description Edit
Issue25782.patch yselivanov, 2015-12-02 18:32 review
Issue25782_2.patch yselivanov, 2015-12-02 19:41 review
Issue25782_3.patch yselivanov, 2015-12-02 22:01 review
Issue25782_4.patch yselivanov, 2015-12-17 22:58 review
set_context_reordering.patch serhiy.storchaka, 2015-12-18 16:04 review
set_context_reordering2.patch serhiy.storchaka, 2016-06-13 20:34 review
Issue25782_5.patch serhiy.storchaka, 2016-06-13 20:36 review
issue27122_broken_cm.py ncoghlan, 2016-06-13 23:03 Explicit reproducer for #27122
Messages (42)
msg255731 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 17:14
try:
    raise Exception
except Exception as ex:
    ex.__context__ = ex
    hasattr(1, 'aa')
msg255739 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 17:46
The bug is in "PyErr_SetObject":

            while ((context = PyException_GetContext(o))) {
                Py_DECREF(context);
                if (context == value) {
                    PyException_SetContext(o, NULL);
                    break;
                }
                o = context;
            }

The loop can be infinite.
msg255740 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 17:52
Looks like this is the original code committed in CPython in 2ee09afee126.  Patch by Antoine Pitrou.

Antoine, how would you fix this?
msg255744 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-02 18:06
I would change __context__ setter to check if it creates a loop.
msg255747 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 18:32
Serhiy, good idea, thanks!  Please review the attached patch.

Larry, I view this as a very serious bug.  Can we ship 3.5.1 with it fixed?
msg255752 - (view) Author: Jack O'Connor (oconnor663) * Date: 2015-12-02 18:59
Yury, do we need to handle more complicated infinite loops, where "self" doesn't actually show up in the loop? Here's an example:

    try:
        raise Exception
    except Exception as ex:
        loop1 = Exception()
        loop2 = Exception()
        loop1.__context__ = loop2
        loop2.__context__ = loop1
        ex.__context__ = loop1
        hasattr(1, 'aa')

I'm unfamiliar with CPython, so I don't know whether full-blown loop detection belongs here. Maybe we could add a hardcoded limit like "fail if we loop more than X times"?
msg255753 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 19:12
> Yury, do we need to handle more complicated infinite loops, where "self" doesn't actually show up in the loop? Here's an example:

My patch works for your example too.  Since it checks for loops in __context__ setter, you shouldn't be able to create complicated loops.
msg255754 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-02 19:12
Should we do the same for __cause__? Is it possible to create __context__ or __cause__ loop without assigning these attributes directly?
msg255758 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 19:41
> Should we do the same for __cause__? Is it possible to create __context__ or __cause__ loop without assigning these attributes directly?

Yes, let's mirror the __context__ behaviour for __cause__.  New patch attached.


Serhiy, Guido,

The new patch raises a TypeError in __cause__ and __context__ setters when a cycle was introduced, so in pure Python the following won't work:


   # will throw TypeError("cycle in exception context chain")
   ex.__context__ = ex chain")

   # will throw TypeError("cycle in exception cause chain")
   ex.__cause__ = ex


However, since PyException_SetContext and PyException_SetCause are public APIs, and their return type is 'void', I can't raise an error when a C code introduces a cycle, in that case, the exc->cause/exc->context will be set to NULL.

Thoughts?

I think that this approach is the only sane one here.  We can certainly fix the infinite loop in PyErr_SetObject, but it will only be a matter of time until we discover a similar loop somewhere else.
msg255759 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-12-02 19:50
Ouch, it's unfortunate those APIs don't have an error return. :-(

Setting it to NULL is one option -- silently ignoring the assignment
(leaving whatever was there) might also be good? In 90% of the cases it
would be the same thing right? (I'm not familiar with this part of the code
TBH.)
msg255760 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 19:54
> Setting it to NULL is one option -- silently ignoring the assignment
(leaving whatever was there) might also be good? In 90% of the cases it
would be the same thing right? 

But leaving the old __context__ there will completely mask the bug...

And as for pure python code -- do you agree we better raise a TypeError if a cycle is about to be introduced?
msg255766 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-12-02 20:03
> But leaving the old __context__ there will completely mask the bug...
OK, NULL is fine then.

>we better raise a TypeError if a cycle is about to be introduced?
Yes.
msg255767 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-02 20:06
Yet one option is the emersion of the exception. Affected exception is removed from the chain and added at it head.

If there is a chain A -> B -> C -> D -> E, after assignment C.__context__ = A we will get a chain C -> A -> B -> D -> E. No exception is lost.
msg255768 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-12-02 20:11
> Larry, I view this as a very serious bug.  Can we ship 3.5.1 with it fixed?

Don't do that, a few hours (!) is not enough to test a fix. It's too
late after a RC1 for such critical change (exceptions).

The bug was here since at least Python 3.3, there is no urgency to fix it.
msg255770 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 20:15
>Don't do that, a few hours (!) is not enough to test a fix. It's too
late after a RC1 for such critical change (exceptions).

Maybe we can add an RC2?
msg255771 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-12-02 20:21
> Maybe we can add an RC2?

Seriously? I'm waiting Python 3.5.1 since 3.5.0 was released. I'm amazed how much time it takes to release a first bugfix version, 3.5.0 was full a bugs (see the changelog).

It's very easy to workaround this issue in pure Python. Why do you want the fix *RIGHT NOW*?
msg255772 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 20:21
> If there is a chain A -> B -> C -> D -> E, after assignment C.__context__ = A we will get a chain C -> A -> B -> D -> E. No exception is lost.

What to do when you try to chain "C -> C"?  

I'm not sure I like this reordering idea -- it might introduce some *very* hard to find bugs -- you expected one type of exception, then you used some external library, and after that you have a completely different exception.
msg255773 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 20:24
> It's very easy to workaround this issue in pure Python. Why do you want the fix *RIGHT NOW*?

Please look at http://bugs.python.org/issue25779.  I think we either should fix this issue, or fix http://bugs.python.org/issue25786 in 3.5.1, since I can't find a workaround for it.  The latter issue is probably easier to get into 3.5.1?
msg255775 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-12-02 20:33
Yury Selivanov added the comment:
> Please look at http://bugs.python.org/issue25779.  I think we either should fix this issue, or fix http://bugs.python.org/issue25786 in 3.5.1, since I can't find a workaround for it.  The latter issue is probably easier to get into 3.5.1?

It's a choice for the release manager. But IHMO there will always be
bugs :-) It looks like you found a lot of issues related to exceptions
handling. Maybe it would be better to fix all the them "at once" in
3.5.2? And then ask Larry to release it early?
msg255776 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-02 20:39
> What to do when you try to chain "C -> C"?

Nothing. Or may be raise an exception (because C.__context__ can't be set to what you try).

> you expected one type of exception, then you used some external library, and after that you have a completely different exception.

I don't understand what new can add the reordering. It doesn't change original exception, it only changes __context__, but you change it in any case. If silently ignore the loop, you would get __context__ different from what you expected. If raise TypeError, you would get TypeError instead of expected exception.

And if the decision will be to raise TypeError, may be RuntimeError is more appropriate?
msg255780 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-12-02 21:14
> Please look at http://bugs.python.org/issue25779.

You closed that one and marked it "not a bug"...?
msg255781 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 21:16
> You closed that one and marked it "not a bug"...?

That particular issue was about asyncio.  After reducing it, I created two new issues -- this one, and another one about another bug in contextlib.
msg255788 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-02 22:01
Serhiy, Victor, thank you for your reviews. Another version of the patch is attached.
msg255995 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-12-06 00:45
I'm not going to hold up 3.5.1 for this.
msg256618 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-12-17 22:58
A new patch is attached.  Please review.

I decided to remove the fix for recursive __cause__.  Currently, `raise e from e` doesn't cause any problem, and if we fix the interpreter to raise an RuntimeError in such cases it will be a backwards incompatible change.  I don't see any point in introducing such a change in a bugfix Python release.  We can open a separate issue for 3.6 though.

Fixing __context__ in 3.5.2 is way more important, since the interpreter can actually infinitely loop itself in some places.  And since there is no syntax for setting __context__ manually (as opposed to __cause__ via raise .. from), I suspect that there will be much less people affected by this fix.
msg256644 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-18 06:40
The patch LGTM (but I prefer reordering solution).
msg256681 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2015-12-18 14:06
> Serhiy Storchaka added the comment:
> 
> The patch LGTM (but I prefer reordering solution).

Serhiy, could you please describe your solution in more detail? An elaborate example would help. Perhaps I just misunderstand your idea. And thanks for the review!
msg256687 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-18 15:04
Here is a patch that illustrates my idea (the documentation could be better).
msg256688 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-18 15:09
And added more comments to your patch.
msg268309 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-06-12 01:16
Patch review: I see two competing solutions with differing behaviors. 

Yury's raises a RuntimeError in the loop situation.

Serhiy's simply reorders the exception context to put the referred to one at the front of the chain in the event of a loop.

The 3.5 Release manager or someone familiar with the contextlib ExitStack code where this is triggered should make the decision as to the best way to fix this.

(I came to this bug via the ExitStack + subprocess issue27122 which merely has a suggested workaround patch as a band aid that might help until this is fixed)
msg268310 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-06-12 01:23
I'm not the right person to decide this.

As RM, all I can do is decide whether or not to hold up a release based on the bug.  The answer: no.  Since this isn't fixed yet in the 3.5 branch, 3.5.2 will go out without it being fixed.  Sorry folks.
msg268465 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-06-13 19:27
> Yury's raises a RuntimeError in the loop situation.

> Serhiy's simply reorders the exception context to put the referred to one at the front of the chain in the event of a loop.

Right, and I believe that my solution is more Pythonic.  Reordering feels highly unintuitive to me.
msg268467 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-06-13 19:52
For both of the proposed patches, could we add a test case based on contextlib.ExitStack and a variant of Victor's #27122 reproducer script at http://bugs.python.org/file42999/hang_bug2.py that uses a deliberately broken __exit__ implementation that always re-raises the exception thrown into the underlying generator?

As discussed on that issue, I'm wondering if we need some additional defensive coding in ExitStack itself, or if resolving this underlying problem will be enough to also categorically prevent the hang in ExitStack.

As far as possible resolutions go, I tend to prefer lying about the shape of the exception state over changing the type of the raised exception - we already know the true exception state is a tree rather than a chain (see #18861 for some related discussions), and we should be able to ensure that all active exceptions remain somewhere in the chain, even if their order is a bit surprising.
msg268472 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-13 20:34
With reordering the issue27122 test is passed without changes. No changes in ExitStack is needed. Here is updated patch.
msg268473 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-13 20:36
With Yury's path the issue27122 test need to be changed (RuntimeError is now raised instead of original exception). Here is updated patch.
msg268485 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-06-13 23:03
Thanks Serhiy. I've attached a new file (issue27122_broken_cm.py) with a context manager that is deliberately buggy in the same way as contextlib._GeneratorContextManager is currently, so the new test can be made independent of #27122 being fixed (Greg Smith has a pending patch to make that CM well behaved again, which would currently lead to your new test passing for the wrong reasons)

issue27122_broken_cm.py also shows why I think "make it work" is the right answer here - the odd context chaining is tolerated in the case where the interpreter is implicitly setting the exception context, so I believe it should also be tolerated when ExitStack sets the context explicitly. If we "do the right thing" (where "right" = "the same as what the interpreter does") in the setter, then not only ExitStack, but anyone else setting __context__ should automatically benefit from the adjustment.
msg268509 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-06-14 03:48
> issue27122_broken_cm.py also shows why I think "make it work" is the right answer here [..]

But fixing this issue by reordering the exception chain will only mask bugs that just better to be fixed.  And sometimes, this will cause weird exceptions chains, that will make *some* people spend a lot of time debugging/googling to understand what's going on.
msg268563 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-06-14 15:36
> But fixing this issue by reordering the exception chain will only mask bugs that just better to be fixed.  And sometimes, this will cause weird exceptions chains, that will make *some* people spend a lot of time debugging/googling to understand what's going on.

Right, but the way to fix that is to figure out a more coherent linearisation strategy (and/or move away from linearisation by allowing __context__ to be a tuple). Both #18861 and the frame annotation idea it spawned in #19585 have some ideas on how we might be able to improve that situation.

Adding *yet another* exception to an already confused exception chain isn't likely to make it easier to debug, and may mask the original exception that triggered the misbehaving error handler (entirely defeating the point of adding exception chaining in the first place).

I'm not saying Serhiy's patch is the right long term solution - I'm saying I think it's a better short term mitigation measure, since it doesn't lose information the way nulling out the __context__ may do.
msg268565 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-06-14 16:06
I think I'm with Nick on this, the re-ordering fix is the closest to the right thing to do for 3.5 as it at least leaves all exceptions present in the chain.

As a more accurate long term fix, it would be great if we could express the tree via tuples in exception context in 3.6 or later.
msg268567 - (view) Author: Roundup Robot (python-dev) Date: 2016-06-14 16:25
New changeset 9ee36b74b432 by Gregory P. Smith in branch '3.5':
Issue #27123: When an exception is raised within the context being
https://hg.python.org/cpython/rev/9ee36b74b432

New changeset 9fadeee05880 by Gregory P. Smith in branch 'default':
Issue #27123: When an exception is raised within the context being
https://hg.python.org/cpython/rev/9fadeee05880
msg268578 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-14 17:51
For reference: the correct issue number is #27122.

Are there examples of creating a loop not involving ExitStack or explicit setting the __context__ attribute?
msg283459 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-12-17 00:46
Issue #28962 appears to be an example of this issue.  It uses 'raise e from e' to create the loop.
History
Date User Action Args
2016-12-17 00:46:50terry.reedysetnosy: + terry.reedy
messages: + msg283459
2016-12-13 20:47:06serhiy.storchakalinkissue28962 dependencies
2016-06-14 17:51:28serhiy.storchakasetmessages: + msg268578
2016-06-14 16:30:22gregory.p.smithunlinkissue27122 dependencies
2016-06-14 16:25:00python-devsetnosy: + python-dev
messages: + msg268567
2016-06-14 16:06:12gregory.p.smithsetmessages: + msg268565
2016-06-14 15:36:15ncoghlansetmessages: + msg268563
2016-06-14 03:48:18yselivanovsetmessages: + msg268509
2016-06-13 23:03:57ncoghlansetfiles: + issue27122_broken_cm.py

messages: + msg268485
2016-06-13 20:36:28serhiy.storchakasetfiles: + Issue25782_5.patch

messages: + msg268473
2016-06-13 20:34:01serhiy.storchakasetfiles: + set_context_reordering2.patch

messages: + msg268472
2016-06-13 19:52:50ncoghlansetmessages: + msg268467
2016-06-13 19:27:21yselivanovsetmessages: + msg268465
2016-06-12 01:23:42larrysetmessages: + msg268310
2016-06-12 01:17:52gregory.p.smithlinkissue27122 dependencies
2016-06-12 01:17:15gregory.p.smithsetstage: commit review -> patch review
2016-06-12 01:16:59gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg268309
2016-05-26 09:57:03Rotem Yaarisetnosy: + Rotem Yaari
2015-12-18 16:04:18serhiy.storchakasetfiles: + set_context_reordering.patch
2015-12-18 16:04:01serhiy.storchakasetfiles: - set_context_reordering.patch
2015-12-18 15:09:11serhiy.storchakasetmessages: + msg256688
2015-12-18 15:04:16serhiy.storchakasetfiles: + set_context_reordering.patch

messages: + msg256687
2015-12-18 14:06:50Yury.Selivanovsetnosy: + Yury.Selivanov
messages: + msg256681
2015-12-18 06:40:44serhiy.storchakasettype: behavior
stage: patch review -> commit review
messages: + msg256644
versions: - Python 3.3, Python 3.4
2015-12-17 22:58:06yselivanovsetfiles: + Issue25782_4.patch

messages: + msg256618
stage: patch review
2015-12-06 00:45:30larrysetpriority: release blocker -> high

messages: + msg255995
2015-12-02 22:06:58pitrousetnosy: - pitrou
2015-12-02 22:01:08yselivanovsetfiles: + Issue25782_3.patch

messages: + msg255788
2015-12-02 21:16:24yselivanovsetmessages: + msg255781
2015-12-02 21:14:19larrysetmessages: + msg255780
2015-12-02 20:39:53serhiy.storchakasetmessages: + msg255776
2015-12-02 20:33:38hayposetmessages: + msg255775
2015-12-02 20:24:15yselivanovsetmessages: + msg255773
2015-12-02 20:21:50yselivanovsetmessages: + msg255772
2015-12-02 20:21:42hayposetmessages: + msg255771
2015-12-02 20:15:29yselivanovsetmessages: + msg255770
2015-12-02 20:11:00hayposetmessages: + msg255768
2015-12-02 20:06:57serhiy.storchakasetmessages: + msg255767
2015-12-02 20:03:58gvanrossumsetmessages: + msg255766
2015-12-02 19:54:39yselivanovsetmessages: + msg255760
2015-12-02 19:50:33gvanrossumsetmessages: + msg255759
2015-12-02 19:41:55yselivanovsetfiles: + Issue25782_2.patch

messages: + msg255758
2015-12-02 19:12:59serhiy.storchakasetmessages: + msg255754
2015-12-02 19:12:13yselivanovsetmessages: + msg255753
2015-12-02 18:59:08oconnor663setnosy: + oconnor663
messages: + msg255752
2015-12-02 18:32:16yselivanovsetfiles: + Issue25782.patch
keywords: + patch
messages: + msg255747
2015-12-02 18:06:14serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg255744
2015-12-02 17:52:26yselivanovsetnosy: + pitrou
messages: + msg255740
2015-12-02 17:46:21yselivanovsetmessages: + msg255739
2015-12-02 17:15:25yselivanovsetpriority: normal -> release blocker
nosy: + larry, georg.brandl
2015-12-02 17:14:19yselivanovlinkissue25779 superseder
2015-12-02 17:14:01yselivanovcreate