_BaseCommandManager: callback proxy matching is broken

[KirillMysnik]

_BaseCommandManager: callback proxy matching is broken

The bug

The bug is discovered by @Hackmastr in his Simple Teleport plugin. The following code:

@SayCommand('cmd1')
@SayCommand('cmd2')
def callback(*args):
    pass

(or a similar chain decoration of TypedSayCommands) will never get the callback unregistered on plugin unload. Client commands are also affected, but not server commands (server command callbacks are not proxied).

As a side effect, after reloading the plugin the callback will never get called.

Intro

Currently _BaseCommandManager uses callback proxies (like commands.auth._AuthCallback) if such proxy defined on its subclass.

It calls the proxy class with original callback and registers the result with the add_callback. The result is also appended to _callback_proxies list:

    def register_commands(self, names, callback, *args, **kwargs):
        names = self._prepare_command_names(names)
        if self._callback_proxy is not None:
            callback = self._callback_proxy(callback, *args, **kwargs)
            self._callback_proxies.append(callback)

        # Register all command names
        for name in names:
            self._register_command(name, callback, *args, **kwargs)

    def _register_command(self, name, callback, *args, **kwargs):
        self._get_command(name).add_callback(callback)

The proxified callback is stored to _callback_proxies so that when the callback comes to unregister, the command manager will be able to obtain the associated proxy by going through the stored proxies. It uses _get_command_proxy to get the proxy by its callback:

    def _get_command_proxy(self, callback):
        for proxy in self._callback_proxies:
            if proxy.callback == callback:
                return proxy

        raise ValueError('Unable to find a proxy for the given callback.')

Proxies are meant to store the original callback in .callback property.

Why it's broken

If we use chain decoration, the original callback will be stored in multiple proxies. It means we can't determine the proxy only using the callback anymore, because we might find a proxy that was registered for a different command (but still has our callback in .callback).

What's funny, this is exactly what happens. SayCommand decorators are initialized from top to bottom (that means they will be auto-unloaded in this order), but they're applied from bottom to top.
In the sample code above cmd2 proxy will get registered first, but when it comes to unregistering, it goes after cmd1. This is where _get_command_proxy screws up.

_BaseCommandManager will try to remove_callback from cmd1 command with a cmd2 proxy and vice versa. That's not good.

Possible solution

    def __init__(self):
        self._callback_proxies = {}  # Not a list anymore

    def _get_command_proxy(self, name, callback):
        for proxy in self._callback_proxies.get(name, ()):
            if proxy.callback == callback:
                return proxy

        raise ValueError('Unable to find a proxy for the given callback.')

    def register_commands(self, names, callback, *args, **kwargs):
        names = self._prepare_command_names(names)

        if self._callback_proxy is None:
            for name in names:
                self._register_command(name, callback, *args, **kwargs)

        else:
            proxy = self._callback_proxy(callback, *args, **kwargs)

            for name in names:
                if name not in self._callback_proxies:
                    self._callback_proxies[name] = []

                self._callback_proxies[name].append(proxy)
                self._register_command(name, proxy, *args, **kwargs)

    def _register_command(self, name, callback, *args, **kwargs):
        self._get_command(name).add_callback(callback)

    def unregister_commands(self, names, callback):
        names = self._prepare_command_names(names)

        if self._callback_proxy is None:
            for name in names:
                self._unregister_command(name, callback)

        else:
            for name in names:
                proxy = self._get_command_proxy(name, callback)
                self._callback_proxies[name].remove(proxy)

                if not self._callback_proxies[name]:
                    del self._callback_proxies[name]

                self._unregister_command(name, proxy)

    # Since we have _register_command, why not have _unregister_command?
    # I like to keep things symmetric, even if _unregister_command isn't
    # overriden by any subclass. Yet.
    def _unregister_command(self, name, callback):
        self._get_command(name).remove_callback(callback)

Any other ideas?

[Ayuto]

I'm fine with the proposed solution. Please go ahead and make the changes. Same for _unregister_command. Not sure why we even left it out in the first place.