function decorated with a context manager can only be invoked once

[pitrou]

BPO	11647
Nosy	@ncoghlan, @pitrou, @giampaolo, @voidspace, @ericsnowcurrently
Files	issue_11647.diff issue_11647_2.diff issue11647_refresh_cm.diff: Patch to make the refresh_cm() method public

^{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/ncoghlan'
closed_at = <Date 2012-05-19.12:52:03.729>
created_at = <Date 2011-03-23.02:04:37.746>
labels = ['type-feature', 'library']
title = 'function decorated with a context manager can only be invoked once'
updated_at = <Date 2012-05-19.17:17:57.232>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2012-05-19.17:17:57.232>
actor = 'eric.snow'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2012-05-19.12:52:03.729>
closer = 'ncoghlan'
components = ['Library (Lib)']
creation = <Date 2011-03-23.02:04:37.746>
creator = 'pitrou'
dependencies = []
files = ['21449', '21475', '25638']
hgrepos = []
issue_num = 11647
keywords = ['patch']
message_count = 21.0
messages = ['131838', '131851', '131931', '131944', '131957', '132090', '132094', '132096', '132100', '132189', '132401', '132471', '132475', '132483', '132484', '132489', '132585', '135204', '135206', '149738', '161113']
nosy_count = 7.0
nosy_names = ['ncoghlan', 'pitrou', 'giampaolo.rodola', 'michael.foord', 'ysj.ray', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue11647'
versions = ['Python 3.3']

[pitrou]

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

[ncoghlan]

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.

[pitrou]

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

[voidspace]

The former sounds like fixing the bug, the latter like removing functionality. So yes, I prefer the former...

[ncoghlan]

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

[ysjray]

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

:)

[pitrou]

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

[ysjray]

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

[pitrou]

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.

[voidspace]

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.

[ysjray]

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. :)

[ysjray]

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.

[ncoghlan]

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.

[ysjray]

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.

[ncoghlan]

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.

[voidspace]

I agree. copy is a separate protocol and shouldn't be involved here.

[ysjray]

Got it. Here is my updated patch. Not sure if the doc is proper.

[python-dev]

New changeset e4ba097123f6 by Nick Coghlan in branch '3.2':
Issue bpo-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 bpo-11647 update from 3.2
http://hg.python.org/cpython/rev/b23d1df7e47e

[ncoghlan]

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)

[ncoghlan]

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

[ncoghlan]

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