fix bugged singleton implementation

[mobuchowski]

Singleton implementation using overloaded __new__ did not work - it always called init on instance creation.

If new() does not return an instance of cls, then the new instance’s init() method will not be invoked.

But it is an cls instance in this case.

Python 3.9.9 (main, Nov 29 2021, 15:18:55)
[Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A:
...     _instance = None
...     def __new__(cls):
...         if cls._instance is None:
...             cls._instance = super().__new__(cls)
...         return cls._instance
...
>>> class A:
...     _instance = None
...     def __new__(cls):
...         if cls._instance is None:
...             cls._instance = super().__new__(cls)
...         return cls._instance
...     def __init__(self):
...         print("init")
...
>>> A()
init
<__main__.A object at 0x1028c4a60>
>>> A()
init
<__main__.A object at 0x1028c4a60>
>>>

This changes it to metaclass-based approach that does not have this bug.

Python 3.9.9 (main, Nov 29 2021, 15:18:55)
[Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class Singleton(type):
...     """Metaclass that allows to implement singleton pattern."""
...     _instances: dict = {}
...     def __call__(cls, *args, **kwargs):
...         if cls not in cls._instances:
...             cls._instances[cls] = super().__call__(*args, **kwargs)
...         return cls._instances[cls]
...
>>> class A(metaclass=Singleton):
...     def __init__(self):
...         print("init")
...
>>> A()
init
<__main__.A object at 0x10268b0a0>
>>> A()
<__main__.A object at 0x10268b0a0>
>>>

I would prefer to have singletons implemented as modules, as in some other places, but in this case those are already classes - I think metaclass based approach it does not change their API while fixing the issue.

[potiuk]

Oh, very interesting. TIL!

looks like we had "ALMOST singletons"

[uranusjr]

It is unclear to me why a metaclass is needed. From what I can tell this can be most simply solved by calling __init__ in ProvidersManager.__new__ (and ResourceVersion doesn’t need initialisation to begin with)

[potiuk]

It is unclear to me why a metaclass is needed. From what I can tell this can be most simply solved by calling __init__ in ProvidersManager.__new__ (and ResourceVersion doesn’t need initialisation to begin with)

I think there are various approaches, I actually like the metaclass approach, it's cleaner and it also allows to avoid duplication.

[potiuk]

I also really like the explicitness of it. Previously we had to make a comment "Hey this class is singleton". now we do not have to - it's obvious from the class definition that it's singleton.

[potiuk]

And we have a test covering the expected behaviour as well.

I see only positives.

[potiuk]

Interestingly - seems that this approach has an interesting (apparently unrelated) side-effect of the www test failing. I wonder what's the reason ?

[mobuchowski]

@potiuk Trying to debug this, but this is totally foreign part of Airflow for me 🙂

[dstandish]

It is unclear to me why a metaclass is needed. From what I can tell this can be most simply solved by calling __init__ in ProvidersManager.__new__ (and ResourceVersion doesn’t need initialisation to begin with)

Can you show what your code would look like @uranusjr (i.e. code for your __new__)? i'm curious

[potiuk]

@potiuk Trying to debug this, but this is totally foreign part of Airflow for me 🙂

I am not sure I am familiar with it, but well might be time to excercise some puzzle solving skills :)

[potiuk]

Fix to the failing tests here: #32248

Very interesting edge-case. I think it has not been triggered before because effectively singleton was not really used and each entry was only accessed once. With fixing the singleton, the "invalid" entry was accessed twice during the same test and the second time it returned functools.partial object instead of None.

[potiuk]

I rebased it after merging the fix in #32248