Argument copied into cell still referenced by frame

[gvanrossum]

BPO	17927
Nosy	@gvanrossum, @brettcannon, @birkenfeld, @amauryfa, @mdickinson, @ncoghlan, @pitrou, @benjaminp, @phmc
Files	cellfree.diff cellfree2.diff cellfree3.diff cellfree4.diff

^{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: ```python assignee = 'https://github.com/benjaminp' closed_at = created_at = labels = ['interpreter-core', 'performance'] title = 'Argument copied into cell still referenced by frame' updated_at = user = 'https://github.com/gvanrossum' ``` bugs.python.org fields: ```python activity = actor = 'benjamin.peterson' assignee = 'benjamin.peterson' closed = True closed_date = closer = 'benjamin.peterson' components = ['Interpreter Core'] creation = creator = 'gvanrossum' dependencies = [] files = ['30166', '30176', '30185', '30188'] hgrepos = [] issue_num = 17927 keywords = ['patch'] message_count = 24.0 messages = ['188672', '188722', '188739', '188740', '188741', '188746', '188750', '188761', '188783', '188794', '188796', '188799', '188805', '188806', '188843', '188844', '188853', '188859', '189069', '189070', '189075', '189082', '189224', '189266'] nosy_count = 12.0 nosy_names = ['gvanrossum', 'brett.cannon', 'georg.brandl', 'amaury.forgeotdarc', 'mark.dickinson', 'ncoghlan', 'pitrou', 'benjamin.peterson', 'ubershmekel', 'python-dev', 'pconnell', 'isoschiz'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = 'patch review' status = 'closed' superseder = None type = 'resource usage' url = 'https://bugs.python.org/issue17927' versions = ['Python 3.3'] ```

[gvanrossum]

This came out of some investigations into Tulip reference cycles. I've only confirmed this with 3.3 and 3.4, but I suspect it's a problem in earlier versions too.

The scenario is as follows.

Consider a object with a method that ends up catching an exception and storing the exception on the object. We know that the __traceback attribute of the exception references the stack frame where the exception was caught, so there is a cycle: self -> exception -> traceback -> frame -> self. To break this cycle without disturbing the __traceback on the exception, the method sets "self = None" before it returns. (The point of breaking the cycle is that at some later point when the object is deleted the traceback can be printed by the __del__ method.)

This works beautifully... Except if the function happens to contain a nested function or a lambda that references 'self'. *Even if the function is never created* (e.g. "if 0: lambda: self"). Then setting "self = None" does not break the cycle. It's not a real leak, because gc.collect() will collect the cycle. But it's still annoying that I can't break the cycle (I don't want to break it at any other point because it would reduce the usefulness of the exception stored on the object).

After two days of investigations and thinking about it I found the cause: the presence of the lambda cause a cell to be created into which self is copied, but the original self argument is still referenced by the frame. Setting "self = None" clears the cell but doesn't affect the original self argument. (FWIW, this indicates it's not specifically about self, it's about any argument that gets copied into a cell.)

I thought I had a one-line fix (see cellfree.diff attached) but it breaks argument-less super(), which looks at the original first argument. I think I can fix super() (it must introspect the code object to find out into which cell self has been copied, if it finds it NULL), but I have to think about that more. If anyone wants to jump in and suggest an approach to that I'd appreciate it.

[gvanrossum]

Here's a new version that copies the cell into the arg slot instead of just clearing it, with matching code in super() that looks in the cell.

I'd appreciate a review from another senior core dev.

[gvanrossum]

Ok, if I don't hear from anyone soon I'm going to commit.

[amauryfa]

with a unit test maybe?

[pitrou]

+1 for a unit test!

[ncoghlan]

Guido, did you try combining your first patch (clearing the local var when populating the cell) with your second patch (by only checking for a cell when the local var is cleared rather than when it is populated)?

It seems slightly more logical to me to have a cell fallback for the "local ref is NULL" case than it does to special case "local ref is not NULL".

[gvanrossum]

I thought about that but I like this version better because the super() code does not have to know the details of how to find the cell.

On Wednesday, May 8, 2013, Nick Coghlan wrote:

Nick Coghlan added the comment:

Guido, did you try combining your first patch (clearing the local var when populating the cell) with your second patch (by only checking for a cell when the local var is cleared rather than when it is populated)?

It seems slightly more logical to me to have a cell fallback for the "local ref is NULL" case than it does to special case "local ref is not NULL".

----------

---

Python tracker \<report@bugs.python.org \<javascript:;>> \http://bugs.python.org/issue17927\

---

[ncoghlan]

Ah, I misread the second patch, I think due to the "copy the cell into" in the comment. I believe I would have grasped it immediately if it said something like "reference the cell from".

[gvanrossum]

Ok, here's a version with a unittest. I've also improved the comment that set Nick off the track.

[benjaminp]

Consider the following testcase

class X:
    def meth(self):
        print(self)
        super()

def f():
    k = X()
    def g():
        return k
    return g
c = f().__closure__[0]
X.meth(c)

With patch $ ./python unboxing.py \<cell at 0x7fddacab1eb8: X object at 0x7fddac7876d8>

Without patch
$ ./python unboxing.py
<cell at 0x7f2d0a218eb8: X object at 0x7f2d09eee6d8>
Traceback (most recent call last):
  File "x.py", line 12, in <module>
    X.meth(c)
  File "x.py", line 4, in meth
    super()
TypeError: super(type, obj): obj must be an instance or subtype of type

Maybe you don't care. OTOH, perhaps it could be fixed by checking if the first argument is in fact a closure in super().

In the best world, super() would be syntax instead of a call, and we would just push the __class__ the closure and first argument in the interpreter loop.

[gvanrossum]

I see. I really don't care, it's extremely rare to see a closure object.

[gvanrossum]

cellfree4.diff. Addressed review:

• Moved test to test_scope.py
• Added @cpython_only
• Fixed comment indent (and removed tabs :-)
• Fixed missing NULL test

I decided to keep the super() call; it's likely that it's tested elsewhere but I don't want to verify that there really is a test that puts self into a cell *and* uses super().

I also decided not to bother rewriting the test using weakrefs. Let me know if you really want me to do that.

[benjaminp]

I think it looks good now. I'll probably just rewrite the test once you commit it.

[ncoghlan]

Looks good to me, too.

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset d331e14cae42 by Guido van Rossum in branch 'default': bpo-17927: Keep frame from referencing cell-ified arguments. http://hg.python.org/cpython/rev/d331e14cae42

[gvanrossum]

Ok, the deed is done. Thanks for your review, Benjamin! I've reassigned it to you so you can fix up the test if you want to.

What would you think of a backport to 3.3?

[6528f7b2-fa2b-4ae4-b68c-c2d86cd1dcd6]

Our usage of Python would certainly benefit from the backport.

We are embedding Python 3.3 in a system with requirements that lead us to disable the gc in most cases, so cycles are an obvious problem for us. Cycles created inadvertently, such as this, are the biggest problem. We would probably backport the fix anyway, but would prefer not to carry patches and instead pick up fixes via 3.3.x releases.

[gvanrossum]

Martin Morrison, can you check if my fix (cellfree4.diff) applies cleanly to 3.3? Or if nearly so, could you upload a fixed version here? (Or create a new issue and assign it to me if you can't upload to this issue.)

[6528f7b2-fa2b-4ae4-b68c-c2d86cd1dcd6]

The latest diff (cellfree4.diff) applies correctly to 3.3 (one hunk fails, but it's just the one where you've removed a blank line).

The tests also pass successfully with the diff applied.

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset f6223bab5657 by Benjamin Peterson in branch 'default': when an argument is a cell, set the local copy to NULL (see bpo-17927) http://hg.python.org/cpython/rev/f6223bab5657

[gvanrossum]

Benjamin, what do you think of backporting this?

[benjaminp]

I think with the way I rewrote it, it would be okay.

[gvanrossum]

Would you mind doing the backport then?

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 2b4b289c1abb by Benjamin Peterson in branch '3.3': when arguments are cells clear the locals slot (backport of bpo-17927) http://hg.python.org/cpython/rev/2b4b289c1abb