EntityDictionary registers entity deletion callback only once (for the first instance)

[KirillMysnik]

Windows, CS:GO

Snippet 1 (one line is commented out)

from listeners import OnClientActive
from players.dictionary import PlayerDictionary


class MyPlayerDictionary(PlayerDictionary):
    def on_automatically_removed(self, index):
        print("Entity (index={}) is removed".format(index))

# player_dictionary = PlayerDictionary(factory=lambda index: None)
my_player_dictionary = MyPlayerDictionary(factory=lambda index: None)


@OnClientActive
def listener_on_client_active(index):
    my_player_dictionary[index]  # Add index to the dictionary

Everything works as intented.
Client connects, and my_player_dictionary[index] is set to None.
When I disconnect,

Entity (index=1) is removed

is printed. Well, along with 15 other lines (for bots).

Snippet 2 (that line is not commented out)

from listeners import OnClientActive
from players.dictionary import PlayerDictionary


class MyPlayerDictionary(PlayerDictionary):
    def on_automatically_removed(self, index):
        print("Entity (index={}) is removed".format(index))

player_dictionary = PlayerDictionary(factory=lambda index: None)
my_player_dictionary = MyPlayerDictionary(factory=lambda index: None)


@OnClientActive
def listener_on_client_active(index):
    my_player_dictionary[index]  # Add index to the dictionary

And everything's going to hell. Client connects, and my_player_dictionary[index] is set to None.
But when I disconnect, nothing is printed.

Tests have shown that in the second snippet my_player_dictionary._on_entity_deleted is never called. Although the same method (player_dictionary._on_entity_deleted) on the first dict is called successfully.

[KirillMysnik]

Long story short, ya'll broken, EntityDictionary instances never empty (besides the very lucky one), I've been triying to figure out why the server crashes for 3+ hours and I ain't developing SPA anymore until I have some sleep :P

[KirillMysnik]

Too clarify this ticket more, the real issue (as explained by Invincible) is somewhere around there. CListenerManager considers all _on_entity_deleted methods duplicate and registers only first of them, even though those methods belong to different Python objects.

[jordanbriere]

To add more info, Python equality check is returning true because both methods are technically the same even if they carry different __self__ objects. For example, set.add has the same issue and I remember that I pointed it out somewhere in the past. The following example should illustrate it if ran as it will only print 1 element in the set while it should prints 2:

class foo():
    def bar():
        pass

a = foo()
b = foo()
c = set()

c.add(a.bar)
c.add(b.bar)

print(c)

Overall, I think we should re-work how we store callbacks in general as this is an issue that might affect other modules that relies on registered callables (hooks, commands, etc.). I think we should expose a Callbacks container class directly from the c++ side that ensure methods bound to different objects are correctly added and iterated over for every objects they have been added for.

Also, I think that in general we should not silently ignore if a callback is being registered twice and should rather raise a duplicated error; if the same function is registered more than once, the plugin is definitely doing something wrong breaking its own logic and I believe it is much more easier to debug an explicit exception than trying to figure out the weird things that might or might not occurs. And this is an issue we would have noticed way before, I guess. This was also probably the cause for the following result Satoon encountered: #181 (comment)

[Ayuto]

Overall, I think we should re-work how we store callbacks in general as this is an issue that might affect other modules that relies on registered callables (hooks, commands, etc.). I think we should expose a Callbacks container class directly from the c++ side that ensure methods bound to different objects are correctly added and iterated over for every objects they have been added for.

Isn't that the purpose of CListenerManager?

Also, I think that in general we should not silently ignore if a callback is being registered twice and should rather raise a duplicated error; if the same function is registered more than once, the plugin is definitely doing something wrong breaking its own logic and I believe it is much more easier to debug an explicit exception than trying to figure out the weird things that might or might not occurs.

I totally agree! Thus, I have added it: c857356

Though, your example works fine:

from listeners import ListenerManager

class Foo(object):
    def f(self):
        print(self)
        
a = Foo()
b = Foo()

mgr = ListenerManager()

mgr.register_listener(a.f)
mgr.register_listener(b.f)

# Prints 'self' twice
mgr.notify()

def unload():
    mgr.clear()

[jordanbriere]

If you instantiate two EntityDictionary, do you get a duplicate error?

[Ayuto]

Yeah, I just noticed that I also get the error when I change the base class from object to dict in my last post.

[Ayuto]

I just did a few more tests and to fix this issue we need to implement the following is_same function:

def is_same(f1, f2):
    try:
        self1 = f1.__self__
    except AttributeError:
        return f1 is f2

    try:
        self2 = f2.__self__
    except AttributeError:
        return f1 is f2

    return self1 is self2 and f1.__func__ is f2.__func__

class Foo(dict):
    def f(self):
        pass

    @staticmethod
    def f2():
        pass

    @classmethod
    def f3(cls):
        pass

a = Foo()
b = Foo()

def f():
    pass

def get_f2():
    def f2():
        pass
    return f2

assert is_same(a.f, a.f) == True
assert is_same(a.f, b.f) == False
assert is_same(a.f, a) == False
assert is_same(a.f, b) == False
assert is_same(a.f, Foo) == False
assert is_same(a.f, f) == False
assert is_same(a.f, get_f2()) == False
assert is_same(f, f) == True
assert is_same(f, a.f) == False
assert is_same(f, a) == False
assert is_same(a, f) == False
assert is_same(a, a.f) == False
assert is_same(get_f2(), get_f2()) == False
assert is_same(Foo.f, Foo.f) == True
assert is_same(Foo.f2, Foo.f2) == True
assert is_same(Foo.f2, a.f2) == True
assert is_same(a.f2, Foo.f2) == True
assert is_same(a.f3, a.f3) == True
assert is_same(Foo.f3, a.f3) == True