Regression in Django with singledispatchmethod on models

[vodik]

Bug report

Bug description:

Also reported upstream to Django here.

The following code works on Python 3.12 and I believe #85160 is the cause.

When traversing a relationship and then calling a singledispatchmethod method registered on a model, the reference to self seems to get cached and locked to the first created model.

class Test(models.Model):
    ...

class Test2(models.Model):
    relationship = models.ForeignKey("Test", on_delete=models.CASCADE)

    @singledispatchmethod
    def bug(self, x):
        print(">", id(self))

    @bug.register
    def _(self, x: int):
        print(">", id(self))

and then it's pretty easy to trigger:

for t in test.test2_set.all():
    print("id:", id(t))  # always changes
    t.bug(4)             # only ever prints a single id

CPython versions tested on:

3.13

Operating systems tested on:

macOS

Linked PRs

• gh-127750: Improve caching in singledispatchmethod #127751
• gh-127750: Fix singledispatchmethod caching #127839
• gh-127750: Fix singledispatchmethod caching (v2) #128648
• gh-127750: Fix and optimize functools.singledispatchmethod() #130008
• [3.13] gh-127750: Fix functools.singledispatchmethod()  #130029
• gh-127750: Improve repr of functools.singledispatchmethod #130220
• gh-127750: Restore inspect and pydoc support of singledispatchmethod #130309
• [3.13] gh-127750: Backport some tests for singledispatchmethod (GH-130309) #130340
• [3.12] gh-127750: Backport some tests for singledispatchmethod (GH-130309) (GH-130340) #130341

[vodik]

I see what the issue is now: https://github.com/AlexWaygood/cpython/blob/3d8d4ba5f5367f2d87488b00afd55f75401c2b7d/Lib/functools.py#L950

Since dictionaries use __hash__/__eq__ for determining if a key is present, we can have collisions. Django hashs models based on the primary key. Here's a way to reproduce the same bug with just the stdlib:

from functools import singledispatchmethod
from dataclasses import dataclass

@dataclass(frozen=True)  # or (unsafe_hash=True)
class Test:
    pk: int

    @singledispatchmethod
    def broken(self, x):
        raise NotImplemenetedError

    @broken.register
    def _(self, x: int):
        return id(self)


t1 = Test(1)
print(id(t1))

t2 = Test(1)
print(id(t2))

assert id(t1) == t1.broken(2)  # both t1 and t2 will share the same dispatcher
assert id(t2) == t2.broken(2)  # fails! 

[vodik]

I have a fix, I think. Working on a PR.

[vodik]

I'm pretty sure the code in question is also leaking memory. See comment here.

[eendebakpt]

@vodik There is indeed a behavior change between 3.12 and 3.13, so perhaps that needs to be fixed. Another example showing the issue is:

from functools import lru_cache
from dataclasses import dataclass

@dataclass(frozen=True)  # or (unsafe_hash=True)
class Test:
    pk: int

t1 = Test(1)
t2 = Test(1)  

@lru_cache
def return_id(obj):
    return id(obj)

print(return_id(t1))
print(return_id(t2))

[vodik]

I didn't think about that, but yeah, I guess yeah you'd see the same behaviour here.

Though there would be an expectation that a dictionary is involved with an lru_cache. Problem with the singledispatchmethod really is that it's hidden.

Funny this is actually documented behaviour on the WeakKeyDictionary docs¹.

Footnotes

1. https://docs.python.org/3/library/weakref.html#weakref.WeakKeyDictionary ↩

[serhiy-storchaka]

The proposed solutions #127839 and #128648 still have issues with reference loops. They also do not work for all objects (immutable or unhashable). Every solution that involves caching will have such issues.

But caching is not needed for the original issue. Creating a new function is not slow, its updating multiple attributes which will never be used is slow. #130008 proposes an alternative solution. Instead of caching, it delays request of attributes. The resulting performance of calling the singledispatchmethod method is the same.

I think that that changes should be reverted in 3.13. There may be issues with the #130008 solution (not such severe like in the current code, but people can use it in a weird way). We should not risk.

[eendebakpt]

The reference loops in #128648 might be ok from a performance point of view (but to be honest, I am unsure what kind of benchmarks we should run to prove or disprove this). The approach from @serhiy-storchaka seems better though.

I agree with reverting the changes in 3.13, I will make a PR later.