Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ValentinDavid mannequin opened this issue May 25, 2016 · 24 comments
Closed

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

ValentinDavid mannequin opened this issue May 25, 2016 · 24 comments
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@ValentinDavid
Copy link
Mannequin

ValentinDavid mannequin commented May 25, 2016

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
    Copy link
    Mannequin Author

    ValentinDavid mannequin commented May 25, 2016

    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
    Copy link
    Member

    Simplified script to reproduce the bug.

    @vstinner
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Contributor

    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
    Copy link
    Member

    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
    Copy link
    Member

    1st1 commented May 25, 2016

    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
    Copy link
    Mannequin

    Vgr255 mannequin commented May 25, 2016

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

    @ncoghlan
    Copy link
    Contributor

    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 gpshead self-assigned this May 28, 2016
    @gpshead
    Copy link
    Member

    gpshead commented Jun 12, 2016

    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)?

    @gpshead gpshead assigned ncoghlan and unassigned gpshead Jun 12, 2016
    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan ncoghlan assigned gpshead and unassigned ncoghlan Jun 13, 2016
    @gpshead
    Copy link
    Member

    gpshead commented Jun 14, 2016

    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

    @gpshead
    Copy link
    Member

    gpshead commented Jun 14, 2016

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

    @gpshead gpshead closed this as completed Jun 14, 2016
    @gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Jun 14, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 14, 2016

    New changeset 6bb68eae63e8 by Gregory P. Smith in branch '3.5':
    bpo-27122: fix typo in the news file, wrong issue #. not bpo-27123.
    https://hg.python.org/cpython/rev/6bb68eae63e8

    New changeset 4e1dbfcc9449 by Gregory P. Smith in branch 'default':
    bpo-27122: fix typo in the news file, wrong issue #. not bpo-27123.
    https://hg.python.org/cpython/rev/4e1dbfcc9449

    @serhiy-storchaka
    Copy link
    Member

    Is it worth to add more specific tests for __context__ and __cause__ attributes of an exception?

    @ncoghlan
    Copy link
    Contributor

    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.

    @serhiy-storchaka
    Copy link
    Member

    Here are additional tests.

    @ncoghlan
    Copy link
    Contributor

    Serhiy's test changes look good to me!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 20, 2016

    New changeset a14b93a4eb49 by Serhiy Storchaka in branch '3.5':
    Added more tests for issue bpo-27122.
    https://hg.python.org/cpython/rev/a14b93a4eb49

    New changeset ebc82b840163 by Serhiy Storchaka in branch 'default':
    Added more tests for issue bpo-27122.
    https://hg.python.org/cpython/rev/ebc82b840163

    @carljm
    Copy link
    Member

    carljm commented Oct 25, 2016

    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! ;-)

    @gpshead
    Copy link
    Member

    gpshead commented Oct 25, 2016

    we talented!

    @berkerpeksag
    Copy link
    Member

    New changeset c6d2f49 by Berker Peksag in branch '3.5':
    bpo-27122: Fix comment to point to correct issue number (#50)
    c6d2f49

    @berkerpeksag
    Copy link
    Member

    New changeset 89b1824 by Berker Peksag in branch '3.6':
    bpo-27122: Fix comment to point to correct issue number (#48)
    89b1824

    @berkerpeksag
    Copy link
    Member

    New changeset af88e7e by Berker Peksag (Nathaniel J. Smith) in branch 'master':
    bpo-27122: Fix comment to point to correct issue number (#47)
    af88e7e

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants