Tim's problem can be reproduced in debug mode as follows (on Windows as well as on Linux): import test.test_weakref import test.test_xmllib Boom! The debugger (on Windows) shows that it does in some GC code. After backing out Fred's last change to _weakref.c, this works as expected and I get no other problems. So I propose to back out that change and be done with it. Here's the CVS comment: ---------------------------- revision 1.8 date: 2001/03/22 18:05:30; author: fdrake; state: Exp; lines: +1 -1 Inform the cycle-detector that the a weakref object no longer needs to be tracked as soon as it is clear; this can decrease the number of roots for the cycle detector sooner rather than later in applications which hold on to weak references beyond the time of the invalidation. ---------------------------- And the diff, to be backed out: *** _weakref.c 2001/02/27 18:36:56 1.7 --- _weakref.c 2001/03/22 18:05:30 1.8 *************** *** 59,64 **** --- 59,65 ---- if (self->wr_object != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); + PyObject_GC_Fini((PyObject *)self); if (*list == self) *list = self->wr_next; self->wr_object = Py_None; *************** *** 78,84 **** weakref_dealloc(PyWeakReference *self) { clear_weakref(self); - PyObject_GC_Fini((PyObject *)self); self->wr_next = free_list; free_list = self; } --- 79,84 ---- Fred, can you explain what the intention of this code was? It's not impossible that the bug is actually in the debug mode macros, but I'd rather not ship code that's instable in debug mode -- that defeats the purpose. --Guido van Rossum (home page: http://www.python.org/~guido/)
[Guido]
It's not impossible that the bug is actually in the debug mode macros, but I'd rather not ship code that's instable in debug mode -- that defeats the purpose.
I *suspect* the difference wrt debug mode is right where it's blowing up: static void gc_list_remove(PyGC_Head *node) { node->gc_prev->gc_next = node->gc_next; node->gc_next->gc_prev = node->gc_prev; #ifdef Py_DEBUG node->gc_prev = NULL; node->gc_next = NULL; #endif } That is, in debug mode, the prev and next fields are nulled out, but not in release mode. Whenever this thing dies, the node passed in has prev and next fields that *are* nulled out. Since under MS debug mode, freed memory is set to a very distinctive non-null bit pattern, this tells me that-- most likely --some single node is getting passed to gc_list_remove *twice*. I bet that's happening in release mode too ... hang on a second ... yup! If I remove the #ifdef above, then the pair test_weakref test_xmllib dies with a null-pointer error here under the release build too. and-that-ain't-good-ly y'rs - tim
More info on the debug-mode test_weakref test_xmllib blowup in gc_list_append, and with the .pyc files already there. While running test_weakref, we call collect() once. Ditto while running test_xmllib: that's when it blows up. collect_generations() is here (***): else { generation = 0; collections0++; if (generation0.gc_next != &generation0) { *** n = collect(&generation0, &generation1); } } collect() is here: gc_list_init(&reachable); move_roots(young, &reachable); *** move_root_reachable(&reachable); move_root_reachable is here: *** (void) traverse(op, (visitproc)visit_reachable, (void *)reachable); And that's really calling dict_traverse, which is iterating over the dict. At blowup time, the dict key is of PyString_Type, with value "ref3", and so presumably left over from test_weakref. The dict value is of PyWeakProxy_Type, has a refcount of 2, and has wr_object pointing to Py_NoneStruct wr_callback NULL hash 0xffffffff wr_prev NULL wr_next NULL It's dying while calling visit() (really visit_reachable) on the latter. Inside visit_reachable, we have: if (gc && gc->gc_refs != GC_MOVED) { and that's interesting too, because gc->gc_refs is 0xcdcdcdcd, which is the MS debug-mode "clean landfill" value: freshly malloc'ed memory is filled with 0xcd bytes (so gc->gc_refs is uninitialized trash). My conclusion: it's really hosed. Take it away, Neil <wink>!
Tim Peters writes:
That is, in debug mode, the prev and next fields are nulled out, but not in release mode.
Whenever this thing dies, the node passed in has prev and next fields that *are* nulled out. Since under MS debug mode, freed memory is set to a very distinctive non-null bit pattern, this tells me that-- most likely --some single node is getting passed to gc_list_remove *twice*.
I bet that's happening in release mode too ... hang on a second ... yup! If I remove the #ifdef above, then the pair test_weakref test_xmllib dies with a null-pointer error here under the release build too.
Ok, I've been trying to keep up with all this, and playing with some alternate patches. The change that's been identified as causing the problem was trying to remove the weak ref from the cycle detectors set of known containers as soon as the ref object was no longer a container. When this is done by the tp_clear handler may be the problem; the GC machinery is removing the object from the list, and calls gc_list_remove() assuming that the object is still in the list, but after the tp_clear handler has been called. I see a couple of options: - Document the restriction that PyObject_GC_Fini() should not be called on an object while it's tp_clear handler is active (more efficient), -or- - Remove the restriction (safer). If we take the former route, I think it is still worth removing the weakref object from the GC list as soon as it has been cleared, in order to keep the number of containers the GC machinery has to inspect at a minimum. This can be done by adding a flag to weakref.c:clear_weakref() indicating that the object's tp_clear is active. The extra flag would not be needed if we took the second option. Another possibility, if I do adjust the code to remove the weakref objects from the GC list aggressively, is to only call PyObject_GC_Init() if the weakref actually has a callback -- if there is no callback, the weakref object does not act as a container to begin with. (It is also possible that with agressive removal of the weakref object from the set of containers, it doesn't need to implement the tp_clear handler at all, in which case this gets just a little bit nicer.) -Fred -- Fred L. Drake, Jr. <fdrake at acm.org> PythonLabs at Digital Creations
On Fri, Mar 23, 2001 at 01:50:21AM -0500, Fred L. Drake, Jr. wrote:
The change that's been identified as causing the problem was trying to remove the weak ref from the cycle detectors set of known containers as soon as the ref object was no longer a container.
I'm not sure what you mean by "no longer a container". If the object defines the GC type flag the GC thinks its a container.
When this is done by the tp_clear handler may be the problem; the GC machinery is removing the object from the list, and calls gc_list_remove() assuming that the object is still in the list, but after the tp_clear handler has been called.
I believe your problems are deeper than this. If PyObject_IS_GC(op) is true and op is reachable from other objects known to the GC then op must be in the linked list. I haven't tracked down all the locations in gcmodule where this assumption is made but visit_reachable is one example. We could remove this restriction if we were willing to accept some slowdown. One way would be to add the invariant (gc_next == NULL) if the object is not in the GC list. PyObject_Init and gc_list_remove would have to set this pointer. Is it worth doing? Neil
Neil Schemenauer writes:
I'm not sure what you mean by "no longer a container". If the object defines the GC type flag the GC thinks its a container.
Given the assumptions you describe, removing the object from the list isn't sufficient to not be a container. ;-( In which case reverting the change (as Tim did) is probably the only way to do it. What I was looking for was a way to remove the weakref object from the set of containers sooner, but appearantly that isn't possible as long as the object's type is the only used to determine whether it is a container.
I believe your problems are deeper than this. If PyObject_IS_GC(op) is true and op is reachable from other objects
And this only considers the object's type; the object can't be removed from the set of containers by call PyObject_GC_Fini(). (It clearly can't while tp_clear is active for that object!)
known to the GC then op must be in the linked list. I haven't tracked down all the locations in gcmodule where this assumption is made but visit_reachable is one example.
So it's illegal to call PyObject_GC_Fini() anywhere but from the destructor? Please let me know so I can make this clear in the documentation!
We could remove this restriction if we were willing to accept some slowdown. One way would be to add the invariant (gc_next == NULL) if the object is not in the GC list. PyObject_Init and gc_list_remove would have to set this pointer. Is it worth doing?
It's not at all clear that we need to remove the restriction -- documenting it would be required. -Fred -- Fred L. Drake, Jr. <fdrake at acm.org> PythonLabs at Digital Creations
On Fri, Mar 23, 2001 at 10:35:24AM -0500, Fred L. Drake, Jr. wrote:
So it's illegal to call PyObject_GC_Fini() anywhere but from the destructor? Please let me know so I can make this clear in the documentation!
No, its okay as long as the object is not reachable from other objects. When tuples are added to the tuple free-list PyObject_GC_Fini() is called. When they are removed PyObject_GC_Init() is called. This is okay because free tubles aren't reachable from anywhere else.
It's not at all clear that we need to remove the restriction -- documenting it would be required.
Yah, sorry about that. I had forgotten about that restriction. When I saw Tim's message things started to come back to me. I had to study the code a bit to remember how things worked. Neil
On Fri, Mar 23, 2001 at 12:10:33AM -0500, Tim Peters wrote:
I *suspect* the difference wrt debug mode is right where it's blowing up:
static void gc_list_remove(PyGC_Head *node) { node->gc_prev->gc_next = node->gc_next; node->gc_next->gc_prev = node->gc_prev; #ifdef Py_DEBUG node->gc_prev = NULL; node->gc_next = NULL; #endif }
PyObject_GC_Fini() should not be called twice on the same object unless there is a PyObject_GC_Init() in between. I suspect that Fred's change made this happen. When Py_DEBUG is not defined the GC will do all sorts of strange things if you do this, hence the debugging code. Neil
So I propose to back out that change and be done with it.
I just did revert the change (rev 1.8 of _weakref.c, back to 1.7), so anyone interested in pursuing the details should NOT update. There's another reason for not updating then: the problem "went away" after the next big pile of checkins, even before I reverted the change. I assume that's simply because things got jiggled enough so that we no longer hit exactly the right sequence of internal operations.
On Fri, Mar 23, 2001 at 01:19:19AM -0500, Tim Peters wrote:
There's another reason for not updating then: the problem "went away" after the next big pile of checkins, even before I reverted the change. I assume that's simply because things got jiggled enough so that we no longer hit exactly the right sequence of internal operations.
Yes. Neil
participants (4)
-
Fred L. Drake, Jr. -
Guido van Rossum -
Neil Schemenauer -
Tim Peters