All commit authors signed the Contributor License Agreement.

I'm not an expert atweakref's, but is this even the right thing to use here? Wouldn't the closure we're caching hold a reference toobj alive?

def_method(*args,**kwargs):ifnotargs:raiseTypeError(f'{funcname} requires at least ''1 positional argument')returndispatch(args[0].__class__).__get__(obj,cls)(*args,**kwargs)

Wouldn't this hold a strong reference toobj and thus prevent either the originalWeakKeyDictionary or my changed approach from cleaning upobj?

Yes! Tracing through the logic manually, I can confirm that theweakref's callback does't fireunless I break the strong reference in_method.

So there's a bit of a space leak here (I assume Python's garbage collector might be able to figure it out though?)

I just put up a different that tried to solve this problem as well, but deviates further from the original. If we settle on an approach I'll figure out how to write tests...

If the main issue is that different objects are colliding in the cache (due to equal__hash__ and__eq__ being equal), perhaps the only solution is to remove the cache altogether (e.g. revert#107148). Using theid in the cache can also cause collisions (for objects with non-overlapping life timeshttps://docs.python.org/3/library/functions.html#id)

Unless we can find a solution that is both robust and has less complexity than this patch, I'm inclined to agree with@eendebakpt — I think a clean revert would be better. It's sad, but it may be the best we can do :(

I'm coming to the same conclusion. This patch now makes this code break:

foo=Foo()dispatcher=foo.mysingledismatchmethoddelfoodispatcher(...)# fails

What if we revert#107148 and I instead contribute a test case specifically for this situation? That'll help if someone wants to tackle this again. I personally wasn't worried about the performance of this feature.

Either way I'll give it another attempt tonight. Maybe there's another approach that can work.

Possibly one way to make this work would be to use theid for the key but useweakref.finalize to register a callback for objects whoseids are stored in the cache such that the objects are evicted from the cache when they're garbage collected

But yes, whether we think of a clever solution or just revert the feature, we should clearly add a regression test.

@AlexWaygood Weakref aside, just to be clear, while working on this patch I found another problem with the caching - that the closure put in the cache also holds a reference to the same obj we key the cache with. So it's circular and won't cleanup. There's a memory leak here too.

I can't think of a way to breakthat without breaking the semantics of the language. And I think that's the deeper problem that requires this to be reverted rather than fixed. I don't think there is a fix for this.

One more try: what if we change the cache to aWeakValueDictionary, cache based on theid of the object and modify the definition of_method to

        def _method(*args, **kwargs):            if not args:                raise TypeError(f'{funcname} requires at least '                                '1 positional argument')            return dispatch(args[0].__class__).__get__(obj, cls)(*args, **kwargs)                _method.__isabstractmethod__ = self.__isabstractmethod__        _method.register = self.register        update_wrapper(_method, self.func)        _method._self_ref = (obj, _method) # holds a strong reference to the value _method in the WeakValueDictionary

• The caching based onid avoids the collisions in the issue report
• As long as the user has references to the objectobj, the tuple_self_ref ensures there is a strong reference to the dictionary value, so the value is kept in theWeakValueDictionary
• Once the only references toobj are internal in the_method, the garbage collector kicks in and removes the object
• Since_obj and_method are collected at the same time, theid(obj) is a safe key to use.

It seems to work locally for me, but definitely needs a second look. I did not measure performance yet.

Before we discuss approaches, I'd like to get a test down to make sure things are working.

In#127839 I added tests for the two issues observed here. They are in commits6c6e8b8 andef3f8c9

It seems reasonable to me too.

#127839 is a better approach