The arguments of a function which was decorated to be a context manager are stored inside the context manager, and are thus kept alive.

This is a memory leak.

Example:

    @contextmanager
    def f(a):
        do_something_with(a)
        a = None  # should release the memory
        yield

if this is now called with something big, say

    with f(something_really_huge):
        pass

then this something_really_huge is kept alive during the with statement, even though the function explicitly let go of it.

Nice catch. This behaviour is an artifact of the ContextDecorator support in _GeneratorContextManager, where context managers created with @contextmanager can be used as function decorators (implicitly wrapping them in a with statement) as well as directly in with statements: to handle the ContextDecorator case, the original arguments are stored on the _GeneratorContextManager instance, which has the side effect of keeping those arguments referenced for the whole of the with statement body (due to the reference to the instance from the bound exit method).

Martin's initial patch just unconditionally deletes those attributes in __enter__, which is technically correct, but *looks* wrong when reading the code (understanding why it's correct requires understanding the private _recreate_cm API used for collaboration between ContextDecorator and _GeneratorContextManager, and the fact that that relationship is a bit weird and convoluted is the main reason it's private in the first place).

My recommendation on the PR for a more self-obviously correct fix is to do the following:

• add a new "allow_recreation=True" flag parameter to _GeneratorContextManager.__init__ that controls whether or not the CM recreation attributes get set or not
• pass False for that argument in _recreate_cm so they get set to None instead
• update __enter__ to set them to None if they're not already None

The practical effect is the same as Martin's original patch, but the more explicit version should make it easier for readers to see what is going on (i.e. even when recreation is allowed during construction, the intent is that for any given instance, you will only ever call either __enter__() *or* _recreate_cm(), and once you do call __enter__(), that's it - the only permitted call after that point on that particular instance is __exit__()).

Wouldn't be better to split _GeneratorContextManager on two classes rather than add a boolean argument? Currently _GeneratorContextManager used two different functions -- an one-shot context manager and a decorator that recreates context managers.

Proposed patch makes _GeneratorContextManager an one-shot context manager again (it no longer keep references to arguments), and adds _GeneratorContextManagerDecorator which can be used either as an one-shot context manager (references to arguments are removed after use) or as a decorator (in the last case the gen attribute is not created).

You're right, there's no actual requirement that _recreate_cm() call self.__class__, so it can use a different type internally.

I personally prefer the current situation. The problem is the following: when an @contextmanager is first called, we don't know yet whether the user wants to use it directly, or as a decorator, so we have to have some kind of hybrid class. Once it's used as a decorator, we need to recreate a context manager every time the decorate function is called. Then we need exactly the direct context manager mentioned above. This is why we recreate it with __class__.

In my MR on github I simplify that by not recreating the entire context manager, but only the generator function. I think this is the most clear way, as it shows what's going on in a confined area of code.

Noting something I picked up in the PR review that apparently isn't covered by the test suite: the whole _recreate_cm() dance is necessary to make recursive and concurrent invocation of decorated functions work correctly. Otherwise the original CM instance ends up being shared between different active calls and problems start to arise.

It's these kinds of subtleties that prompted me to declare trying to make otherwise one-shot CMs usable with ContextDecorator an inherently flawed design and hence opt out of making it a public API: https://bugs.python.org/issue11647#msg161113

However, we do still need to keep it working reliably for the @contextmanager case - it's just going to be inherently messy at the implementation level because the original design decision to support the hybrid usage model turned out to be a questionable one.

Hi, so where do we go from here? I could write the proposed allow_recreation flag, but IMHO this adds only dead code, as it is of no need at all. I don't think code gets more clear by adding dead code...

I opted for one line more of comment, I think that helps much more. Plus the additional tests I wrote in the PR I think it would be good in the Python core.

Re-reading my previous comment, I now realise it wasn't particularly clear.

To be more explicit, I agree with Serhiy that since the internals of how @contextmanager works are a private API, it makes sense to switch to a two-class design that doesn't rely on the _recreate_cm() mechanism at all, and instead relies on overriding __call__() for the decorator use case.

At runtime, the end result is pretty similar to your current implementation, but the split into two classes more clearly separates the base "use a generator as a context manager" functionality from the "implicitly wrap a generator around a functional call" feature.

It makes sense to do that, since the current self.__class__ based design stems from the time when I was considering making this a public API (and hence needed to account for inheritance), and I didn't go back and review it for possible refactoring opportunities when I decided not to do that.

Ouch, this clearly slipped off my review radar last year - I just picked it up again now while going through all my currently assigned issues before 3.7b1.

While I still think the suggested refactoring above would be a good way to go, Martin's single-line fix in the PR passes all the new test cases, and solves the reported problem, so I'm going to add a NEWS entry and then merge it.

New changeset dd0e087 by Nick Coghlan (Martin Teichmann) in branch 'master':
bpo-30306: release arguments of contextmanager (GH-1500)
dd0e087

New changeset a278ad2 by Nick Coghlan in branch 'master':
bpo-30306: Add missing NEWS entry (GH-5374)
a278ad2

Thanks for the issue report and patch Martin, and sorry for the long delay in getting it merged!