Issue843455
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2003-11-17 03:18 by tim.peters, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
weakref_gc.txt | tim.peters, 2003-11-17 03:18 | make gc and callbacks play nice together | ||
clear_first.txt | tim.peters, 2003-11-18 05:52 | Version 2: clear weakref callbacks first | ||
clear_first2.txt | tim.peters, 2003-11-18 18:16 | Version 3: second version of clear_first | ||
clear_first3.txt | tim.peters, 2003-11-18 19:35 | Version 4: third version of clear_first |
Messages (11) | |||
---|---|---|---|
msg44895 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-17 03:18 | |
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. |
|||
msg44896 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-17 07:38 | |
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. |
|||
msg44897 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-18 05:52 | |
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. |
|||
msg44898 - (view) | Author: Neil Schemenauer (nascheme) * | Date: 2003-11-18 16:26 | |
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. |
|||
msg44899 - (view) | Author: Neil Schemenauer (nascheme) * | Date: 2003-11-18 17:09 | |
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. |
|||
msg44900 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-18 17:41 | |
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. |
|||
msg44901 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-18 18:16 | |
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. |
|||
msg44902 - (view) | Author: Neil Schemenauer (nascheme) * | Date: 2003-11-18 19:04 | |
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. |
|||
msg44903 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-18 19:35 | |
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. |
|||
msg44904 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-18 20:56 | |
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. |
|||
msg44905 - (view) | Author: Tim Peters (tim.peters) * | Date: 2003-11-20 21:27 | |
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. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:56:01 | admin | set | github: 39559 |
2003-11-17 03:18:18 | tim.peters | create |