#5843 closed defect (fixed)
race condition in cached_method (should not be shared between instances)
Reported by: | nthiery | Owned by: | mhansen |
---|---|---|---|
Priority: | critical | Milestone: | sage-4.3.2 |
Component: | misc | Keywords: | race condition, cached_method, cache |
Cc: | sage-combinat, mhansen | Merged in: | sage-4.3.2.alpha0 |
Authors: | Willem Jan Palenstijn | Reviewers: | Tim Dumol |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Consider the following class (simplified from a real life example, after 3 hours of heisenbug debugging):
class bla: def __init__(self, value): self.value = value # @cached_method def f(self, x): return self.value
The method f ignores its input, and should return self.value:
sage: x = bla(1) sage: y = bla(2) sage: x.f(None) 1 sage: y.f(None) 2
Then, y.f(x.f) should ignore the inner x.f and return 2. It does not:
sage: sage: y.f(x.f) 1
The reason is that x.f and y.f, and all other instances of bla share the same cached_method object.
sage: x.f is y.f True sage: x.f is x.__class__.f True
and the _instance field is set to the latest instance for which this method has been queried:
sage: yf = y.f sage: yf._instance is y True sage: x.f Cached version of <function f at 0xb532d84> sage: yf._instance is y False sage: yf._instance is x True
Most of the time things work well, but there can be race conditions, as in the example above.
Nicolas and Florent
Attachments (4)
Change History (13)
comment:1 Changed 8 years ago by
Changed 7 years ago by
comment:2 Changed 7 years ago by
- Report Upstream set to N/A
- Status changed from new to needs_review
I added a patch that returns a (newly created) CachedMethodCaller
object from CachedMethod.__get__
that stores the instance on which the CachedMethod
was called.
Additionally this object has a __get__
of its own that does the same to handle stored copies of CachedMethodCaller
s, which is something that categories seem to do.
Using a function/closure would also handle the caching (and I added a comment in the patch on how to do it exactly), but you would lose the ability to call methods like is_in_cache()
which are used in some places in sage.
comment:3 Changed 7 years ago by
- Reviewers set to Tim Dumol
- Status changed from needs_review to needs_work
Excellent patch (I couldn't figure out how to fix this, glad you did), but there doesn't seem to be a test for the main problem:
class bla: def __init__(self, value): self.value = value # @cached_method def f(self, x): return self.value
The method f ignores its input, and should return self.value:
sage: x = bla(1) sage: y = bla(2) sage: x.f(None) 1 sage: y.f(None) 2
Then, y.f(x.f) should ignore the inner x.f and return 2. It does not:
sage: sage: y.f(x.f) 1
Changed 7 years ago by
comment:4 Changed 7 years ago by
- Status changed from needs_work to needs_review
Good point. I replaced the doctest patch with one that adds a more direct test for this ticket.
comment:5 Changed 7 years ago by
Nice. I'm giving this a positive review, but I've attached a little patch that transfers some of the documentation from the __init__
method to the the class itself, and adds some more documentation. It would be great for you to review it.
comment:6 Changed 7 years ago by
- Status changed from needs_review to positive_review
Looks good. Thanks!
Patches to apply, in order: 5843_CachedMethodCaller.patch 5843_doctests.patch trac_5843-doctests-ref.2.patch
comment:7 Changed 7 years ago by
- Merged in set to sage-4.3.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Merged patches in this order:
- 5843_CachedMethodCaller.patch
- 5843_doctests.patch
- trac_5843-doctests-ref.2.patch --- timdumol: please remember to put your username in your patch. I have committed this patch in your name.
comment:8 Changed 7 years ago by
Willem, Tim: thanks so much for fixing this!
comment:9 Changed 7 years ago by
Btw: #383 plays similar trick. Maybe Sage (actually Python) should have a standard tool for method wrappers as there already is for functions (see functools.wrapper).
Possible correct implementation: