classification
Title: function decorated with a context manager can only be invoked once
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: eric.snow, giampaolo.rodola, michael.foord, ncoghlan, pitrou, python-dev, ysj.ray
Priority: normal Keywords: patch

Created on 2011-03-23 02:04 by pitrou, last changed 2012-05-19 17:17 by eric.snow. This issue is now closed.

Files
File name Uploaded Description Edit
issue_11647.diff ysj.ray, 2011-03-29 08:33
issue_11647_2.diff ysj.ray, 2011-03-30 13:19
issue11647_refresh_cm.diff ncoghlan, 2012-05-19 12:52 Patch to make the refresh_cm() method public review
Messages (21)
msg131838 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-23 02:04
If you add the following test to text_contextlib:

diff --git a/Lib/test/test_contextlib.py b/Lib/test/test_contextlib.py
--- a/Lib/test/test_contextlib.py
+++ b/Lib/test/test_contextlib.py
@@ -363,6 +363,8 @@ class TestContextDecorator(unittest.Test
             state.append(x)
         test('something')
         self.assertEqual(state, [1, 'something', 999])
+        test('something else')
+        self.assertEqual(state, [1, 'something', 999, 1, 'something else', 999])
 
 
 # This is needed to make the test actually run under regrtest.py!



You get the following error:

======================================================================
ERROR: test_contextmanager_as_decorator (test.test_contextlib.TestContextDecorator)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/cpython/32/Lib/contextlib.py", line 28, in __enter__
    return next(self.gen)
StopIteration

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/antoine/cpython/32/Lib/test/test_contextlib.py", line 366, in test_contextmanager_as_decorator
    test('something else')
  File "/home/antoine/cpython/32/Lib/contextlib.py", line 15, in inner
    with self:
  File "/home/antoine/cpython/32/Lib/contextlib.py", line 30, in __enter__
    raise RuntimeError("generator didn't yield")
RuntimeError: generator didn't yield


Clearly there is a problem in the API or its implementation, as in a function decorated with a context manager can only be invoked once! This isn't good...
msg131851 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-03-23 04:10
On Wed, Mar 23, 2011 at 12:04 PM, Antoine Pitrou <report@bugs.python.org> wrote:
> Clearly there is a problem in the API or its implementation, as in a function decorated with a context manager can only be invoked once! This isn't good...

The problem stems from a fundamentally bad call on my part: making
ContextGenerator inherit from ContextDecorator.

I failed to account for the fact that in order to work correctly the
latter requires "reusable" context managers that can be used more than
once, but ContextGenerator objects are one-shots (i.e. once __enter__
has been called once, you can't use them again).

We can either hack this to work by providing ContextDecorator with a
way to get the underlying context manager to create a new copy of
itself each time, or else revert to the 3.1 status quo and declare
that context managers created via contextlib.contextmanager simply
can't be used as decorators. I'm inclined to favour the latter option.
msg131931 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-23 22:18
> We can either hack this to work by providing ContextDecorator with a
> way to get the underlying context manager to create a new copy of
> itself each time, or else revert to the 3.1 status quo and declare
> that context managers created via contextlib.contextmanager simply
> can't be used as decorators. I'm inclined to favour the latter option.

I would vote for the former option :)
msg131944 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-03-23 23:39
The former sounds like fixing the bug, the latter like removing functionality. So yes, I prefer the former...
msg131957 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-03-24 05:07
> The former sounds like fixing the bug, the latter like removing functionality. So yes, I prefer the former...

The reason I'm reluctant is that it makes for a fundamental change in
behaviour between the use of an @contextmanager definition as a
context manager and as a decorator. The former would continue to be a
one-shot operation, just like opening a file and implicitly closing it
at the end. The latter (if fixed) would need to implicitly recreate
the context manager on each call to the function, creating the
illusion of the context manager being reusable. The difference in
underlying behaviour bothers me a great deal, as does the effective
creation of a "context decorator" protocol that is the moral
equivalent of the implicit context manager __with__ method that
received barely any interest when I posted it on python-ideas.

Given that it is impossible for anyone to be using this feature at
this stage (since we just added it and it doesn't actually work in
practice), I would prefer to simply acknowledge that I screwed up in
failing to completely think through the implications of adding it and
ditch it completely. If someone really wants this functionality to
work, then they can do it right and write the implicit context manager
PEP. (Note that ContextDecorator itself is fine, as long as the
context manager is reusable. It's the fact that ContextGenerator
*isn't* reusable that causes problems).
msg132090 - (view) Author: ysj.ray (ysj.ray) Date: 2011-03-25 14:17
Agreed with nick's idea, the implicitly recreation of the context managers would confuse users.

How about removing the "generator must yield exactly one value" restriction of @contextlib.contextmanage? Then if I want a generator to be used as a common context manager, I can make it yield exactly one value, else further if I want the resulting context manager can be used as a decorator, (reusable), I can put the generator code in a "while True" loop and add a "yield" at the end of loop body, like this:

@contextmanager
def func():
    while True:
        print('begin func')
        yield
        print('end func')
        yield

:)
msg132094 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-25 14:38
> Agreed with nick's idea, the implicitly recreation of the context
> managers would confuse users.

Uh, why would it? That's exactly what I expect the decorator to do, and
I was astonished to discover that it *doesn't*.
msg132096 - (view) Author: ysj.ray (ysj.ray) Date: 2011-03-25 14:50
> > Agreed with nick's idea, the implicitly recreation of the context
> > managers would confuse users.

> Uh, why would it? That's exactly what I expect the decorator to do, and
> I was astonished to discover that it *doesn't*.


Because there is no *OBVIOUS* code or sign which can illustrate that context manager changes from a one-shot to a reusable. When using in "with" statement, it's a one-shot, while using as a decorator, it becomes a reusable. I think the way using it doesn't provide enough reason for its behavior change. Is there somebody who may expected that the GeneratorContextManager IS a one-shot?
msg132100 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-25 15:24
> Because there is no *OBVIOUS* code or sign which can illustrate that
> context manager changes from a one-shot to a reusable.

I'm talking about the decorator, not the context manager. Surely there
is a way for the decorator to instantiate a new context manager each
time the decorated function is called? I may miss something, but it
doesn't sound like rocket science.
msg132189 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-03-26 00:15
I'm strongly with Antoine on this. If you use context manager as a decorator you should be able to reuse it - and in general both generators and context managers can be reused and so I don't understand the phrase 'moral equivalent of the implicit context manager'.

If having APIs that can be used as context managers *and* decorators is a useful thing (and usage in django, py.test and mock seems to show that it is) then this functionality should be provided.
msg132401 - (view) Author: ysj.ray (ysj.ray) Date: 2011-03-28 14:49
Now I found that I am wrong and I feel OK with make the context manager  reusable when used as a decorator. I missed one thing: the reusable behavior is regarded as in the decorated function but not in the decorator context manager. :)
msg132471 - (view) Author: ysj.ray (ysj.ray) Date: 2011-03-29 08:33
Here I worked out a patch to make the function decorated by context manager returned by @contextmanager reusable, by storing original function object and argments(args, kwds objects) in _GeneratorContextManager and construct a generator when needed(in self.__enter__ while used as a context manager and self.__call__ while used as a decorator). It still inherit ContextDecorator to keep class inheritation complete and the __call__ method is override.
msg132475 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-03-29 10:30
Part of the problem is that there was a specific reason we didn't go with lazy initialisation of the internal generator: lazy initialisation means there can be an arbitrary delay between creation of the context manager and the creation of the underlying generator that lets you know that you got the arguments wrong.

By creating the generator immediately, any exceptions are thrown at the point where the context manager is created rather than when it is used.

I think the cleanest way to fix this is to still create the generator automatically in __init__, but *also* save the original function and argument details. Then override __call__ to create a fresh instance of _GeneratorContextManager each time, instead of using "with self:" the way ContextDecorator does.

The ContextDecorator documentation should also be updated to point out that it only works with reusable context managers.

For 3.3, the fix could be generalised with ContextDecorator supporting a documented "self._recreate" internal interface that, by default, just returns self, but would be overridden in _GeneratorContextManager to actually create a new instance. The custom __call__ implementation for _GeneratorContextManager could then be removed.
msg132483 - (view) Author: ysj.ray (ysj.ray) Date: 2011-03-29 14:23
> For 3.3, the fix could be generalised with ContextDecorator supporting a documented "self._recreate" internal interface that, by default, just returns self, but would be overridden in _GeneratorContextManager to actually create a new instance. The custom __call__ implementation for _GeneratorContextManager could then be removed.

How about directly using the existing "copy.copy()" semantics instead of self._recreate()? That is, call "with copy.copy(self)" instead of "with self" in ContextGenerator.__call__(), implement ContextGenerator.__copy__() method to simply return self, and implement  _GeneratorContextManager.__copy__() to create a copy of itself. This saves an internal interface.
msg132484 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-03-29 15:24
Because I don't want to conflate the two APIs. ContextDecorator can be used with *any* context manager, and some of those may already be using their copy semantics for something else.

If anyone ever writes the __with__ PEP, then ContextDecorator._recreate can be updated to use that, but in the meantime an operation specific internal API is my preferred option.
msg132489 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-03-29 16:59
I agree. copy is a separate protocol and shouldn't be involved here.
msg132585 - (view) Author: ysj.ray (ysj.ray) Date: 2011-03-30 13:19
Got it. Here is my updated patch. Not sure if the doc is proper.
msg135204 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-05 14:04
New changeset e4ba097123f6 by Nick Coghlan in branch '3.2':
Issue #11647: allow contextmanager objects to be used as decorators as described in the docs. Initial patch by Ysj Ray.
http://hg.python.org/cpython/rev/e4ba097123f6

New changeset b23d1df7e47e by Nick Coghlan in branch 'default':
Merge #11647 update from 3.2
http://hg.python.org/cpython/rev/b23d1df7e47e
msg135206 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-05-05 14:09
The core bug has been fixed for 3.2.1 and forward ported to 3.3.

I credited the patch to "Ysj Ray" in ACKS and NEWS, let me know if you'd prefer a different attribution.

Leaving the issue open for now, since _recreate_cm() should be renamed and documented for 3.3 (but I haven't settled on a public name at this point - recreate_cm() isn't quite right for the public API, as reusable CMs aren't actually recreated by the method)
msg149738 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-12-18 07:25
I'm prototyping a public version of the recreation API as "ContextDecorator.refresh_cm()" in contextlib2:

http://contextlib2.readthedocs.org/en/latest/index.html#contextlib2.ContextDecorator.refresh_cm
msg161113 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-05-19 12:52
I'm closing this *without* converting ContextDecorator._recreate_cm() to a public method (although I'm also attaching the patch that would have done exactly that).

My rationale for doing so is that I *still* consider making _GeneratorContextManager a subclass of ContextDecorator a design error on my part. Converting the existing _recreate_cm() hook to a public refesh_cm() method would be actively endorsing that mistake and encouraging others to repeat it.

If anyone else wants to pursue this, create a new issue and be prepared to be very persuasive. After all, the current module maintainer just rejected his own implementation of the feature :)
History
Date User Action Args
2012-05-19 17:17:57eric.snowsetnosy: + eric.snow
2012-05-19 12:52:03ncoghlansetstatus: open -> closed
files: + issue11647_refresh_cm.diff
resolution: fixed
messages: + msg161113
2011-12-18 07:25:45ncoghlansettype: behavior -> enhancement
messages: + msg149738
2011-12-12 19:00:06giampaolo.rodolasetnosy: + giampaolo.rodola
2011-05-05 14:09:20ncoghlansetmessages: + msg135206
versions: - Python 3.2
2011-05-05 14:04:03python-devsetnosy: + python-dev
messages: + msg135204
2011-05-05 12:23:07ncoghlansetassignee: ncoghlan
versions: + Python 3.2, Python 3.3
2011-03-30 13:19:52ysj.raysetfiles: + issue_11647_2.diff

messages: + msg132585
2011-03-29 16:59:35michael.foordsetmessages: + msg132489
2011-03-29 15:24:31ncoghlansetmessages: + msg132484
2011-03-29 14:23:57ysj.raysetmessages: + msg132483
2011-03-29 10:30:48ncoghlansetmessages: + msg132475
2011-03-29 08:33:33ysj.raysetfiles: + issue_11647.diff
keywords: + patch
messages: + msg132471
2011-03-28 14:49:18ysj.raysetmessages: + msg132401
2011-03-26 00:15:21michael.foordsetmessages: + msg132189
2011-03-25 15:24:21pitrousetmessages: + msg132100
2011-03-25 14:50:11ysj.raysetmessages: + msg132096
2011-03-25 14:38:08pitrousetmessages: + msg132094
2011-03-25 14:17:02ysj.raysetnosy: + ysj.ray
messages: + msg132090
2011-03-24 05:07:32ncoghlansetmessages: + msg131957
2011-03-23 23:39:12michael.foordsetmessages: + msg131944
2011-03-23 22:18:03pitrousetmessages: + msg131931
2011-03-23 04:10:41ncoghlansetmessages: + msg131851
2011-03-23 02:04:37pitroucreate