stefano-m / lua-dbus_proxy

Simple API around GLib's GIO:GDBusProxy built on top of lgi
https://stefano-m.github.io/lua-dbus_proxy/
Apache License 2.0
17 stars 7 forks source link

Support for asynchronous method calls #6

Closed frenzox closed 3 years ago

frenzox commented 4 years ago

Hi,

I see that currently generated methods are called using the g_dbus_proxy_call_sync function. This is fine for non intensive tasks, but when methods can take longer to return, using the synchronous call can be an issue.

Would it be possible to generate an asynchronous version of the proxy methods as well?

stefano-m commented 3 years ago

Hi! And thanks for your interest in this little library.

Your request is not unreasonable, but it would be a brand new feature. It would be great if you could give some concrete example where the limitations of the synchronous calls come out.

If I agreed to pull the feature in, would you be available to contribute code and tests?

Thanks!

frenzox commented 3 years ago

Hi @stefano-m, thanks for your reponse.

The issue I have is due to the fact that one of the applications I'm currently working on uses lua_dbus-proxy to make method calls on other applications, but it's also a service on DBus. One of the other applications I'm interfacing with abstracts some devices connected via UART and it can take some time to return the device's response. If I use the synchronous method calls, my main loop is blocked and I cannot answer other method calls on my own interface while waiting for the response. Of course the other application could be changed to return the GDBusMethodInvocation right away and emit a signal when it's done, but that's an anti-pattern (see Using Signals, Properties and Errors section on https://dbus.freedesktop.org/doc/dbus-api-design.html).

Similar issue would occur if my application used websockets for instance, being blocked could prevent the application from sending a pong frame, causing a disconnection.

I can definitely work on a PR if you agree on this feature.

Thank you!

stefano-m commented 3 years ago

Hi,

thanks for the explanation, I guess it makes sense to add the async call feature.

Before you go off implementing it, can you please stub out the API?

It'd be nice to document how one could use the async calls within lua as an end user.

Thank you! Stefano

frenzox commented 3 years ago

Sure, I was thinking of having the Async suffix appended to the method names, the call would look like:

proxy:SomeMethodAsync(callback, context, ...)

The callback signature should follow something like:

local function some_method_cb(proxy, context, result, error)

So:

local p = require("dbus_proxy")
local some_proxy = p.Proxy:new(
                      {
                        bus = p.Bus.SESSION,
                        name = "com.example.Some",
                        interface = "com.example.SomeInterface",
                        path = "/com/example/someObjectPath"
                      }
                    )

local function some_method_cb(proxy, context, result, error)
  if error ~= nil then
    print(error.message)
    return
  end

  print("Got response from call id: " .. context)
  print("out_args[1] = " .. result[1] .. ", out_args[2] = " .. result[2])
  print("x is " .. proxy.x)
end

local call_id = 0
local in_arg = "dbus method argument"
some_proxy:SomeMethodAsync(some_method_cb, call_id, in_arg)
-- Do something else in the meantime

What do you think? Should I write it somewhere else?

Thanks!

stefano-m commented 3 years ago

Sounds good to me, although I'm not convinced about call_id: why would one need it?

Regarding the signature of the callback function, what does context look like? AFAIU it should be callback_fn(proxy_obj, result, error). Am I missing something?

Also, we should be able to call with all parameters one need, right?

proxy_obj:MethodAsync(callback_fn, arg1, arg2, ...)

I guess you'd need to wrap g_dbus_proxy_call()[1], and g_dbus_proxy_call_finish()[2]?

I am in two minds about whether generating async methods by default or giving that option to the caller of Proxy:new(), probably the former approach is the simpler, but it can be sorted out later.

Most of all, I care about having good test coverage (note: the last CI build was from a while ago, so I hope that in the meantime TravisCI has remained stable enough)

Thanks again!

[1] https://developer.gnome.org/gio/stable/GDBusProxy.html#g-dbus-proxy-call [2] https://developer.gnome.org/gio/stable/GDBusProxy.html#g-dbus-proxy-call-finish Stefano

frenzox commented 3 years ago

Hi @stefano-m,

Sounds good to me, although I'm not convinced about call_id: why would one need it?

This is just an example of what can be passed as context. This is also called user_data in some places inside glib for instance. A common use case is: let's say I make multiple calls on the same proxy and method, but with different arguments. The callback will be called also multiple times and the user needs some context to know which callback relates to which call. Another use case is when it's required to act upon an object when we get the response, but the object is not normally in the scope of the callback. If you look at the signature of the g_dbus_proxy_call function and also it's callback type GAsyncReadyCallback, you'll find the user_data there. The problem is that lgi doesn't support it (see https://github.com/pavouk/lgi/blob/master/docs/guide.md#22-callbacks). Luckily we can work around lgi limitation by using lua closures and provide this feature to lua-dbus_proxy users.

Regarding the signature of the callback function, what does context look like? AFAIU it should be callback_fn(proxy_obj, result, error). Am I missing something?

I believe the above answers this as well.

Also, we should be able to call with all parameters one need, right?

proxy_obj:MethodAsync(callback_fn, arg1, arg2, ...)

That's correct. Sorry if it wasn't clear from the signature I added, the ellipsis (varargs) should take care of that.

I guess you'd need to wrap g_dbus_proxy_call()[1], and g_dbus_proxy_call_finish()[2]?

Yes, these are the functions from the GDBus API to work async method calls indeed!

I am in two minds about whether generating async methods by default or giving that option to the caller of Proxy:new(), probably the former approach is the simpler, but it can be sorted out later.

I'm personally in favor of the former as well.

Most of all, I care about having good test coverage (note: the last CI build was from a while ago, so I hope that in the meantime TravisCI has remained stable enough)

Totally understand, I'll take care of that.

Thanks again!

Thank you ;)

Should I proceed with the implementation?

stefano-m commented 3 years ago

Hi! And thanks for the thorough explanation.

All looks good to me then.

If possible, I would ask to keep your implementation in a separate file as much as possible: I would expect you to be the maintainer for that part of code, and that would also help with attribution and copyright.

Thanks,

Stefano

stefano-m commented 3 years ago

Thanks for your contribution!

Integrated in https://github.com/stefano-m/lua-dbus_proxy/releases/tag/v0.10.2