[Patches] [ python-Patches-843455 ] Make weakref callbacks play nice in gc

[SourceForge.net]

Patches item #843455, was opened at 2003-11-16 22:18
Message generated for change (Comment added) made by tim_one
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: Tim Peters (tim_one)
Date: 2003-11-18 13: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 12: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 12: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 11: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 00: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 02: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