Hang with contextlib.ExitStack and subprocess.Popen (regression)

[ValentinDavid]

BPO	27122
Nosy	@gpshead, @ncoghlan, @vstinner, @carljm, @berkerpeksag, @serhiy-storchaka, @1st1, @Vgr255
Files	hang_bug.py: An example of valid program that hangs hang_bug2.py issue27122-gps01.diff issue27122-additional-tests.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 = 'https://github.com/gpshead'
closed_at = <Date 2016-06-20.02:31:31.349>
created_at = <Date 2016-05-25.11:48:00.220>
labels = ['type-bug']
title = 'Hang with contextlib.ExitStack and subprocess.Popen (regression)'
updated_at = <Date 2017-03-25.00:31:22.113>
user = 'https://bugs.python.org/ValentinDavid'

bugs.python.org fields:

activity = <Date 2017-03-25.00:31:22.113>
actor = 'berker.peksag'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2016-06-20.02:31:31.349>
closer = 'serhiy.storchaka'
components = []
creation = <Date 2016-05-25.11:48:00.220>
creator = 'Valentin David'
dependencies = []
files = ['42989', '42999', '43354', '43447']
hgrepos = []
issue_num = 27122
keywords = ['patch']
message_count = 24.0
messages = ['266333', '266356', '266357', '266358', '266361', '266366', '266369', '266379', '266406', '268311', '268463', '268569', '268570', '268573', '268577', '268626', '268774', '268876', '268879', '279449', '279452', '290447', '290450', '290451']
nosy_count = 10.0
nosy_names = ['gregory.p.smith', 'ncoghlan', 'vstinner', 'carljm', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'yselivanov', 'abarry', 'Valentin David']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27122'
versions = ['Python 3.5']

[ValentinDavid]

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.

[vstinner]

Simplified script to reproduce the bug.

[vstinner]

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 bpo-22906)".

[vstinner]

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

[ncoghlan]

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.

[vstinner]

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?

[1st1]

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

[Vgr255]

Yes, and it seems that it is waiting for a review. http://bugs.python.org/issue25782

[ncoghlan]

Experimenting with this locally, it looks like the C level hang is indeed due to bpo-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.

[gpshead]

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 bpo-25782's problem with this one (that causes the hang, but once that is fixed isn't there still a problem here to be fixed)?

[ncoghlan]

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 bpo-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.