classification
Title: Use an iterative implementation for contextlib.ExitStack.__exit__
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alonho, jcea, ncoghlan, python-dev
Priority: normal Keywords: patch

Created on 2012-05-30 11:07 by ncoghlan, last changed 2012-06-12 20:45 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
14963.patch alonho, 2012-05-30 22:36 review
14963.2.patch alonho, 2012-05-31 09:30 review
Messages (9)
msg161944 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-05-30 11:07
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
msg161971 - (view) Author: alon horev (alonho) * Date: 2012-05-30 22:36
The iterative approach turned out elegant and concise.
It actually now resembeles the implementation of nested's __exit__.
msg161978 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-05-31 00:15
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.
msg161986 - (view) Author: alon horev (alonho) * Date: 2012-05-31 09:30
that was indeed trickier, but overriding the __context__ attribute did the trick.
msg161999 - (view) Author: Roundup Robot (python-dev) Date: 2012-05-31 13:49
New changeset fc73e6ea9e73 by Nick Coghlan in branch 'default':
Issue #14963: Added test cases for contextlib.ExitStack exception handling behaviour (Initial patch by Alon Horev)
http://hg.python.org/cpython/rev/fc73e6ea9e73
msg162000 - (view) Author: Roundup Robot (python-dev) Date: 2012-05-31 14:00
New changeset c0c7618762e5 by Nick Coghlan in branch 'default':
Close #14963: Use an iterative algorithm in contextlib.ExitStack.__exit__ (Patch by Alon Horev)
http://hg.python.org/cpython/rev/c0c7618762e5
msg162001 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-05-31 14:08
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 #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/
msg162129 - (view) Author: alon horev (alonho) * Date: 2012-06-02 08:59
after #14969 has closed, can this be closed? any more action items?
msg162130 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-06-02 09:21
It *was* closed - I inadvertently reopened it with my comment. Fixed :)
History
Date User Action Args
2012-06-12 20:45:01jceasetnosy: + jcea
2012-06-02 09:21:26ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg162130

stage: test needed -> resolved
2012-06-02 08:59:29alonhosetmessages: + msg162129
2012-05-31 14:08:39ncoghlansetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg162001

stage: resolved -> test needed
2012-05-31 14:00:58python-devsetstatus: open -> closed
resolution: fixed
messages: + msg162000

stage: test needed -> resolved
2012-05-31 13:49:46python-devsetnosy: + python-dev
messages: + msg161999
2012-05-31 09:30:43alonhosetfiles: + 14963.2.patch

messages: + msg161986
2012-05-31 00:15:30ncoghlansetmessages: + msg161978
2012-05-30 22:36:05alonhosetfiles: + 14963.patch

nosy: + alonho
messages: + msg161971

keywords: + patch
2012-05-30 11:07:46ncoghlancreate