Repository navigation
weakref reference lets go too soon #3896
Description
Activity
We could give it a try and see if it has any problems with reference cycles loading to memory leaks. I agree it shouldn't be a problem for all cases. My main concern (and maybe I'm misunderstanding--getting pretty 💤) would be cases like:
t = Table.read(...) col = t['colname'] del t
Where in the above example the idea is the user only wants to keep one column from the table and junk the rest of the table to be garbage collected. I don't even know if this currently works, but if the only reference to a table is from a Column's
_parent_refI don't think the table should hang around (instead it should run a callback to give theColumna chance to get any last bits of information it needs from theTable, then let it go...)@taldcroft -- is this actually a problem? In both cases, the object you create is never used, so isn't it fine for the link to disappear? Anytime I do create an object that stays (as in your
c12example), it seems to work:d = c.copy() d.info._parent_ref # --> <weakref at 0x7f4a52203688; to 'Column' at 0x7f4a52204438>@mhvk - perhaps I'm being too perfectionist... but this did come up for me in just messing around trying to do development work and it really confused me (esp. when it occurs in a repr and you don't get a traceback). Maybe the answer is just to catch the case where the weakref resolves to None and issue an exception that explains that they are using a temporary object and should make a permanent ref instead.
@embray - your example would indeed be problematic, thanks for the insight. I think explicitly deleting the table would be slightly unusual, but having a function that reads a table and returns one column is perfectly natural and would do the same thing.
@taldcroft - Maybe the simplest for now is simply to add a comment (or docstring??) to
_parent_ref, perhaps giving the above examples as a case where "just messing around trying to do development work" can be quite confusing!It wouldn't be the only code that breaks in the midst of debugging. I do have a few places where I put in assertions or the like to check for things like None in spots that have bitten me in debugging. (usually with a comment indicating that such a condition shouldn't occur during normal use)
I'm going to split the difference here. For the
info._parent_refI have changed this to a direct referenceinfo._parent:
taldcroft@f112b35There was a real reason, which you can see in that commit if you look at the diffs for
io/ascii/html.py. This commit backs out a workaround that I needed (storing objects in a temporary list) in order to force the reference count to stay above zero fornew_colobjects.But we will leave
parent_tableas a weakref. And with that I am closing the issue. Thanks!
The Table and Column and InfoDescriptor classes use weak references (
weakref.ref) in various places to hold a reference to a parent object. For instance a column knows the table it lives in (if it exists), and an InfoDescriptor knows the parent data object that provides information about. We started using weak references at some point because there was a problem with cyclic references not getting garbage-collected #2879.This was all good, but now I've stumbled onto a problem that seems kind of fundamental in this approach. If an object is created without a permanent reference, then it seems the parent ref is DOA. Presumably the object got garbage-collected before the
_parent_refattribute access occurred (will I'm just speculating here).This can be seen in other (more well-established) places:
I'm wondering about backing off on using weakref for all parent refs. Python is supposed to handle these cyclic references OK as long as none of the classes have a
__del__method. I can't tell from #2879 if that patch actually reduced memory use for the original problem report.