Closed FFY00 closed 3 years ago
Would you be interested in a PR that addresses this issue? If so, I have an implementation, but could use some guidance on how you'd like to see a few things.
Hi, thank you! This is certainly helpful. I also have a WIP implementation, but I have been a bit busy and slow to pick up development again.
Looking at your implementation, I have some thoughts.
The implementation of the signal queue as a class variable assumes that only one server will be running at a time. This will result in some weird unexpected behavior that I am just not happy with. I would like not have any unexpected attachments between objects and servers. I think the best solution for this would be to make the backends implement a send_signal
method and register it in the object when we add it, it would support multiple servers using the same object by registering it in a dictionary.
I think the easiest path forward would be to continue my implementation and maybe pick up some pieces from yours if needed.
Sorry if this is not what you were hoping for, but as I will have to maintain this code long term I need to make sure the implementation won't come back later to bite me in the ass, at least to the best extent that I can.
Since you have shown interest, I will bump this up in my TODO list.
No problem -- the queue was one of the things I was hoping for some guidance on :-) Anything I can do to help?
Should be solved in #24
I just merged #24, I am fairly confident but please try it out and see if you can find any issue with it.
Great, thanks, I'll have a look!
Ok, this is super-useful, thank you much!
I ran into a few things adapting this to my use case, which you may or may not want to do anything about. I can hack around these no problem for my own case, so certainly don't implement anything just because I ran into it :-) Also, if you're OK with the suggested approaches below, I'm happy to do the work myself and submit a PR.
Context: I'm using dbus-objects
to mock an Avahi mDNS server, which I use to test a Python module for registering mDNS services with the system Avahi daemon. So I'm trying to replicate (parts of) the Avahi server's behaviour using dbus-objects
.
The way Avahi works is that if as a client you e.g. want to browse the network for mDNS services, you perform a method call on the Avahi Server
object to create a ServiceBrowser
object and then wait for signals from the service browser that contain the available services. The message exchange looks something like:
--> object Server, path /: method_call: ServiceBrowserNew
<-- object Server, path /: method_return: /x/y/z (the new object's path)
<-- object ServiceBrowser, path /x/y/z: signal: ItemNew, (1, 0, 'SSH', '_ssh._tcp', 'local', 0)
<-- object ServiceBrowser, path /x/y/z: signal: ItemNew, (1, 0, 'HTTP', '_http._tcp', 'local', 0)
<-- object ServiceBrowser, path /x/y/z: signal: AllForNow, ()
I'm a bit of a n00b when it comes to DBus, but I get the feeling the Avahi DBus API is a bit weird, so the following might be really specific to my case. Anyway:
The Avahi service browser ItemNew
signal has a field names that conflict with the optional arguments to the DBusSignal
constructor. Specifically interface
and name
(from: https://github.com/lathiat/avahi/blob/master/avahi-daemon/org.freedesktop.Avahi.ServiceBrowser.xml).
My work-around: just use unnamed fields. But those field names might be common, so perhaps consider using e.g. dbus_interface
and dbus_name
in the constructor to make collisions less likely. Ugly, I know...
Also, the error message is not very helpful when you do have a collision, because it's raised somewhere deep in the jeepney
serialization code. Not sure what to do about that:
emitting signal: ItemNew (1, 0, 'SSH', '_ssh._tcp', 'local', 0)
An exception ocurred when try to emit signal ItemNew (1, 0, 'SSH', '_ssh._tcp', 'local', 0): 6 entries for 5 fields
The signals sent by the Avahi objects are unicast to the specific DBus client that created the object.
Sending a unicast signal back to the client requires that the emitting object know the client address (something like ':1.23'
). AFAICT in the current implementation, there is no easy way in a server-side method handler to figure out which client called the method and thus in my case to which address the service browser object should send its signals.
Apparently unicast signals are permitted by the standard (https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-routing). If sending unicast signals is "normal", you might want to find a way to make the caller's address accessible to called methods. Possibly there are other DBus message properties (message flags? future header field types?) that are similary useful. That might make it worth while to find a more general solution that just for the sender.
Maybe allowing method handlers to request a "method context" from the server would be a solution, or having the server pass in a context in a keyword-only argument to the method handler if the object registers interest by decorating its handler with e.g. @dbus_method(context=True)
? For example:
class XYZ(dbus_objects.DBusObject):
@dbus_method(context=True):
def method_that_triggers_signal(self, *, dbus_context):
self.unicast_signal(42, dbus_destination=dbus_context.sender)
unicast_signal = dbus_objects.DBusSignal(answer=int)
Also, the emit_signal()
method doesn't provide a way to specify the destination for the signal. I think just an optional keyword argument would suffice.
I think the general thing here is that inversion of control makes it hard to control the ordering of signals except from the body of a method handler. You can have a separate thread of control emitting signals, but its hard to synchronize it with the method handler.
In my particular case the trouble is that the method return message must be sent before the signals. The client uses the path returned by the method call to know which object is sending signals and to register signal handlers for them.
Achieving this is tricky in the current implementation, because my Avahi Server
method handler only has control during the method call. After creating the ServiceBrowser
object in the method body, I can either:
have it immediately send all of its signals from its constructor before returning from the method call -- this a) results in the signals being sent before the method return, and b) isn't a general solution because in realistic cases the service browser needs to listen on the network for (10s of) seconds and I don't think you want to block in the method for that long.
spawn a new thread to the run service browser -- but then I need some way of synchronizing with the thread so it doesn't start sending signals before the method return has been sent. And I have to return out of the method handler to send the method return message, so I don't have anywhere to e.g. set an event or whatever.
Maybe a solution would be to allow objects to register a callback to be called by the server after the method return message has been sent? E.g. again using the decorator @dbus_method(on_return_sent=<callback here>)
-- not so nice, as self
is not in scope in the decorator, making it tricky for the user. Or if you think the method context idea above is OK, maybe use a method of the context class:
class XYZ(dbus_objects.DBusObject):
@dbus_method(context=True):
def do_stuff(self, *, dbus_context):
# Do method stuff, e.g. starting a thread to send signals later
# Set up a callback to trigger the thread after the method call is complete.
dbus_context.on_return_sent(self.callback)
def callback(self):
# Do stuff after method returns, e.g. allow other threads to start sending signals
pass
Thanks again for including the signals so far -- despite all my complaining above, it's made my code a lot simpler already :-)
Thank you for exploring this in-depth, it helps very much sort out these issues right now rather than at a later point, when we may have more users.
I'm a bit of a n00b when it comes to DBus, but I get the feeling the Avahi DBus API is a bit weird, so the following might be really specific to my case. Anyway:
I think there is a lot of weirdness around DBus :sweat_smile:
The Avahi service browser
ItemNew
signal has a field names that conflict with the optional arguments to theDBusSignal
constructor. Specificallyinterface
andname
(from: https://github.com/lathiat/avahi/blob/master/avahi-daemon/org.freedesktop.Avahi.ServiceBrowser.xml).
Alright, time for some API redesign!
I had forgotten about that, I think the best approach is to put the signal body in a container (list, tuple or dict).
my_signal1 = dbus_objects.object.DBusSignal((int, str), name='custom_name1')
my_signal2 = dbus_objects.object.DBusSignal([int, int, int], name='custom_name2')
my_signal3 = dbus_objects.object.DBusSignal({'param1': str, 'param2': int}, name='custom_name3')
With some variants:
my_signal = dbus_objects.object.DBusSignal(int, str)
my_signal = dbus_objects.object.DBusSignal.custom((int, str), name='custom_name')
my_signal = dbus_objects.object.DBusSignal.custom({'param1': str, 'param2': int}, name='custom_name')
my_signal = dbus_objects.object.DBusSignal(int, str)
my_signal = dbus_objects.object.DBusSignal.custom(name='custom_name')(param1=str, param2=int)
I am not very happy with the custom
name, suggestions are welcome. Maybe custom_constructor
.
Or maybe the opposite approach?
my_signal1 = dbus_objects.object.DBusSignal((int, str), name='custom_name1')
my_signal = dbus_objects.object.DBusSignal.simple(int, str)
Perhaps quick
instead of simple
? I don't know
Other possible approaches that I am not so sure about are:
my_signal1: DBusSignal[int, str] = dbus_objects.object.DBusSignal(name='custom_name1')
my_signal1: DBusSignal[int, str] = dbus_objects.object.DBusSignal(name='custom_name1', param_names=['param1', 'param2'])
my_signal1: DBusSignal[int, str] = dbus_objects.object.DBusSignal(name='custom_name1')
my_signal1: DBusSignal[
NamedParameter[int, 'param1'],
NamedParameter[str, 'param2'],
] = dbus_objects.object.DBusSignal(name='custom_name1')
This last one is what I was considering doing to name method return arguments, it would require #1.
Also, the
emit_signal()
method doesn't provide a way to specify the destination for the signal. I think just an optional keyword argument would suffice.
Yes, I think we should support this. It would also run into the same issue as above.
I am not so sure about the best approach here. Changing the destination would be fairly uncommon IMO, I want to optimize for most users.
self.my_signal((10, 'test'))
self.my_signal((10, 'test'), destination=...)
self.my_signal([10, 'test'])
self.my_signal([10, 'test'], destination=...)
self.my_signal({'param1': 10, 'param2': 'test'})
self.my_signal({'param1': 10, 'param2': 'test'}, destination=...)
self.my_signal()(10, 'test')
self.my_signal(destination=...)(10, 'test')
self.my_signal()(param1=10, param2='test')
self.my_signal(destination=...)(param1=10, param2='test')
self.my_signal(10, 'test')
self.my_signal.to_destination(...)(10, 'test')
self.my_signal(param1=10, param2='test')
self.my_signal.to_destination(...)(param1=10, param2='test')
self.my_signal(10, 'test')
self.my_signal.to_destination(..., body=(10, 'test'))
self.my_signal(param1=10, param2='test')
self.my_signal.to_destination(..., body={'param1'=10, 'param2'='test'})
I am leading towards the 3rd or 4th as they are the prettiest, but I am not sure.
I think the general thing here is that inversion of control makes it hard to control the ordering of signals except from the body of a method handler. You can have a separate thread of control emitting signals, but its hard to synchronize it with the method handler.
In my particular case the trouble is that the method return message must be sent before the signals. The client uses the path returned by the method call to know which object is sending signals and to register signal handlers for them.
Okay, we can use a generator for this, I feel is better than registering a callback.
@dbus_objects.object.dbus_method()
def my_normal_method() -> int:
# do something...
return 10
@dbus_objects.object.dbus_method()
def my_special_method() -> ContextReturn[int]:
# do something...
yield 10 # the message will be sent here
# do something after the message... eg. send a signal
Here if you want to send a signal to a specific destination, maybe we should allow fetching the destination in the constructor?
@dbus_objects.object.dbus_method()
def my_normal_method(desatination: DBusDestination):
...
You'd have to annotate it with a custom type, DBusDestination
, so that we know what/where to pass and to ignore it from the DBus signature.
..., when we may have more users.
Eeeiuw, users :stuck_out_tongue_winking_eye:
I like:
my_signal = dbus_objects.object.DBusSignal(int, str)
my_signal = dbus_objects.object.DBusSignal.custom(name='custom_name')(param1=str, param2=int)
The simple case stays simple and if you want something more complex you have to ask for it specifically.
I am not very happy with the
custom
name, suggestions are welcome. Maybecustom_constructor
.
Hmm, not much inspiration I'm afraid... You could just call it create
-- not really specific, but it does fairly clearly indicate a non-constructor way to create an instance.
Or you could lift the creation function out of DBusSignal
, like:
my_signal = dbus_objects.object.signal_for(name='foo', interface='bar')(arg1=int, arg2=str)
my_signal = dbus_objects.object.custom_signal(name='foo', interface='bar')(arg1=int, arg2=str)
my_signal = dbus_objects.object.create_signal(name='foo', interface='bar')(arg1=int, arg2=str)
Other possible approaches that I am not so sure about are:
my_signal1: DBusSignal[int, str] = dbus_objects.object.DBusSignal(name='custom_name1') (snip)
These seem the most complete, but also the most clunky :-(
I am not so sure about the best approach here. Changing the destination would be fairly uncommon IMO, I want to optimize for most users.
Agreed.
I am leading towards the 3rd or 4th as they are the prettiest, but I am not sure.
Yeah, 3 & 4 both look good to me too.
It might be worthwhile to have the descriptor ask its owner for a destination when __get__()
is called. That would make it more transparent (if also more magically implicit). The descriptor could just query an attribute of its owner, e.g. dest = getattr(obj, 'dbus_signal_destination', None)
. If "reserving" a fixed name is too much of a downside, the same mechanism as above for interface
and name
might work to specify the attribute/method name from which to retrieve the destination.
For my case, it would let me pass the calling connection name to the constructor when I create a a ServiceBrowser
object, and have the signals 'automatically' sent to the right destination from then on. For example:
class Server(DbusObject):
""" Avahi Server class """
def __init__(self, server: BlockingDBusServer):
self.server = server
@dbus_method()
def service_browser_new(self, *args, caller: DBusDestination) -> str:
path = '<something unique>'
obj = ServiceBrowser(*args, creator = caller)
self.server.register_object(path, obj)
return path
class ServiceBrower(DbusObject):
def __init__(self, *args, creator: DBusDestination):
self.dbus_signal_destination = creator
item_new = DBusSignal(...)
# or: item_new = DBusSignal.custom(destination='dbus_signal_destination')(...)
... meanwhile, in DBusSignal.emit_signal_callback() ...
def emit_signal_callback(self, owner: Any) -> Callable[[Any], None]:
dst = getattr(owner, 'dbus_signal_destination', None)
if not isinstance(dst, DBusDestination):
dst = None
def emit_signal(*args: Any) -> None:
# ...
callback(self, body=args, dst=dst)
Here if you want to send a signal to a specific destination, maybe we should allow fetching the destination in the constructor?
You'd have to annotate it with a custom type,
DBusDestination
, so that we know what/where to pass and to ignore it from the DBus signature.
Yeah, that looks good. I'd call it DBusCallerName
or DBusUniqueConnectionName
or something, because it's really the unique connection name of the client calling the method.
Okay, we can use a generator for this, I feel is better than registering a callback.
Cool, that works for me!
Okay, I am struggling a bit trying to come up a pythonic API for this. Some ideas, option 5 particularly stand out: