race condition in cached_method (should not be shared between instances)

[nthiery]

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

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

[wjp]

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.

[timdumol]

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

[wjp]

Good point. I replaced the doctest patch with one that adds a more direct test for this ticket.

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

[timdumol]

Adds a little bit more documentation to CachedMethod?.

[timdumol]

Changes the doctest to use print instead of sleep

[wjp]

Looks good. Thanks!

Patches to apply, in order: 5843_CachedMethodCaller.patch 5843_doctests.patch trac_5843-doctests-ref.2.patch

[mvngu]

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.

[nthiery]

Willem, Tim: thanks so much for fixing this!

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