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.

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.

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.

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.

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.

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.

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.

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.

Logged In: YES 
user_id=31435

That's cool.  New patch is clear_first3.txt.

Comment changed to

"""
In the end, then, wr_callbacks consists of cleared weakrefs 
that are immune from collection.  Near the end of gc, after
collecting all the cyclic trash, we call release_weakrefs().
That releases our references to the cleared weakrefs, which
in turn may trigger callbacks on their callbacks.
"""

That's no more complicated than it really is <wink>.

New patch also has more comments in the new tests, and 
another new test setting up both "safe" and "unsafe" weakref 
callbacks *on* a weakref callback.

Logged In: YES 
user_id=31435

Jim Fulton told me he applied clear_first3.txt to the 2.3.2 
release, and ran Zope3 against it without problems (no 
segfaults).  Zope3 is what triggered the segfaults to begin 
with, so that's good news.

Logged In: YES 
user_id=31435

I checked this in on the head, so closing it.  I'll backport it to 
2.3 maint.

Fred, I think the changes to the weakref module were simple 
and harmless.  But if you still want to review them, you're 
more than welcome to!  Note that the new file

Modules/gc_weakref.txt

contains an account of the problem and how it was cured.