#14159: Don't install callbacks on values of TripleDict, MonoDict
------------------------------+---------------------------------------------
Reporter: nbruin | Owner: tbd
Type: defect | Status: needs_review
Priority: major | Milestone: sage-5.8
Component: memleak | Resolution:
Keywords: | Work issues:
Report Upstream: N/A | Reviewers: Nils Bruin
Authors: Simon King | Merged in:
Dependencies: | Stopgaps:
------------------------------+---------------------------------------------
Old description:
> In #11521
> [http://trac.sagemath.org/sage_trac/attachment/ticket/11521/trac_11521_callback.patch
> trac_11521_callback.patch] a callback was installed on a weakref *value*
> of a !TripleDict:
> {{{
> _cache[key] = KeyedRef(H, _cache.eraser, (id(X),id(Y),id(category)))
> }}}
> That's not safe: If the value under that key gets changed while the value
> remains in memory, the callback will be executed and remove an entry that
> now has an unrelated value!
>
> So: Either prove that the value under this key will not change for the
> lifetime of H (keep in mind that cyclic references can extend the
> lifetime of an otherwise unreachable object essentially indefinitely, so
> the proof needs to include that all key components survive H, otherwise
> those ids could get reused) or provide a more selective callback (for
> instance, ensure that the value is still as expected before deleting).
>
> Another point is that the API of `_cache.eraser` isn't really published,
> so this behaviour is probably better encapsulated in a method on the dict
> itself.
>
> See [http://trac.sagemath.org/sage_trac/ticket/12313#comment:317 #12313
> comment 317] for a situation that likely implicates these callbacks
> (someone has to hold strong references to these keys in order to set the
> dict, so the absence of the keys suggests a spurious execution of this
> kind of callback)
New description:
In #11521
[http://trac.sagemath.org/sage_trac/attachment/ticket/11521/trac_11521_callback.patch
trac_11521_callback.patch] a callback was installed on a weakref *value*
of a !TripleDict:
{{{
_cache[key] = KeyedRef(H, _cache.eraser, (id(X),id(Y),id(category)))
}}}
That's not safe: If the value under that key gets changed while the value
remains in memory, the callback will be executed and remove an entry that
now has an unrelated value!
So: Either prove that the value under this key will not change for the
lifetime of H (keep in mind that cyclic references can extend the lifetime
of an otherwise unreachable object essentially indefinitely, so the proof
needs to include that all key components survive H, otherwise those ids
could get reused) or provide a more selective callback (for instance,
ensure that the value is still as expected before deleting).
'''Note:''' The patch trac_14159_safer_callback.patch follows the second
approach, so that a memory leak remains fixed.
Another point is that the API of `_cache.eraser` isn't really published,
so this behaviour is probably better encapsulated in a method on the dict
itself.
See [http://trac.sagemath.org/sage_trac/ticket/12313#comment:317 #12313
comment 317] for a situation that likely implicates these callbacks
(someone has to hold strong references to these keys in order to set the
dict, so the absence of the keys suggests a spurious execution of this
kind of callback)
__Apply__
- [attachment:trac_14159_safer_callback.patch]
--
Comment (by SimonKing):
I did not run the full tests yet, but I think the new patch can be
reviewed.
I introduce a variant of `TripleDictEraser` dedicated for use on the
values of a `TripleDict` (if we use a callback for the values of
`MonoDict` then we need to do the same). It will only erase an item if the
value is the originally stored weak reference.
The patch contains tests demonstrating that `TripleDictEraser` is unsafe
but `TripleDictEraserOnValue` is safe on values, and it contains a test
demonstrating that homsets can still be garbage collected.
Apply trac_14159_safer_callback.patch
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14159#comment:17>
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.
