classification
Title: Hang with contextlib.ExitStack and subprocess.Popen (regression)
Type: behavior Stage: resolved
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Valentin David, berker.peksag, carljm, ebarry, gregory.p.smith, haypo, ncoghlan, python-dev, serhiy.storchaka, yselivanov
Priority: normal Keywords: patch

Created on 2016-05-25 11:48 by Valentin David, last changed 2017-03-25 00:31 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
hang_bug.py Valentin David, 2016-05-25 11:48 An example of valid program that hangs
hang_bug2.py haypo, 2016-05-25 13:52
issue27122-gps01.diff gregory.p.smith, 2016-06-12 02:03 review
issue27122-additional-tests.patch serhiy.storchaka, 2016-06-18 07:36 review
Messages (24)
msg266333 - (view) Author: Valentin David (Valentin David) Date: 2016-05-25 11:48
The attached script hangs while using 100% on Python 3.5.1 but not on Python 3.4.3. Tested both on Gentoo Linux and Ubuntu Linux.

The loop seems to be in PyErr_SetObject in a loop that recursively go through PyException_GetContext.

subprocess.Popen seems to cause the issue while calling io.open. strace reveals a error on call to lseek on a pipe.

The issue does not happen when using nested "with" instead of ExitStack.
msg266356 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-05-25 13:52
Simplified script to reproduce the bug.
msg266357 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-05-25 13:55
It's a regression introduced by the PEP 479 in Lib/contextlib.py by the changeset 36a8d935c322, "PEP 479: Change StopIteration handling inside generators (Closes issue #22906)".
msg266358 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-05-25 13:57
Workaround, or maybe fix?, for the issue:

diff -r 0af15b8ef3b2 Lib/contextlib.py
--- a/Lib/contextlib.py Thu May 12 10:37:58 2016 +0300
+++ b/Lib/contextlib.py Wed May 25 15:56:50 2016 +0200
@@ -87,6 +87,8 @@ class _GeneratorContextManager(ContextDe
                 # (see PEP 479).
                 if exc.__cause__ is value:
                     return False
+                if exc is value:
+                    return
                 raise
             except:
                 # only re-raise if it's *not* the exception that was
msg266361 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-05-25 14:30
The suggested fix looks basically correct to me - the problem is that the new except clause added by that patch is currently missing the "don't re-raise the passed in exception" logic needed to fully abide by the context management protocol, but we missed that on the patch review.

The only tweak I would recommend is putting that check before the check for StopIteration being converted to RuntimeError so it's immediately obvious to the reader that it takes priority over the exc.__cause__ check.

The comment on that clause should also be adjusted to reflect the fixed behaviour.
msg266366 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-05-25 14:43
> The loop seems to be in PyErr_SetObject in a loop that recursively go through PyException_GetContext.

We should also fix this function to avoid the infinite loop.

Maybe we should deny setting "exc.__context__=exc" (modify PyException_SetContext for that). Maybe raise a new RuntimeError in this case?
msg266369 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-05-25 15:53
> 
> Maybe we should deny setting "exc.__context__=exc" (modify PyException_SetContext for that). Maybe raise a new RuntimeError in this case?

There is a patch for that somewhere on the tracker
msg266379 - (view) Author: Emanuel Barry (ebarry) * Date: 2016-05-25 17:30
Yes, and it seems that it is waiting for a review. http://bugs.python.org/issue25782
msg266406 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-05-26 03:55
Experimenting with this locally, it looks like the C level hang is indeed due to #25782, but the fact that we're implicitly trying to do "exc.__context__ = exc" is a new bug in _GeneratorContextManager introduced by the PEP 479 changes.

However, I'm now wondering whether that's also revealed a more general oversight in ExitStack's "_fix_exception_context" internal helper function: it's not explicitly handling the case where an __exit__ method re-raises the exception that was passed in, rather than returning a false value (indicating to the context management machinery that it should re-raise the original exception). I just hadn't noticed that before since all stdlib context managers are well-behaved in that regard (modulo bugs like the PEP 479 one).

Some failed attempts to create a simpler reproducer than Victor's last example show it isn't straightforward to get the current code to misbehave, but an upfront check for "new_exc is old_exc" may still be a worthwhile defensive coding measure.
msg268311 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-06-12 02:03
Proposed fix based on STINNER and Nick's earlier comments and the simple reproducer turned into a test.

Nick - your most recent comment makes me wonder if this shouldn't be doing more.

this does make both hang_bug*.py examples do something reasonable.

I'm at a loss for what Misc/NEWS entry to write for this one as I don't know how to accurately describe the scope of the problem.  I'd also like to avoid conflating issue25782's problem with this one (that causes the hang, but once that is fixed isn't there still a problem here to be fixed)?
msg268463 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-06-13 19:24
Greg, I think you should apply your change to eliminate the regression and get us back to the state of all stdlib context managers being well-behaved in this regard (we unfortunately missed the 3.5.2 release, but that will ensure it's fixed for 3.5.3).

Given the challenges I had trying to reproduce the hang with a plain context manager rather than a generator, I now the problem is specifically just #25782 - in the absence of that, ExitStack() should misbehave in the same way actual nested with statements would, rather than trying to implicitly "fix" the misbehaving context managers.
msg268569 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-06-14 16:29
Fixes are in 3.5 and default:

remote: notified python-checkins@python.org of incoming changeset 9ee36b74b432
remote: notified python-checkins@python.org of incoming changeset 9fadeee05880
msg268570 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-06-14 16:30
(with a follow up change to fix the typo in the news file which the bot hasn't seen fit to mention here yet)

i'm removing the 25782 dependency as that isn't blocking fixing this.
msg268573 - (view) Author: Roundup Robot (python-dev) Date: 2016-06-14 17:38
New changeset 6bb68eae63e8 by Gregory P. Smith in branch '3.5':
issue27122: fix typo in the news file, wrong issue #.  not issue27123.
https://hg.python.org/cpython/rev/6bb68eae63e8

New changeset 4e1dbfcc9449 by Gregory P. Smith in branch 'default':
issue27122: fix typo in the news file, wrong issue #.  not issue27123.
https://hg.python.org/cpython/rev/4e1dbfcc9449
msg268577 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-14 17:49
Is it worth to add more specific tests for __context__ and __cause__ attributes of an exception?
msg268626 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-06-15 18:09
There's an existing scenario test at https://hg.python.org/cpython/file/default/Lib/test/test_contextlib.py#l600 and https://hg.python.org/cpython/file/default/Lib/test/test_contextlib.py#l648 that aims to ensure ExitStack unwinding and context setting matches the behaviour of actual with statements.

It wouldn't hurt to add some explicit checks to the new test for this particular bug, though.
msg268774 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-18 07:36
Here are additional tests.
msg268876 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-06-20 00:58
Serhiy's test changes look good to me!
msg268879 - (view) Author: Roundup Robot (python-dev) Date: 2016-06-20 02:30
New changeset a14b93a4eb49 by Serhiy Storchaka in branch '3.5':
Added more tests for issue #27122.
https://hg.python.org/cpython/rev/a14b93a4eb49

New changeset ebc82b840163 by Serhiy Storchaka in branch 'default':
Added more tests for issue #27122.
https://hg.python.org/cpython/rev/ebc82b840163
msg279449 - (view) Author: Carl Meyer (carljm) * Date: 2016-10-25 19:30
Greg, there was also a (different!) typo of the issue number in the code comment committed with this fix; that typo hasn't been fixed. Sent me on quite the chase looking for this bug. (I tracked down the bug independently, then wasn't able to repro it on trunk and found your fix and code comment). It's an impressive achievement to typo the same bug ID two different ways within the same twelve-line patch! ;-)
msg279452 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-10-25 19:54
we talented!
msg290447 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-25 00:27
New changeset c6d2f498142c29ed62241ab6d89cb7b5e38f2fca by Berker Peksag in branch '3.5':
bpo-27122: Fix comment to point to correct issue number (#50)
https://github.com/python/cpython/commit/c6d2f498142c29ed62241ab6d89cb7b5e38f2fca
msg290450 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-25 00:28
New changeset 89b1824e693419b20b6a9113e5293f1c1a78065f by Berker Peksag in branch '3.6':
bpo-27122: Fix comment to point to correct issue number (#48)
https://github.com/python/cpython/commit/89b1824e693419b20b6a9113e5293f1c1a78065f
msg290451 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-25 00:31
New changeset af88e7eda4101f36e904771d3cf59a5f740b3b00 by Berker Peksag (Nathaniel J. Smith) in branch 'master':
bpo-27122: Fix comment to point to correct issue number (#47)
https://github.com/python/cpython/commit/af88e7eda4101f36e904771d3cf59a5f740b3b00
History
Date User Action Args
2017-03-25 00:31:22berker.peksagsetmessages: + msg290451
2017-03-25 00:28:26berker.peksagsetmessages: + msg290450
2017-03-25 00:27:40berker.peksagsetnosy: + berker.peksag
messages: + msg290447
2016-10-25 19:54:29gregory.p.smithsetmessages: + msg279452
2016-10-25 19:30:07carljmsetnosy: + carljm
messages: + msg279449
2016-06-20 02:31:31serhiy.storchakasetstatus: open -> closed
2016-06-20 02:30:58python-devsetmessages: + msg268879
2016-06-20 00:58:29ncoghlansetmessages: + msg268876
2016-06-18 07:36:20serhiy.storchakasetstatus: closed -> open
files: + issue27122-additional-tests.patch
messages: + msg268774
2016-06-15 18:09:41ncoghlansetmessages: + msg268626
2016-06-14 17:49:48serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg268577
2016-06-14 17:38:58python-devsetnosy: + python-dev
messages: + msg268573
2016-06-14 16:32:02gregory.p.smithsetstatus: open -> closed
type: behavior
resolution: fixed
stage: resolved
2016-06-14 16:30:22gregory.p.smithsetdependencies: - CPython hangs on error __context__ set to the error itself
messages: + msg268570
2016-06-14 16:29:00gregory.p.smithsetmessages: + msg268569
2016-06-13 23:09:47ncoghlansetassignee: ncoghlan -> gregory.p.smith
2016-06-13 19:24:19ncoghlansetmessages: + msg268463
2016-06-12 02:03:39gregory.p.smithsetassignee: gregory.p.smith -> ncoghlan
2016-06-12 02:03:05gregory.p.smithsetfiles: + issue27122-gps01.diff
keywords: + patch
messages: + msg268311
2016-06-12 01:17:53gregory.p.smithsetdependencies: + CPython hangs on error __context__ set to the error itself
2016-05-28 21:57:59gregory.p.smithsetassignee: gregory.p.smith
2016-05-26 03:55:17ncoghlansetmessages: + msg266406
2016-05-25 23:44:41gregory.p.smithsetnosy: + gregory.p.smith, - gps
2016-05-25 17:30:46ebarrysetnosy: + ebarry
messages: + msg266379
2016-05-25 15:53:16yselivanovsetmessages: + msg266369
2016-05-25 14:43:01hayposetmessages: + msg266366
2016-05-25 14:30:40ncoghlansetmessages: + msg266361
2016-05-25 13:57:11hayposetmessages: + msg266358
2016-05-25 13:55:54hayposetnosy: + yselivanov
2016-05-25 13:55:48hayposetmessages: + msg266357
2016-05-25 13:52:59hayposetfiles: + hang_bug2.py
nosy: + haypo
messages: + msg266356

2016-05-25 13:32:26r.david.murraysetnosy: + gps
2016-05-25 13:31:54r.david.murraysetnosy: + ncoghlan
2016-05-25 11:48:00Valentin Davidcreate