[Patches] [ python-Patches-843455 ] Make weakref callbacks play
nice in gc
SourceForge.net
noreply at sourceforge.net
Tue Nov 18 14:04:29 EST 2003
Patches item #843455, was opened at 2003-11-17 03:18
Message generated for change (Comment added) made by nascheme
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=843455&group_id=5470
Category: Core (C code)
Group: Python 2.4
Status: Open
Resolution: None
Priority: 9
Submitted By: Tim Peters (tim_one)
Assigned to: Fred L. Drake, Jr. (fdrake)
Summary: Make weakref callbacks play nice in gc
Initial Comment:
See bug 839548. Weakref callbacks attached to objects
in dead cycles can see objects that have gone thru
tp_clear() now, and can resurrect such objects too. A
wide variety of bad things can result, up to and including
segfaults.
This implements a scheme I detailed on Python-Dev to
teach gc about the bad things callbacks can do, still
calling them, but ensuring that a callback can't reach
any objects on which tp_clear has been invoked. If a
callback happens to resurrect an object, tp_clear won't
get invoked on the latter object at all by gc. This keeps
Python-visible objects wholly sane, and even
unsurprising.
If this patch is accepted, I'll also check in a plain-text
file with a version of the Python-Dev msg explaining the
scheme.
Neal, I most want your eyeballs on this, since you know
the most about gc's design. Please assign to Fred when
you're done (whether or not you can make time),
because I want his eyeballs on the weakref changes.
Two changes were made to the weakref
implementation: (1) a new private API function
_PyWeakref_HasCallback(), so gc can determine which
objects have associated weakref callbacks; and, (2)
because objects in cyclic trash don't have refcount 0, I
had to remove the refcnt==0 check from
PyObject_ClearWeakRefs(). Maybe it would be better to
introduce a workalike private function that skipped that
one check, and leave the public function alone?
When reviewing this, note that it has to be backported
to the 2.3 line too: 2.3.2 is the release in which Jim
Fulton first saw segfaults in real life.
----------------------------------------------------------------------
>Comment By: Neil Schemenauer (nascheme)
Date: 2003-11-18 19:04
Message:
Logged In: YES
user_id=35752
Small nit: the comment "wr_callbacks mutates to contain
temporarily-immortal weakref objects abused to hold
temporarily immortal callbacks" confused me more than it
helped. I was looking around for some trashcan-like pointer
swapping. I think you mean that the weakrefs in
wr_callbacks are temporarily made immortal so that their
callbacks will not be deallocated. After the GC is done
tearing things up with tp_clear then they are made mortal again.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2003-11-18 18:16
Message:
Logged In: YES
user_id=31435
Assigned to Fred for weakref-abuse review. As Neal
suggested, there's a new private _PyWeakref_ClearRef()
function that allows most of the obscure weakref fiddling
needed by gc to live inside the weakref module. The new
patch is clear_first2.txt.
I still need to write more tests, and finish writing comments
for the tests already in the patch. I hope that none of the C
code will need to change again.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2003-11-18 17:41
Message:
Logged In: YES
user_id=31435
Na, I like that idea a lot! But maybe I didn't fully understand
what you meant, so implemented a better idea <wink>.
Assigned back to me until tests finish and I upload a new
patch. Then back to Fred. It appears that gc and weakref
internals *have* to know a lot about each other to prevent
segfaults (etc), but the rework you suggested allows most of
the obscure mucking with weakref internals to live inside the
weakref module.
----------------------------------------------------------------------
Comment By: Neil Schemenauer (nascheme)
Date: 2003-11-18 17:09
Message:
Logged In: YES
user_id=35752
Well, scratch that last idea. After trying to come up with
a patch I've concluded that it's not any cleaner. I also
tried changing the weakref tp_clear method to not decref the
wr_callback attribute. I'm not sure that's any better
either since it spreads the subtle code around.
----------------------------------------------------------------------
Comment By: Neil Schemenauer (nascheme)
Date: 2003-11-18 16:26
Message:
Logged In: YES
user_id=35752
The patch looks sound. I think it would be cleaner if the
weakref object provided a _PyWeakref_ClearRefs() function
for the GC to use (that did not mess with the callback).
That way the GC could just incref the weakref and call that
function.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2003-11-18 05:52
Message:
Logged In: YES
user_id=31435
New patch (clear_first.txt). This implements a scheme of
doing tp_clear() on trash weakrefs first, so that their
callbacks get disabled. There are subtleties (heh), most
discussed on Python-Dev already.
Eyeballs, Neal? This no longer changes anything in the
weakref implementation, but it does engage in weakref abuse,
so I'd like Fred to peer at it too.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2003-11-17 07:38
Message:
Logged In: YES
user_id=31435
Back to me: I think there's a fundamental flaw here. Despite
that it fixes the new test cases, what matters isn't really
which objects are reachable from the objects whose deaths
trigger callbacks, what matters is which objects are reachable
from the callbacks themselves. Those are the ones that must
not get tp_clear'ed before the callbacks run. In all the test
cases so far, it turned out they were also reachable from the
objects whose deaths trigger callbacks, so it seemed to
work. But it should be possible to stumble into a tougher
case.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=843455&group_id=5470
More information about the Patches
mailing list