Don't install callbacks on values of TripleDict, MonoDict
|Reported by:||nbruin||Owned by:||tbd|
|Cc:||SimonKing, jpflori||Merged in:||sage-5.10.beta0|
|Authors:||Simon King||Reviewers:||Nils Bruin|
|Report Upstream:||N/A||Work issues:|
Description (last modified by )
_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 #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)
Change History (65)
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 4 years ago by
comment:13 Changed 4 years ago by
- Component changed from PLEASE CHANGE to memleak
- Reviewers set to Nils Bruin
- Type changed from PLEASE CHANGE to defect
comment:20 Changed 4 years ago by
- Status changed from needs_review to needs_work
- Work issues set to Make part the coverage script happy
comment:27 in reply to: ↑ 26 Changed 4 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues Make part the coverage script happy deleted