#12313: Fix yet another memory leak caused by caching of coercion data
---------------------------------------------------------------------------+
Reporter: SimonKing |
Owner:
Type: defect |
Status: needs_review
Priority: major |
Milestone: sage-5.8
Component: memleak |
Resolution:
Keywords: coercion weak dictionary |
Work issues:
Report Upstream: N/A |
Reviewers: Simon King, Jean-Pierre Flori, John Perry, Nils Bruin
Authors: Simon King, Jean-Pierre Flori |
Merged in:
Dependencies: #715, #11521, #12215, #13746, #13378; merge with #13387 |
Stopgaps:
---------------------------------------------------------------------------+
Comment (by nbruin):
'''Conjectures on how the bug in [comment:317] can occur'''
I had a bit of trouble coming up with a scenario where a key triple could
get deleted during its setting. The reason is that when someone is setting
a key, presumably they are holding (strong) references to the key
components, so those can't possibly be slated for deletion.
However, the very statement introduces by #11521 changes that:
- A value in a TripleDict may be stored via a weak reference (this was
already the case)
- It installs a callback on the weakref that removes the
keytriple:weakref pair from the dictionary (this IS new).
However, since actual deallocation of this weakreffed value may be
delayed, it is very well possible that by the time the callback gets
executed, the value stored under the key triple is no longer the original
value. Thus, deletion is unwarranted! Therefore, the callback on this
weakreffed value should check that the key component is as expected AND
that the stored value is still as expected (a dead weakref would do, but
checking that it's the exact weakref that we expect there would be even
better). Otherwise, we may be erasing entries that don't have to do with
the just deallocated object!
My guess is that #14084 might have something to do with that. Are there
ever situations where the homset cached for two objects gets changed
(category refinement comes to mind as one case where this might happen
...).? That could well lead to the scenario sketched above.
In short:
[http://trac.sagemath.org/sage_trac/attachment/ticket/11521/trac_11521_callback.patch
trac_11521_callback.patch] doesn't seem entirely safe to me, because the
callback installed on the value only checks the key for deletion. It
doesn't check that the value hasn't changed in the mean time.
We still need #13387 anyway, because ANY GC event in the lookup loop can
have disastrous consequences which, like any memory corruption, could go
unnoticed for a long time and then lead to undefined behaviour later on.
The `PyTuple_New` in the currently generated code means we cannot rule
this out.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/12313#comment:320>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
