classification
Title: Argument copied into cell still referenced by frame
Type: resource usage Stage: patch review
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: amaury.forgeotdarc, benjamin.peterson, brett.cannon, georg.brandl, gvanrossum, isoschiz, mark.dickinson, ncoghlan, pconnell, pitrou, python-dev, ubershmekel
Priority: normal Keywords: patch

Created on 2013-05-07 16:02 by gvanrossum, last changed 2013-05-15 03:33 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
cellfree.diff gvanrossum, 2013-05-07 16:02 review
cellfree2.diff gvanrossum, 2013-05-08 15:35 review
cellfree3.diff gvanrossum, 2013-05-09 16:12 review
cellfree4.diff gvanrossum, 2013-05-10 00:22 review
Messages (24)
msg188672 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-07 16:02
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.
msg188722 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-08 15:35
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.
msg188739 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-08 22:02
Ok, if I don't hear from anyone soon I'm going to commit.
msg188740 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2013-05-08 22:27
with a unit test maybe?
msg188741 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-08 22:28
+1 for a unit test!
msg188746 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-09 03:19
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".
msg188750 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-09 05:05
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>
> _______________________________________
>
msg188761 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-09 09:58
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".
msg188783 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-09 16:12
Ok, here's a version with a unittest.  I've also improved the comment that set Nick off the track.
msg188794 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-05-09 23:22
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.
msg188796 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-09 23:36
I see. I really don't care, it's extremely rare to see a closure object.
msg188799 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-10 00:22
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.
msg188805 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-05-10 02:52
I think it looks good now. I'll probably just rewrite the test once you commit it.
msg188806 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-10 02:54
Looks good to me, too.
msg188843 - (view) Author: Roundup Robot (python-dev) Date: 2013-05-10 15:48
New changeset d331e14cae42 by Guido van Rossum in branch 'default':
#17927: Keep frame from referencing cell-ified arguments.
http://hg.python.org/cpython/rev/d331e14cae42
msg188844 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-10 15:50
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?
msg188853 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-05-10 17:35
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.
msg188859 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-10 18:15
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.)
msg189069 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-05-12 23:14
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.
msg189070 - (view) Author: Roundup Robot (python-dev) Date: 2013-05-12 23:18
New changeset f6223bab5657 by Benjamin Peterson in branch 'default':
when an argument is a cell, set the local copy to NULL (see #17927)
http://hg.python.org/cpython/rev/f6223bab5657
msg189075 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-12 23:45
Benjamin, what do you think of backporting this?
msg189082 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-05-13 00:05
I think with the way I rewrote it, it would be okay.
msg189224 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-14 14:44
Would you mind doing the backport then?
msg189266 - (view) Author: Roundup Robot (python-dev) Date: 2013-05-15 03:32
New changeset 2b4b289c1abb by Benjamin Peterson in branch '3.3':
when arguments are cells clear the locals slot (backport of #17927)
http://hg.python.org/cpython/rev/2b4b289c1abb
History
Date User Action Args
2013-05-15 03:33:07benjamin.petersonsetstatus: open -> closed
2013-05-15 03:32:49python-devsetmessages: + msg189266
2013-05-14 14:44:36gvanrossumsetmessages: + msg189224
2013-05-13 00:05:54benjamin.petersonsetmessages: + msg189082
2013-05-12 23:45:02gvanrossumsetmessages: + msg189075
2013-05-12 23:18:16python-devsetmessages: + msg189070
2013-05-12 23:14:59isoschizsetmessages: + msg189069
2013-05-10 18:15:12gvanrossumsetmessages: + msg188859
2013-05-10 17:35:28isoschizsetmessages: + msg188853
2013-05-10 15:50:15gvanrossumsetassignee: gvanrossum -> benjamin.peterson
resolution: fixed
messages: + msg188844
2013-05-10 15:48:03python-devsetnosy: + python-dev
messages: + msg188843
2013-05-10 02:54:22ncoghlansetmessages: + msg188806
2013-05-10 02:52:17benjamin.petersonsetkeywords: - needs review

messages: + msg188805
2013-05-10 00:22:44gvanrossumsetfiles: + cellfree4.diff

messages: + msg188799
2013-05-09 23:36:30gvanrossumsetmessages: + msg188796
2013-05-09 23:22:41benjamin.petersonsetmessages: + msg188794
2013-05-09 16:12:42gvanrossumsetfiles: + cellfree3.diff
keywords: + patch
messages: + msg188783
2013-05-09 09:58:38ncoghlansetmessages: + msg188761
2013-05-09 05:05:28gvanrossumsetmessages: + msg188750
2013-05-09 03:19:03ncoghlansetmessages: + msg188746
2013-05-08 22:28:43pitrousetnosy: + pitrou
messages: + msg188741
2013-05-08 22:27:10amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg188740
2013-05-08 22:02:55gvanrossumsetmessages: + msg188739
2013-05-08 16:51:12mark.dickinsonsetnosy: + mark.dickinson
2013-05-08 16:50:00pitrousetnosy: + brett.cannon, georg.brandl, ncoghlan, benjamin.peterson
2013-05-08 15:35:29gvanrossumsetstage: needs patch -> patch review
2013-05-08 15:35:14gvanrossumsetkeywords: + needs review, - patch
files: + cellfree2.diff
messages: + msg188722
2013-05-08 07:37:57pconnellsetnosy: + isoschiz
2013-05-08 07:37:36pconnellsetnosy: + pconnell
2013-05-07 18:55:59ubershmekelsetnosy: + ubershmekel
2013-05-07 16:02:23gvanrossumcreate