[ python-Bugs-1055820 ] weakref callback vs gc vs threads
SourceForge.net
noreply at sourceforge.net
Sat Oct 30 09:55:50 CEST 2004
Bugs item #1055820, was opened at 2004-10-27 21:58
Message generated for change (Comment added) made by tim_one
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1055820&group_id=5470
Category: Python Interpreter Core
Group: None
Status: Open
Resolution: None
Priority: 9
Submitted By: Tim Peters (tim_one)
Assigned to: Neil Schemenauer (nascheme)
Summary: weakref callback vs gc vs threads
Initial Comment:
Oh oh. It's that time of year again. I'm sure the
attached (temp2a.py) can be simplified, perhaps down
to two objects and one thread. As is, I *think* it
demonstrates that invoking a weakref callback can do
fatal damage, not necessarily because of what the
callback does, but because simply calling it while gc is
running can allow other threads to run during gc too,
and resurrect a piece of cyclic trash T that's already
been tp_clear()'ed, via invoking a still-living weakref to
T.
If so, this isn't new in 2.4. It's a real problem since
temp2a.py is what's left of ZODB 3.4a1 <wink>.
----------------------------------------------------------------------
>Comment By: Tim Peters (tim_one)
Date: 2004-10-30 03:55
Message:
Logged In: YES
user_id=31435
patch3.txt repairs some typos in comments; semantically, it's
the same as patch2.txt.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-29 23:23
Message:
Logged In: YES
user_id=31435
patch2.txt stops abusing weakref `hash`, and seems to have
no issues w/ Python's or Zope3's -uall/--all tests, in debug or
release builds. weakrefs are handled entirely in
move_troublemakers() now; collect() doesn't even have its
old wr_callbacks list anymore.
If there's a more elegant approach coming, great, but if it's
not coming soon I'm going to check this in, to get as much
developer test coverage as possible before 2.4.b1.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-29 13:13
Message:
Logged In: YES
user_id=31435
> 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.
> 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 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. See the end of gc_weakref.txt:
An alternative would have been to treat objects with
callbacks like objects with __del__ methods, refusing to
collect them, appending them to gc.garbage instead.
That would have been much easier. Jim Fulton gave a
strong argument against that (on Python-Dev):
...
> I'm attaching a patch. It's a work in progress.
Aren't we all <wink>.
----------------------------------------------------------------------
Comment By: Neil Schemenauer (nascheme)
Date: 2004-10-29 13:00
Message:
Logged In: YES
user_id=35752
I had to change _PyWeakref_ClearRef() since it was also
clearing the weakref list of the trash object. Now it just
sets wr_object to Py_None. 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).
I'm attaching a patch. It's a work in progress.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-29 12:41
Message:
Logged In: YES
user_id=31435
Bingo. This is a bit delicate <heh>. It's still necessary to call
_PyWeakref_ClearRef() on a trash weakref with a callback, to
prevent the callback from ever triggering (that was the key
to fixing the previous pile of bugs). For all other weakrefs to
trash, I think you're right that just setting wr_object to None
is conceptually enough. But ...
What I pissed away the last half hour discovering is that if
you set wr_object to None *before* calling
_PyWeakref_ClearRef(), then the latter doesn't do anything,
because clear_weakref() doesn't do anything when wr_object
is None.
So that leaves me a little worried: if we just set wr_object
to None on some weakrefs, then PyObject_ClearWeakrefs()
will never (and for the same reason) remove such a weakref
from its doubly-linked list either. Doesn't look like the
weakref code intended that this be possible, and I'm not yet
entirely convinced it can't hurt ...
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-29 12:01
Message:
Logged In: YES
user_id=31435
Hmm. Just about anything fixes the current rash of bugs.
It's keeping the old bugs from coming back that's been the
hangup -- don't forget to run test_weakref.py too <wink>.
The last batch of bugs was really much subtler than this
batch!
test_callback_in_cycle_1 is a bitch -- I think we have to stop
its callback from ever getting invoked, not just prevent I.wr()
from returning J ...
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-29 11:18
Message:
Logged In: YES
user_id=31435
Excellent! I think you're right about this. It never occurred
to me that just setting wr_object to None would be as
effective at disabling a weakref as calling clear_weakref(). If
that's really so (& I can't see why not offhand), it would be
better in oh-so-many ways.
----------------------------------------------------------------------
Comment By: Neil Schemenauer (nascheme)
Date: 2004-10-29 10:56
Message:
Logged In: YES
user_id=35752
I don't see why you need the extra bit. When you find
weakly referenced objects, clear the wr_object pointer of
the weakref. Move trash weakref objects with callbacks to
the wr_callbacks list as we always did. When the trash goes
away then PyObject_ClearWeakRefs() will invoke the callbacks
normally (we only cleared the wr_object pointer, the
backpointer list is still intact).
I'm going to see if I can make this work. In the process I
will probably discover what I have been missing. :-)
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-29 05:11
Message:
Logged In: YES
user_id=31435
I strongly suspect that abusing the weakref struct's `hash`
member is responsible for the gazillions of KeyErrors. Dang. I
need one lousy bit ...
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-29 02:54
Message:
Logged In: YES
user_id=31435
Neil, can you make some time to eyeball this (patch.txt)? If
not, please assign to Fred. The Python -uall suite passes in
debug and release builds.
I just ran the -all Zope3 test suite in a debug buld, and that
passes as well as it does with Python 2.3.4 on Windows, but
I see many thousands of lines like:
Exception exceptions.KeyError: <weakref at 104EA620; dead>
in <function remove at 0x01595560> ignored
So there's something still wrong here. I'm not sure who's
writing those msgs; I expect it's in the guts of gc, when
invoking delayed weakref callbacks, and triggered by the
weak dictionary implementations (which haven't changed).
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-28 16:55
Message:
Logged In: YES
user_id=31435
A fix is in progress, as sketched on Python-Dev. I expect it
to land Friday (or tonight yet, if I'm lucky).
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-28 04:56
Message:
Logged In: YES
user_id=31435
temp2d.py shows that we don't need any weakrefs with
callbacks to get in trouble -- a __del__ method can hose us
too.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-28 04:06
Message:
Logged In: YES
user_id=31435
temp2c.py may be as bad as it gets. It shows that the
problem can occur on a gc collection that doesn't see *any*
object that has a weakref with a callback.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-27 22:37
Message:
Logged In: YES
user_id=31435
Noting that temp2b.py fails in current CVS, 2.3.4, and 2.2.3.
That's all the Pythons I have handy right now.
----------------------------------------------------------------------
Comment By: Tim Peters (tim_one)
Date: 2004-10-27 22:28
Message:
Logged In: YES
user_id=31435
Yup, temp2b.py probably shows the same problem, with 2
objects and one thread. This one is definitely more strained,
though, since the weakref callback does the damage
directly. In temp2a.py, nothing is trying to fool anything, and
the only damage done by the wr callbacks there is simply in
releasing the GIL (thus allowing other threads to do perfectly
ordinary things with weakrefs).
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1055820&group_id=5470
More information about the Python-bugs-list
mailing list