#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: #13387 | Stopgaps:
------------------------------+---------------------------------------------
Changes (by SimonKing):
* status: needs_work => needs_review
* work_issues: Make part the coverage script happy =>
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).
>
> '''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]
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_weak_value_triple_dict.patch]
--
Comment:
Replying to [comment:26 nbruin]:
> At the moment I'm getting a feeling we're entering the realm of
overengineering.
That's quite possible. But the changes were rather mild, and so I am
submitting my patch anyway.
Do you think it is overengineered? We should of course test if the
performance drops too much.
And we need to run the test suite (I didn't, yet).
Apply trac_14159_weak_value_triple_dict.patch
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14159#comment:27>
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.
