classification
Title: contextlib.nested inconsistent with, well, nested with statements due exceptions raised in __enter__
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: out of date
Dependencies: Superseder: Add contextlib.ExitStack
View: 13585
Assigned To: ncoghlan Nosy List: jwp, ncoghlan
Priority: low Keywords: patch

Created on 2009-02-13 18:09 by jwp, last changed 2012-05-07 11:40 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
nested_issue.py jwp, 2009-02-13 18:09 shows the variations of nested context managers and their results
pep377_v2.diff ncoghlan, 2009-03-15 03:04 Second draft review
pep377_v3.diff ncoghlan, 2009-03-15 12:29 Functionally complete (except compiler package) review
pep377_bench.py ncoghlan, 2009-03-15 12:31 Micro benchmark for with statement speed
outer_sees_no_exc.py jwp, 2009-03-16 02:34
Messages (20)
msg81957 - (view) Author: James William Pye (jwp) Date: 2009-02-13 18:09
Basically, nested() doesn't seem to be consistent with explicitly nested
with-statements when an exception is thrown in a CM's __enter__.

Consider a pair of nested CMs, the inner __enter__ raises an exception
trapped by the outer. In the situation of explicitly nested
with-statements, the inner's block will not be ran as an exception was
raised prior to the block. The outer traps the exception and code
continues to flow after the outer's block because the exception was
*not* raised.
Currently, if contextlib.nested() is used in such a situation, it blows
up with a RuntimeError("generator didn't yield").

See the attached file for a set of naive variations and their resulting
exceptions or lack thereof in the situation of explicitly nested CMs.

Despite the RuntimeError raised by nested(), I'm not sure it's
*currently* possible for an implementation of nested() to be *totally*
consistent with explicitly nested with-statements. It would seem that an
additional facility(AbortBlock exception?) would be needed to
communicate that the block should not actually be ran, and that no
exception should be raised as it was consumed by an "outer" CM.


If this is considered to be the intended behavior, I would think the
doc-string on contextlib.nested should be updated to document the
inconsistency as it currently states that nested() and nested with
statements are equivalent. Based on the results of the attached file in
Python 3.0, they are clearly not.

Cheers folks; lovin' CMs. And, of course, apologies if this has already
been discussed. :P
msg82701 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-02-25 11:36
That's an interesting problem... since one of the things I like about
CMs is the ability to turn pretty much any block of code into a context
manager by substituting a yield statement at the appropriate point, not
having the ability to skip the yield is actually something of a hole in
the design.

This will need a bit of python-dev discussion, and may even turn into a
PEP (assuming we do go ahead with adding a SkipBlock exception and
modifying the with statement to check for it being raised by __enter__()
methods).
msg82886 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-02-27 23:56
I'll be working on a PEP for 2.7/3.1 to try to get the semantics of with
statement changed as suggested.

http://mail.python.org/pipermail/python-dev/2009-February/086470.html
msg83297 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-08 04:14
PEP 377 (http://www.python.org/dev/peps/pep-0377/) has now been submitted.
msg83304 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-08 05:44
Attached simple benchmark script to check for any slowdowns introduced
by the planned with statement changes.
msg83588 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-14 15:08
First draft of patch attached.

Limitations of this version of the patch:
- includes some unrelated marshal.c changes (improved error messages)
- no tests of the new functionality
- contextlib not changed yet
- no documentation changes

The new semantics are in place though, and the existing test_with and
test_contextlib tests all still pass.
msg83615 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-15 02:50
Removed first draft of patch - it was fundamentally flawed (it didn't
clean up the stack properly when the statement body was skipped)
msg83616 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-15 03:04
Second draft attached, this time with tests and sans segmentation faults :)

Limitations of this version of the patch:
- still includes some unrelated marshal.c changes (improved error messages)
- no documentation changes yet
msg83617 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-15 04:04
Note that the semantics in the current patch aren't quite correct, since
the __exit__ attribute is retrieved inside the scope of the outer
try/except block. Updated patch with the correct semantics coming soon.
msg83621 - (view) Author: James William Pye (jwp) Date: 2009-03-15 06:46
Just downloaded v2 and tried it out against Python 2.7a0 (trunk:70381M,
Mar 14 2009, 23:12:51).

output of the "nested_issue.py" script with patch:

jwp@torch[]:org/python/trunk 0% /src/build/py/bin/python ./nested_issue.py
()
[try_with_nested]
Skipping statement body
()
SUCCESS! i guess..
()
[try_with_nested_class]
Skipping statement body
()
SUCCESS! i guess..
()
[try_with_nested_statements]
()
SUCCESS! i guess..
()
[try_with_nested_statements_class]
()
SUCCESS! i guess..

I'm going to play with it a bit more, but it looks pretty solid already...
msg83623 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-15 07:20
To see what is wrong with v2, try something like:

  class Bizarre():
    def __getattr__(self, attr):
        raise SkipStatement

  with Bizarre():
    print "Not gonna happen, but no exception either!"
msg83626 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-15 12:29
Version 3 of patch attached
- now functionally complete with the correct semantics
- compiler package not updated yet
- documentation not updated yet

Unfortunately, getting the semantics right involves moving
context.__enter__ and context.__exit__ to temporary variables instead of
being able to leave them on the stack - that makes for something of a
speed hit.
msg83627 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-15 12:31
Updated micro benchmark to also give numbers for direct try/finally
execution
msg83628 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-15 12:36
I'm definitely not happy with the effect the current patch has on with
statement execution speed - I think I'll try to come up with a patch
that adds a new SETUP_WITH opcode and see how that performs.
msg83658 - (view) Author: James William Pye (jwp) Date: 2009-03-16 02:34
I tested the attached script against v2.

It further identifies consistencies between nested with-statements and
nested() that should exist(and do in v2). It answers the question: what
is the effect of a SkipStatement exception on another, outer CM?

with nested(outer(), trap(), fail()):
 ...

That is, validate that outer()'s __exit__ invocation is not given an
exception.

This is probably an obvious effect of the patch, but I think it merited
a test.
msg83924 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-03-21 08:58
Reclassifying as a documentation bug, since PEP 377 was rejected by Guido.
msg95324 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-11-16 07:00
Lowering priority due to implementation of explicit syntax for multiple
context managers in a single with statement
msg119516 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-10-24 14:01
R.I.P contextlib.nested (it is gone in 3.2 following the deprecation in 3.1). The issue is obscure enough that I don't see much value in updating the documentation for the versions that still contain it in deprecated form.
msg120497 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-11-05 14:27
Reopening as a reminder to myself that I have a new PEP I want to write in this area. The idea is essentially a lighter weight alternative to PEP 377 that adds an optional __entered__ method to the context management protocol along the following lines:

_v = cm.__enter__()
try:
  if hasattr(cm, "__entered__"):
    VAL = cm.__entered__(_v)
  else:
    VAL = _v
  # do stuff
finally:
  cm.__exit__(*exception_status)

Providing a second, optional method that is executed *inside* the body will let CMs do things they can't do now (like skip the body of the with statement) without significantly affecting the behaviour of normal CMs. Notably, GeneratorContextManager will be able to use this to more gracefully handle the case where the generator doesn't yield a value.

I plan to flesh this out into a python-ideas post (and likely a subsequent PEP) at some point in the next few months.
msg160139 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-05-07 11:40
Superseded by issue 13585 (which will add an improved dynamic context management API)
History
Date User Action Args
2012-05-07 11:40:43ncoghlansetstatus: open -> closed
superseder: Add contextlib.ExitStack
resolution: postponed -> out of date
messages: + msg160139
2010-11-05 14:27:53ncoghlansetstatus: closed -> open
versions: + Python 3.3, - Python 3.1, Python 2.7, Python 3.2
messages: + msg120497

components: + Interpreter Core, - Documentation
resolution: out of date -> postponed
2010-10-24 14:01:25ncoghlansetstatus: open -> closed
resolution: out of date
messages: + msg119516
2010-08-04 23:47:53terry.reedysetversions: - Python 2.6, Python 2.5
2010-04-28 15:08:05ncoghlansetversions: + Python 3.2, - Python 3.0
2009-11-16 07:00:26ncoghlansetpriority: low

messages: + msg95324
2009-03-21 08:58:50ncoghlansetmessages: + msg83924
components: + Documentation, - None
2009-03-16 02:34:35jwpsetfiles: + outer_sees_no_exc.py

messages: + msg83658
2009-03-15 12:57:03ncoghlansetfiles: - pep377_bench.py
2009-03-15 12:36:57ncoghlansetmessages: + msg83628
2009-03-15 12:31:43ncoghlansetfiles: + pep377_bench.py

messages: + msg83627
2009-03-15 12:29:54ncoghlansetfiles: + pep377_v3.diff

messages: + msg83626
2009-03-15 07:20:21ncoghlansetmessages: + msg83623
2009-03-15 06:46:45jwpsetmessages: + msg83621
2009-03-15 04:04:44ncoghlansetmessages: + msg83617
2009-03-15 03:04:54ncoghlansetfiles: + pep377_v2.diff

messages: + msg83616
2009-03-15 02:50:31ncoghlansetmessages: + msg83615
2009-03-15 02:49:28ncoghlansetfiles: - pep377_v1.diff
2009-03-14 15:08:15ncoghlansetfiles: + pep377_v1.diff
keywords: + patch
messages: + msg83588
2009-03-08 05:44:40ncoghlansetfiles: + pep377_bench.py
messages: + msg83304
2009-03-08 04:14:58ncoghlansetmessages: + msg83297
2009-02-27 23:56:42ncoghlansetmessages: + msg82886
2009-02-25 11:37:01ncoghlansetmessages: + msg82701
2009-02-17 01:57:24benjamin.petersonsetassignee: ncoghlan
nosy: + ncoghlan
2009-02-13 18:09:29jwpcreate