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

CC: @sagetrac-sage-combinat @mwhansen

Component: misc

Keywords: race condition, cached_method, cache

Author: Willem Jan Palenstijn

Reviewer: Tim Dumol

Merged: sage-4.3.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/5843

[nthiery]

comment:1

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]

Attachment: 5843_CachedMethodCaller.patch.gz

[wjp]

comment:2

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]

Author: Willem Palenstijn

[TimDumol]

comment:3

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

[TimDumol]

Reviewer: Tim Dumol

[wjp]

comment:4

Attachment: 5843_doctests.patch.gz

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

[TimDumol]

comment:5

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]

Attachment: trac_5843-doctests-ref.patch.gz

Adds a little bit more documentation to CachedMethod.

[TimDumol]

Attachment: trac_5843-doctests-ref.2.patch.gz

Changes the doctest to use print instead of sleep

[wjp]

comment:6

Looks good. Thanks!

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

[wjp]

Changed author from Willem Palenstijn to Willem Jan Palenstijn

[sagetrac-mvngu]

Merged: sage-4.3.2.alpha0

[sagetrac-mvngu]

comment:7

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]

comment:8

Willem, Tim: thanks so much for fixing this!

[nthiery]

comment:9

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