altdesktop / python-dbus-next

🚌 The next great DBus library for Python with asyncio support
https://python-dbus-next.readthedocs.io/en/latest/
MIT License
191 stars 60 forks source link

Match rules for signals are too broad #53

Open un-def opened 4 years ago

un-def commented 4 years ago

I recently tried to replace python-dbus with python-dbus-next in one of my projects and realized that it is not possible to register a signal handler for the specific combination of signal arguments.

For example, if one wants to get a notification when the Spotify app is closed (that is, when its well-known bus name loses an owner), they can do this using python-dbus:

bus.get_object(
    bus_name='org.freedesktop.DBus',
    object_path='/org/freedesktop/DBus',
).connect_to_signal(
    signal_name='NameOwnerChanged',
    handler_function=on_name_owner_changed,
    dbus_interface='org.freedesktop.DBus',
    arg0='org.mpris.MediaPlayer2.spotify',
    arg2='',
)

The code above does to the following method call:

org.freedesktop.DBus.AddMatch("\
type='signal',\
sender='org.freedesktop.DBus',\
path='/org/freedesktop/DBus',\
interface='org.freedesktop.DBus',\
member='NameOwnerChanged',\
arg0='org.mpris.MediaPlayer2.spotify',\
arg2=''\
")

In this case, the client will only receive signal messages which have the following signature: NameOwnerChanged('org.mpris.MediaPlayer2.spotify', <anystring>, '') (see https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-name-owner-changed), other messages will not be routed to the client at all.

I dug into the library code and found out that the current implementation uses very broad match rule: https://github.com/altdesktop/python-dbus-next/blob/6dee14cc43ca3494e9a206eeba137f81a0c0fe4e/dbus_next/proxy_object.py#L47

That is, any signal with the specified combination of a bus_name, interface, and object path will be delivered to the client regardless of whether or not there is a registered handler. If there is no registered handler, the message will be just ignored (if ... msg.member not in self._signal_handlers): https://github.com/altdesktop/python-dbus-next/blob/6dee14cc43ca3494e9a206eeba137f81a0c0fe4e/dbus_next/proxy_object.py#L76

It could be inefficient. If there are lots of signals and one is only interested in a small part of them, they will still receive all of them. Moveover, mattern matching must be implemented in the signal handler code. For example, if one registered a signal handler using python-dbus as it described in the example above, they would not have to check the name and new_onwer arguments because the check would be already done on the server side (nonmatching signals would not be sent to the client in the first place):

def on_name_owner_changed(name: str, old_owner: str, new_owner: str):
    # this check is not required
    if name != 'org.mpris.MediaPlayer2.spotify' or new_owner != '':
        return
    do_something()

However, it isn't that simple. Pattern matching (dispatching) is still required because there can be more than one signal handler for the same signal. That is, we still need to dispatch a signal message to an appropriate handler function, otherwise all handlers will be called on all signals regardless of signal arguments. The point is that pattern matching should be a part of a dispatching mechanism of the library, not be implemented ad hoc by a library user. This is how it is implemented in dbus-python:

In my opinion, it could work like this:

  1. BaseProxyInterface._signal_match_rule is not used anymore.
  2. Instead, When and only when interface.on_<signal>(fn, [arg0=..., [arg1=..., [argN=...]]]) is called, the org.freedesktop.DBus.AddMatch method is called with more specific matching rule. The rule should be constructed using <signal> as a mandatory member= field and argN arguments as optional argN= fields (if any). The rule should have a Python representation for dispatching purposes (see https://github.com/freedesktop/dbus-python/blob/dbus-python-1.2.14/dbus/connection.py#L55 for example). The Python representation of the rule and the handler should be registered in some sort of mapping or tree.
  3. When a signal is received, the corresponding handler(s) are found using registered matching rules and called.
  4. It must be possible to unregister a specific handler with a specific set of arguments.
acrisci commented 4 years ago

Yeah sounds reasonable. Are you available to work on it?

It must be possible to unregister a specific handler with a specific set of arguments.

Yeah kind of tricky. This is an edge case that needs to be worked out.

iface.off_signal(fn) # remove all handlers regardless of match rules?
iface.off_signal(fn, arg1='foo') # remove all handlers with `arg1='foo'` even if they specified an `arg2='bar'`?
un-def commented 4 years ago

Unfortunately, I'm not ready to work on it, at least in the near future.

acrisci commented 4 years ago

Ok well that workaround should work fine until someone gets around to it.