Re: weakref callback vs gc vs threads
[Taking this to email. Carrying out discussions via the SF bug tracker sucks.] Comment By: Tim Peters (tim_one)
[Neil]
I had to change _PyWeakref_ClearRef() since it was also clearing the weakref list of the trash object.
That was really its *purpose*. If a trash weakref with a callback isn't removed from the referent's list of weakrefs, then the callback will trigger when PyObject_ClearWeakRefs() is invoked on the referent. The purpose of _PyWeakref_ClearRef() was to ensure that the callback never triggers.
But it's okay of the callback triggers, as long as the callback doesn't reference trash.
Now it just sets wr_object to Py_None.
That won't stop the callback from triggering. It also means (see earlier comment) that PyObject_ClearWeakRefs() will never removed the weakref from the list either, although I'm not sure that does real harm.
I'm trying to figure out PyObject_ClearWeakRefs() right now.
I also made some serious simplifications to gcmodule by just treating trash weakref objects with callbacks the same as objects with __del__ methods (i.e. move them to the finalizers list and then do the transitive closure of that set).
Does that mean they can end up in gc.garbage too? If so, I don't think that's sellable.
I think so. That can be easily changed though. What we can't do is invoke those callbacks. Neil
[Neil]
I had to change _PyWeakref_ClearRef() since it was also clearing the weakref list of the trash object.
[Tim]
That was really its *purpose*. If a trash weakref with a callback isn't removed from the referent's list of weakrefs, then the callback will trigger when PyObject_ClearWeakRefs() is invoked on the referent. The purpose of _PyWeakref_ClearRef() was to ensure that the callback never triggers.
[Neil]
But it's okay of the callback triggers, as long as the callback doesn't reference trash.
Indeed, the callback must trigger if a weakref W's referent is in CT (cyclic trash) but W is not in CT (and W has a callback, of course). If W is also in CT, it's not necessary to invoke the callback, and it can be disastrous to invoke it (that was the key to fixing the *last* pile of bugs, which were subtler than this pile). The meaning of "clearing a weakref" has become ambiguous -- so far, to me (and I believe to Fred too) it's always meant both "set wr_object to Py_None" and "remove the weakref from its referent's list of weakrefs". You're sometimes using it to mean only the former. That may be a clever thing to do -- unsure yet.
Now it just sets wr_object to Py_None.
That won't stop the callback from triggering. It also means (see earlier comment) that PyObject_ClearWeakRefs() will never remove the weakref from the list either, although I'm not sure that does real harm.
I'm trying to figure out PyObject_ClearWeakRefs() right now.
For each weakref W in the object's list of weakrefs, it clears W (in the dual sense above), and invokes W's callback (if any). If W->wr_object is Py_None, the callback is still invoked, but W isn't removed from its referent's list of weakrefs then. I don't think Fred intended that wr_object could be None at the same time wr_callback is non-NULL (Fred?), so the consequences of doing "half a clear" are only half clear <wink>.
I also made some serious simplifications to gcmodule by just treating trash weakref objects with callbacks the same as objects with __del__ methods (i.e. move them to the finalizers list and then do the transitive closure of that set).
Does that mean they can end up in gc.garbage too? If so, I don't think that's sellable.
I think so. That can be easily changed though. What we can't do is invoke those callbacks.
Some callbacks, yes, but others must be invoked. For example, in any sensible use of a weak dictionary, it's vital that callbacks on weakrefs to cyclic trash get invoked; the weakrefs themselves aren't in CT in such cases (the weak dict holds strong references to the weakrefs, so for as long as the weak dict isn't trash, the weakrefs aren't trash either). It would be great if you've found a simpler way to get this all working. The most sensitive tests for this stuff are test_gc (with the diffs from my patch on SF), test_weakref, and test_descr. But they're not verifying that things don't end up in gc.garbage, so that's something to look out for.
[Tim] ...
The most sensitive tests for this stuff are test_gc (with the diffs from my patch on SF), test_weakref, and test_descr.
BTW, use a debug build of Python. Some of the relevant tests set up scenarios so excruciatingly delicate that the only reliable symptom when they fail is triggering a C-level assert (whatever->mro != NULL is most common). In a release build, failure may or may not be visible (it usually is, but sometimes an insane object can hide its insanity temporarily in platform-specific ways).
[Tim] ...
The meaning of "clearing a weakref" has become ambiguous -- so far, to me (and I believe to Fred too) it's always meant both "set wr_object to Py_None" and "remove the weakref from its referent's list of weakrefs". You're sometimes using it to mean only the former. That may be a clever thing to do -- unsure yet.
It has one downside that seems rare (the full Zope3 test suite provokes it, but not Python's): say a weakref W "is cleared" just by setting wr_object to Py_None. If W's refcount later falls to 0, weakref_dealloc() won't remove W from W's referent's list of weakrefs. That's the way clear_weakref() is written, and I think it has to be that way: if clear_weakref doesn't know what the referent is, it can't find the pointer to the head of the referent's weakref list, so it can't unlink W from the list correctly (unlike gc's doubly-linked lists, weakref lists aren't circular, and don't have list heads) This leaves free'd memory *in* the referent's weakref list. So if the referent's refcount falls to 0 after that, then PyObject_ClearWeakRefs() goes nuts trying to crawl over the freed memory. In a debug build that's obvious when it happens, because _PyWeakref_GetWeakrefCount() hits a weakref object full of 0xdb bytes.
On Fri, Oct 29, 2004 at 03:59:37PM -0400, Tim Peters wrote:
if clear_weakref doesn't know what the referent is, it can't find the pointer to the head of the referent's weakref list, so it can't unlink W from the list correctly (unlike gc's doubly-linked lists, weakref lists aren't circular, and don't have list heads) This leaves free'd memory *in* the referent's weakref list. So if the referent's refcount falls to 0 after that, then PyObject_ClearWeakRefs() goes nuts trying to crawl over the freed memory.
Crap. That's got to be the problem that I've been bashing my head against for the past few hours. Okay, how about we first clearly specify what needs to happen. Afterwards we can figure how to do it while retaining binary compatibility. :-( Non-trash weakrefs to trash objects MUST have their callbacks invoked. Trash weakrefs MUST NOT have their callbacks invoked. Any weakref to a trash object MUST NOT reveal the trash (i.e. via the wr_object pointer). Does that sound correct? Neil
On Friday 29 October 2004 04:22 pm, Neil Schemenauer wrote:
Non-trash weakrefs to trash objects MUST have their callbacks invoked. Trash weakrefs MUST NOT have their callbacks invoked. Any weakref to a trash object MUST NOT reveal the trash (i.e. via the wr_object pointer).
Yes, that's right. -Fred -- Fred L. Drake, Jr. <fdrake at acm.org>
[Neil, on "half clear"ing]
Crap. That's got to be the problem that I've been bashing my head against for the past few hours.
I can believe that. It was attractive because otherwise (as my patch does) the guts of PyObject_ClearWeakRefs() get duplicated inline inside gc, spread across distinct passes.
Okay, how about we first clearly specify what needs to happen. Afterwards we can figure how to do it while retaining binary compatibility. :-(
It's a good plan.
Non-trash weakrefs to trash objects MUST have their callbacks invoked.
Yes.
Trash weakrefs MUST NOT have their callbacks invoked.
That's actually a kind of optimization. It's never *necessary* to invoke a callback from a trash weakref, and it *may* be catastrophic to do so. So the cheapest/easiest route is to avoid analyzing whether this case is safe, and never do it. IOW, "MUST NOT" is a correct approach to this case, but looser approaches could suffice.
Any weakref to a trash object MUST NOT reveal the trash (i.e. via the wr_object pointer).
Yes. Other subtleties follow from those. Chief example: if you decide not to invoke a callback in a trash weakref to a trash object, that can't be accomplished correctly by decref'ing wr_callback then setting wr_callback to NULL. The problem is that wr_callback may itself be weakly referenced by a different weakref with a different callback, so decref'ing wr_callback may cause that other callback to run right away, and that may not be safe. So if setting wr_calback to NULL is the strategy for preventing wr_callback from running, it can't be carried out safely until after all weakrefs to trash have been cleared. That's why dealing with trash weakrefs required two passes when fixing the last pile of bugs. There are also subtleties about what "trash" means, exactly, here. gc has been ignoring the worst of these. For example, if the referent of a non-trash weakref is in the unreachable set, but the referent is *also* in a cycle and has a __del__ method, the referent may well end up in gc.garbage. It's arguably wrong to trigger the callback in that case. It's also arguable that the user deserves whatever they get in that case <0.7 wink>. All that said, the only problem I have with the patch I posted is that the abuse of a weakref's `hash` member breaks weak-key (but not weak-value) dictionaries. That doesn't cause any tests to fail, but isn't acceptable (weak-key dicts don't work as intended). Since I'm not convinced there's a squeaky-clean approach just waiting to be found here, I think I'll go back to repairing that glitch now.
participants (3)
-
Fred L. Drake, Jr. -
Neil Schemenauer -
Tim Peters