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

Use an iterative implementation for contextlib.ExitStack.__exit__ #59168

Closed
ncoghlan opened this issue May 30, 2012 · 9 comments
Closed

Use an iterative implementation for contextlib.ExitStack.__exit__ #59168

ncoghlan opened this issue May 30, 2012 · 9 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 14963
Nosy @jcea, @ncoghlan
Files
  • 14963.patch
  • 14963.2.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 = None
    closed_at = <Date 2012-06-02.09:21:26.479>
    created_at = <Date 2012-05-30.11:07:46.653>
    labels = ['type-feature', 'library']
    title = 'Use an iterative implementation for contextlib.ExitStack.__exit__'
    updated_at = <Date 2012-06-12.20:45:01.486>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2012-06-12.20:45:01.486>
    actor = 'jcea'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-06-02.09:21:26.479>
    closer = 'ncoghlan'
    components = ['Library (Lib)']
    creation = <Date 2012-05-30.11:07:46.653>
    creator = 'ncoghlan'
    dependencies = []
    files = ['25763', '25770']
    hgrepos = []
    issue_num = 14963
    keywords = ['patch']
    message_count = 9.0
    messages = ['161944', '161971', '161978', '161986', '161999', '162000', '162001', '162129', '162130']
    nosy_count = 4.0
    nosy_names = ['jcea', 'ncoghlan', 'alonho', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue14963'
    versions = ['Python 3.3']

    @ncoghlan
    Copy link
    Contributor Author

    The current implementation of contextlib.ExitStack [1] actually creates a nested series of frames when unwinding the callback stack in an effort to ensure exceptions are chained correctly, just as they would be if using nested with statements.

    It would be nice to avoid this overhead by just using the one frame to iterate over the callbacks and handling correct exception chaining directly. This is likely to be a little tricky to get right, though, so the first step would be to set up a test that throws and suppresses a few exceptions and ensures the chaining when using ExitStack matches that when using nested with statements.

    [1] http://hg.python.org/cpython/file/94a5bf416e50/Lib/contextlib.py#l227

    @ncoghlan ncoghlan added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 30, 2012
    @alonho
    Copy link
    Mannequin

    alonho mannequin commented May 30, 2012

    The iterative approach turned out elegant and concise.
    It actually now resembeles the implementation of nested's __exit__.

    @ncoghlan
    Copy link
    Contributor Author

    Sorry, I wasn't clear on what I meant by "chained correctly", and that's the part that makes this trickier than the way contextlib.nested did it. I'm referring to the __context__ attribute on exceptions that is set automatically when an exception occurs in another exception handler, which makes error displays like the following possible:

    >>> from contextlib import ExitStack
    >>> with ExitStack() as stack:
    ...     @stack.callback
    ...     def f():
    ...         1/0
    ...     @stack.callback
    ...     def f():
    ...         {}[1]
    ... 
    Traceback (most recent call last):
      File "/home/ncoghlan/devel/py3k/Lib/contextlib.py", line 243, in _invoke_next_callback
        suppress_exc = _invoke_next_callback(exc_details)
      File "/home/ncoghlan/devel/py3k/Lib/contextlib.py", line 240, in _invoke_next_callback
        return cb(*exc_details)
      File "/home/ncoghlan/devel/py3k/Lib/contextlib.py", line 200, in _exit_wrapper
        callback(*args, **kwds)
      File "<stdin>", line 7, in f
    KeyError: 1
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<stdin>", line 5, in <module>
      File "/home/ncoghlan/devel/py3k/Lib/contextlib.py", line 256, in __exit__
        return _invoke_next_callback(exc_details)
      File "/home/ncoghlan/devel/py3k/Lib/contextlib.py", line 245, in _invoke_next_callback
        suppress_exc = cb(*sys.exc_info())
      File "/home/ncoghlan/devel/py3k/Lib/contextlib.py", line 200, in _exit_wrapper
        callback(*args, **kwds)
      File "<stdin>", line 4, in f
    ZeroDivisionError: division by zero

    The recursive approach maintains that behaviour automatically because it really does create a nested set of exception handlers. With the iterative approach, we leave the exception handler before invoking the next callback, so we end up bypassing the native chaining machinery and will need to recreate it manually.

    If you can make that work, then we'd end up with the best of both worlds: the individual exceptions would be clean (since they wouldn't be cluttered with the recursive call stack created by the unwinding process), but exception chaining would still keep track of things if multiple exceptions are encountered in cleanup operations.

    @alonho
    Copy link
    Mannequin

    alonho mannequin commented May 31, 2012

    that was indeed trickier, but overriding the __context__ attribute did the trick.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 31, 2012

    New changeset fc73e6ea9e73 by Nick Coghlan in branch 'default':
    Issue bpo-14963: Added test cases for contextlib.ExitStack exception handling behaviour (Initial patch by Alon Horev)
    http://hg.python.org/cpython/rev/fc73e6ea9e73

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 31, 2012

    New changeset c0c7618762e5 by Nick Coghlan in branch 'default':
    Close bpo-14963: Use an iterative algorithm in contextlib.ExitStack.__exit__ (Patch by Alon Horev)
    http://hg.python.org/cpython/rev/c0c7618762e5

    @python-dev python-dev mannequin closed this as completed May 31, 2012
    @ncoghlan
    Copy link
    Contributor Author

    Interesting - it turns out we can't fully reproduce the behaviour of nested with statements in ExitStack (see the new reference test I checked in, as well as bpo-14969)

    I added one technically redundant variable to the implementation to make it more obviously correct to the reader, as well as a test that ensures the stack can handle ridiculous numbers of callbacks without failing (a key advantage of using a single frame rather than one frame per callback)

    While it isn't mandatory, we prefer it if contributors submit Contributor Agreements even for small changes. If you're happy to do that, I consider emailing a scanned or digitally photographed copy of the signed form as described here to be the simplest currently available approach: http://www.python.org/psf/contrib/

    @ncoghlan ncoghlan reopened this May 31, 2012
    @alonho
    Copy link
    Mannequin

    alonho mannequin commented Jun 2, 2012

    after bpo-14969 has closed, can this be closed? any more action items?

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Jun 2, 2012

    It *was* closed - I inadvertently reopened it with my comment. Fixed :)

    @ncoghlan ncoghlan closed this as completed Jun 2, 2012
    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant