ccxtechnologies / adbus

D-Bus Binding​ for Python that supports coroutines (asyncio)
MIT License
32 stars 5 forks source link

Call process in async task when event loop starts #64

Closed laped83 closed 4 years ago

laped83 commented 4 years ago

Fixes getting timeout on first call on a dbus activatible service #61 .

Calls process() on dbus service when the event loop is started.

Signed-off-by: Lars Pedersen laa@kamstrup.com

charleseidsness commented 4 years ago

I like the concept but we may be able to improve on the implementation a little. Does the branch bugfix-systemd-startup also resolve this issue?

laped83 commented 4 years ago

Tried the bugfix-systemd-startup branch and it looks likes it is working, but the sleep 0.5 seconds is shown when using the service. A busctl call fluctuates between ~0 ms to 500 ms because of the sleep. I don't know if this is desired. We a basically calling process every 500ms even if nothing is to be processed.

charleseidsness commented 4 years ago

Right, I was under the impression that sd_bus_process would block for some reason. I'll make some updates.

charleseidsness commented 4 years ago

What doesn't make sense to me about this issue is that it seems like all it takes is another call to sd_bus_process to resolve it.

The call to sd_bus_get_unique_name should block until it's connected to the bus, which also calls sd_bus_process. If you just add another sd_bus_process call after sd_bus_get_unique_name does that also resolve the issue? It seems like there is data there to process but it isn't being processed for some reason.

charleseidsness commented 4 years ago

We could maybe so something like bugfix-systemd-startuo-wait but I'm confused as to why (assuming it does resolve it).

This is essentially the same as what you have, but I just pulled it into the cython module.

laped83 commented 4 years ago

I played a little with an idea on the bugfix-systemd-startup branch (inspired by http://0pointer.net/blog/the-new-sd-bus-api-of-systemd.html, which is more or less the documentation :D) Instead of the asyncio.sleep I use sd_bus_wait, but it doesn't work all the time for some reason. Maybe you can find the issue :D I' try the bugfix-systemd-startuo-wait branch out.

diff --git a/adbus/sdbus/service.pxi b/adbus/sdbus/service.pxi
index 4cc1c16..049a85a 100644
--- a/adbus/sdbus/service.pxi
+++ b/adbus/sdbus/service.pxi
@@ -70,6 +70,11 @@ cdef class Service:
     def __dealloc__(self):
         self.bus = sdbus_h.sd_bus_unref(self.bus)

+    async def sdbus_wait(self):
+        r = sdbus_h.sd_bus_wait(self.bus, -1)
+        if r < 0:
+            raise BusError(f"D-Bus Wait error: {errorcode[-r]}")
+
     async def process(self):
         """Processes all available transactions from the D-Bus.

@@ -87,7 +92,7 @@ cdef class Service:

                 elif r == 0:
                     print("adbus: No Process Operation")
-                    await asyncio.sleep(0.5)
+                    await self.sdbus_wait()

                 elif r < 0:
                     print(f"adbus: D-Bus Process Error: {errorcode[-r]}")
diff --git a/adbus/sdbus_h.pxd b/adbus/sdbus_h.pxd
index 3ee3fab..390683e 100644
--- a/adbus/sdbus_h.pxd
+++ b/adbus/sdbus_h.pxd
@@ -134,6 +134,7 @@ cdef extern from "systemd/sd-bus.h":

     int sd_bus_process(sd_bus *bus, sd_bus_message **r)
     int sd_bus_get_fd(sd_bus *bus)
+    int sd_bus_wait(sd_bus *bus, stdint.uint64_t timeout_usec)

     int sd_bus_message_new_method_return(sd_bus_message *call,
             sd_bus_message **m)
laped83 commented 4 years ago

bugfix-systemd-startuo-wait - works for me without any delay or high CPU.

Still think that my comment with the sd_bus_wait might be a better approach. It looks more like Potterering example. Using only process and wait instead of using the file descriptor (sd_bus_get_fd).

charleseidsness commented 4 years ago

The problem with sd_bus_wait is that is doesn't wait in an asyncio friendly fashion, it will block the mainloop in order to use it it would have to be run in a separate thread. I should have realized that my other approach wouldn't really work because of that but it's been a few years since I worked on it and I totally forgot.

If that branch works for you I'll create a PR and merge it. I'll close this one and the issue.

laped83 commented 4 years ago

Okay nice. Great work :D

charleseidsness commented 4 years ago

Thanks for raising this issue in the first place, and working through it with us, it's very much appreciated!

Feedback like this is what makes open source software so great!

laped83 commented 4 years ago

On a sidenote. Do you have an example to reply back to the callee in an async manner, if I have a service method that takes minutes to complete.?

I'm trying to create a dbus server where the caller can queue multiple messages that needs to be pushed by HTTP. The status code should than be replied back to the callee.

charleseidsness commented 4 years ago

If you increase all of your timeouts a method call can wait for minutes, but we would use a property for that. Where we call a method that starts something and then monitor the state of a property to get status back on long method calls.

You could also use a signal, property updates just use a signal anyways. In your case, since you could have multiple callers you would need to come up with some sort of protocol to determine who the status code is for. You can set up signal watchers that would look for specific text in a signal for instance.