[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan  added the comment:

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

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan  added the comment:


New changeset a278ad2faa76ae61569a58f36e06ba8745f4a9b7 by Nick Coghlan in 
branch 'master':
bpo-30306: Add missing NEWS entry (GH-5374)
https://github.com/python/cpython/commit/a278ad2faa76ae61569a58f36e06ba8745f4a9b7


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Change by Nick Coghlan :


--
pull_requests: +5215
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan  added the comment:


New changeset dd0e087edc8f1e4d2c0913236b1a62a77d9db6d8 by Nick Coghlan (Martin 
Teichmann) in branch 'master':
bpo-30306: release arguments of contextmanager (GH-1500)
https://github.com/python/cpython/commit/dd0e087edc8f1e4d2c0913236b1a62a77d9db6d8


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Martin Teichmann]

Martin Teichmann added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Martin Teichmann]

Martin Teichmann added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--
keywords: +patch
nosy: +serhiy.storchaka
Added file: http://bugs.python.org/file46852/refactor-contextmanager.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Martin Teichmann]

Changes by Martin Teichmann :


--
pull_requests: +1602

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Raymond Hettinger]

Changes by Raymond Hettinger :


--
assignee:  -> ncoghlan
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30306] release arguments of contextmanager

[Martin Teichmann]

New submission from Martin Teichmann:

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.

--
components: Library (Lib)
messages: 293234
nosy: Martin.Teichmann
priority: normal
severity: normal
status: open
title: release arguments of contextmanager
type: resource usage
versions: Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com