bpo-30306: release arguments of contextmanager

[tecki]

The arguments of a function which was declared as a contextmanager
are stored inside the context manager, and thus are kept alive.

This is a possible unnecessary memory leak.

This fixes the problem and adds a test for it. Look at the test to see an example of the problem.

https://bugs.python.org/issue30306

[mention-bot]

@tecki, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @berkerpeksag and @Yhg1s to be potential reviewers.

[rhettinger]

Does this change cause problems for recreate_cm?

    def _recreate_cm(self):
        # _GCM instances are one-shot context managers, so the
        # CM must be recreated each time a decorated function is
        # called
        return self.__class__(self.func, self.args, self.kwds)

[tecki]

it shouldn't make problems for recreation. At the line you are showing the context manager has not yet been called, so the arguments have not yet been deleted. And once the context manager is actually __enter__d it cannot be recreated anymore (it's his "father" that gets recreated).

At least the tests still run, and there are tests for recreating context managers.

[ncoghlan]

Nice catch, and it makes me pleased I decided exposing _recreate_cm as a public API would be a bad idea (otherwise it wouldn't really be feasible to change the current behaviour).

To address Raymond's concern, I think a more self-explanatory way of implementing the change would be to explicitly not save the arguments in the cases where we know _recreate_cm isn't going to be called on the instance (i.e. those created by the self.__class__ call in _recreate_cm).

Suggested setup for that aspect:

• Add a allow_recreation=True keyword-only parameter to GeneratorContextManagerBase.__init__
• Make setting self.[func, args, kwds] conditional on that parameter being set (otherwise set them to None)
• Add a allow_recreation=False argument to the self.__class__ call in _recreate_cm

And then make clearing the settings in __enter__ conditional on self.func != None, with an explanatory note that when the instance is used directly as a context manager, we don't need to keep the arguments for _recreate_cm around, so we release them ASAP.

(I'll add some more background and discussion about this on https://bugs.python.org/issue30306 so we have a record of the discussion)

[ncoghlan]

The request for changes here relates mainly to the structural changes described above, but there's also a minor typo in the new test's helper function name.

[ncoghlan]

s/whohoo/woohoo/ (Yes, it's just a silly name for a throwaway function, but we may as well spell it correctly and consistently)

[tecki]

I looked at the code more carefully and realized that it is unnecessarily complex. The recreation of the context manager is actually not necessary at all. All we need is to recreate the generator. This can be much more easily done directly in __enter__.

One might notice that I prefixed the variables in _GeneratorContextBase with double underscores. This is to show the world that their use is really confined to just those 25 lines of class code.

[ncoghlan]

The generator is created eagerly so that argument/parameter mismatches get reported at the right spot (i.e. when the context manager is created, not when it first gets used). Please don't change that (but feel free to add a comment explaining why it is written that way).

[tecki]

I changed the implementation to create the generator eagerly as proposed, and added test for it.

I think the proposed implementation is pretty clear by now.

[ncoghlan]

While I haven't tested it, I'm pretty sure the revised implementation of the ContextDecorator support is going to have problems when applied to a recursive function - without an entire new inner CM being created for each call, the exit invocations won't be properly independent.

It isn't possible to provide that independence implicitly in __enter__, as the interpreter has already saved its reference to the bound __exit__ method by the time that runs.

If I'm right, that does suggest there's a missing test case for that scenario, though - the current test case just covers reuse without covering recursion.

[tecki]

So I undid my additional changes, going back to my originally proposed two lines, plus tests added for all the cases that @ncoghlan mentioned.

[ncoghlan]

Branch editing is disabled, so I'll skip the NEWS entry for now, and add it in a follow-up commit.

[ncoghlan]

NEWS entry PR: #5374