Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#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)

5843_CachedMethodCaller.patch (8.1 KB) - added by wjp 8 years ago.
5843_doctests.patch (9.2 KB) - added by wjp 8 years ago.
trac_5843-doctests-ref.patch (1.7 KB) - added by timdumol 8 years ago.
Adds a little bit more documentation to CachedMethod?.
trac_5843-doctests-ref.2.patch (2.1 KB) - added by timdumol 8 years ago.
Changes the doctest to use print instead of sleep

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by nthiery

Possible correct implementation:

cached_method could return a closure instead of function object. Upon assignment to the class, this closure would become just a usual unbound method, removing the need to handle by hand the instance binding.

Changed 8 years ago by wjp

comment:2 Changed 8 years ago by wjp

  • 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 CachedMethodCallers, 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 8 years ago by timdumol

  • Authors set to Willem Palenstijn
  • 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 8 years ago by wjp

comment:4 Changed 8 years ago by wjp

  • 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 8 years ago by timdumol

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.

Changed 8 years ago by timdumol

Adds a little bit more documentation to CachedMethod?.

Changed 8 years ago by timdumol

Changes the doctest to use print instead of sleep

comment:6 Changed 8 years ago by wjp

  • Authors changed from Willem Palenstijn to Willem Jan Palenstijn
  • 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 8 years ago by mvngu

  • 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:

  1. 5843_CachedMethodCaller.patch
  2. 5843_doctests.patch
  3. 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 8 years ago by nthiery

Willem, Tim: thanks so much for fixing this!

comment:9 Changed 8 years ago by nthiery

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).

Note: See TracTickets for help on using tickets.