After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 640181 - Support Pythonic gdbus method invocation
Support Pythonic gdbus method invocation
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal enhancement
: ---
Assigned To: Martin Pitt
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2011-01-21 13:50 UTC by Martin Pitt
Modified: 2011-02-19 14:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
git formatted patch (4.16 KB, patch)
2011-01-21 13:51 UTC, Martin Pitt
none Details | Review
git formatted patch (4.16 KB, patch)
2011-01-26 17:50 UTC, Martin Pitt
none Details | Review
git formatted patch (4.14 KB, patch)
2011-02-08 14:49 UTC, Martin Pitt
needs-work Details | Review
git formatted patch (7.15 KB, patch)
2011-02-08 21:39 UTC, Martin Pitt
none Details | Review
git formatted patch (7.24 KB, patch)
2011-02-08 21:53 UTC, Martin Pitt
none Details | Review
git formatted patch (9.98 KB, patch)
2011-02-14 09:02 UTC, Martin Pitt
committed Details | Review

Description Martin Pitt 2011-01-21 13:50:42 UTC
Right now, doing gdbus calls from Python is still a lot less convenient than with dbus-python, as you need to do the GVariant conversion of the arguments, know about and use the API of call_sync(), and unmarshall the result.

This patch will provide a wrapper with which method calls now look as simple as they can possibly be:

      proxy = Gio.DBusProxy.new_sync(...)
      result = proxy.MyMethod('(is)', 42, 'hello')

As discussed with Tomeu, we require specifying the input argument signature as the first argument of each method call. This ensures that the types of e. g. integers are always correct, and avoids having to do expensive D-Bus introspection for each call.

The basis for this is already in trunk (see commits 6d8ff4d5bdd, e97e2804, a060287d), this now provides the icing on the cake. :-)
Comment 1 Martin Pitt 2011-01-21 13:51:31 UTC
Created attachment 178938 [details] [review]
git formatted patch
Comment 2 Martin Pitt 2011-01-26 17:50:39 UTC
Created attachment 179384 [details] [review]
git formatted patch

This patch required a slight update after the stricter GVariant constructor change that just hit git head.
Comment 3 johnp 2011-01-26 18:38:16 UTC
Note that in dbus python the signature was always optional and at the end of the paranmeters.
Comment 4 Martin Pitt 2011-01-26 19:21:49 UTC
I discussed that extensively with Tomeu, and we both think that it'd be a lot safer to always explicitly give it. I had too many crashes because sometimes in dbus-python your numbers got converted to the wrong type, etc.

If you prefer to have the signature at the end, I can change the patch accordingly. Personally I prefer them at the start.
Comment 5 Simon McVittie 2011-01-27 13:24:20 UTC
(In reply to comment #0)
> As discussed with Tomeu, we require specifying the input argument signature as
> the first argument of each method call.

With my dbus-python maintainer hat on, I approve entirely; guessing what signature you wanted was by far the most error-prone part of dbus-python.

(python -c "import this" | grep guess :-)
Comment 6 Martin Pitt 2011-02-08 14:49:05 UTC
Created attachment 180389 [details] [review]
git formatted patch

Updated to apply to current git head, and with Python 3 fixes.
Comment 7 johnp 2011-02-08 17:19:34 UTC
Comment on attachment 180389 [details] [review]
git formatted patch

>From 38e8f1475c33af6b6bd3ad53587ea5fdb6555815 Mon Sep 17 00:00:00 2001
>From: Martin Pitt <martin.pitt@ubuntu.com>
>Date: Tue, 8 Feb 2011 15:38:21 +0100
>Subject: [PATCH] [gi] Add Pythonic gdbus method invocation
>
>Provide a wrapper for Gio.DBusProxy for calling D-Bus methods like on a normal
>Python object. This will handle the Python object <-> GVariant conversion, and
>optional keyword arguments for flags and timeout.
>
>Require specifying the input argument signature as the first argument of each
>method call. This ensures that the types of e. g. integers are always correct,
>and avoids having to do expensive D-Bus introspection for each call.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=640181
>---
> gi/overrides/Gio.py |   40 ++++++++++++++++++++++++++++++++++++++++
> tests/test_gdbus.py |   27 +++++++++++++++++++++++++++
> 2 files changed, 67 insertions(+), 0 deletions(-)
>
>diff --git a/gi/overrides/Gio.py b/gi/overrides/Gio.py
>index 78affa2..fddc236 100644
>--- a/gi/overrides/Gio.py
>+++ b/gi/overrides/Gio.py
>@@ -23,6 +23,8 @@ from ..importer import modules

You have two GLib imports
 
> from gi.repository import GLib
> 
>+from gi.repository import GLib
>+
> Gio = modules['Gio']._introspection_module
> 
> __all__ = []
>@@ -97,3 +99,41 @@ class Settings(Gio.Settings):
> 
> Settings = override(Settings)
> __all__.append('Settings')
>+
>+class _DBusProxyMethodCall:
>+    '''Helper class to implement DBusProxy method calls.'''
>+
>+    def __init__(self, dbus_proxy, method_name):
>+        self.dbus_proxy = dbus_proxy
>+        self.method_name = method_name

What happens if we support introspected keywords like we do in python-dbus in the future?  Perhaps all named args should start with '_' which is not valid in dbus so will never clash.  Also, I would really like that we support async calls out of the box with error and success callbacks.  Sync calls were always controversial because it makes programmers lazy but the fact that you can freeze your UI for up to 25 seconds by default is not nice.

>+    def __call__(self, signature, *args, **kwargs):
>+        arg_variant = GLib.Variant(signature, tuple(args))
>+        result = self.dbus_proxy.call_sync(self.method_name, arg_variant,
>+                kwargs.get('flags', 0), kwargs.get('timeout', -1), None)
>+        return result.unpack()
>+
>+class DBusProxy(Gio.DBusProxy):
>+    '''Provide comfortable and pythonic method calls.
>+    
>+    This marshalls the method arguments into a GVariant, invokes the
>+    call_sync() method on the DBusProxy object, and unmarshalls the result
>+    GVariant back into a Python tuple.
>+
>+    The first argument always needs to be the D-Bus signature tuple of the
>+    method call. Example:
>+
>+      proxy = Gio.DBusProxy.new_sync(...)
>+      result = proxy.MyMethod('(is)', 42, 'hello')
>+
>+    You can optionally specify a keyword argument "timeout=msec" to set a
>+    timeout. Without it, it uses the default D-Bus timeout.
>+
>+    You can also optionally specify a keyword argument
>+    "flags=Gio.DBusCallFlags.*".
>+    '''
>+    def __getattr__(self, name):
>+        return _DBusProxyMethodCall(self, name)
>+
>+DBusProxy = override(DBusProxy)
>+__all__.append('DBusProxy')
>diff --git a/tests/test_gdbus.py b/tests/test_gdbus.py
>index 567433c..449dab1 100644
>--- a/tests/test_gdbus.py
>+++ b/tests/test_gdbus.py
>@@ -92,3 +92,30 @@ class TestGDBusClient(unittest.TestCase):
>         self.dbus_proxy.call('UnknownMethod', None,
>                 Gio.DBusCallFlags.NO_AUTO_START, 500, None, call_done, data)
>         main_loop.run()
>+
>+    def test_python_calls_sync(self):
>+        result = self.dbus_proxy.ListNames('()')
>+        self.assertTrue(isinstance(result, tuple))
>+        self.assertEqual(len(result), 1)
>+        self.assertTrue(len(result[0]) > 1)
>+        self.assertTrue('org.freedesktop.DBus' in result[0])
>+
>+        result = self.dbus_proxy.GetNameOwner('(s)', 'org.freedesktop.DBus')
>+        self.assertTrue(isinstance(result, tuple))
>+        self.assertEqual(len(result), 1)
>+        self.assertEqual(type(result[0]), type(''))
>+
>+        # same with keyword argument; timeout=0 will fail immediately
>+        try:
>+            self.dbus_proxy.GetConnectionUnixProcessID('()', timeout=0)
>+            self.fail('call with timeout=0 should raise an exception')
>+        except Exception as e:
>+            self.assertTrue('Timeout' in str(e), str(e))
>+
>+    def test_python_calls_sync_errors(self):
>+        # error case: invalid argument types
>+        try:
>+            self.dbus_proxy.GetConnectionUnixProcessID('()')
>+            self.fail('call with invalid arguments should raise an exception')
>+        except Exception as e:
>+            self.assertTrue('InvalidArgs' in str(e), str(e))
>-- 
>1.7.2.3
>
Comment 8 Simon McVittie 2011-02-08 17:37:24 UTC
(In reply to comment #7)
> support introspected keywords like we do in python-dbus

I consider it to be a bug, not a feature, that dbus-python changes its behaviour based on Introspect(). In particular, remote services shouldn't be able to crash us by putting information in their introspection XML that conflicts with the arguments we think we're giving them.

I'd even go as far as saying that debugging tools like d-feet, and static binding tools like dbus-binding-tool, are the only valid uses for Introspect().

dbus-python couldn't fix this (or the rest of the guessing I mentioned in Comment #5) without breaking API, but this is new API anyway.

> Also, I would really like that we support async
> calls out of the box with error and success callbacks.

... or a single callback that gets a tuple or an error as appropriate? Only having one callback seems to be the conventional GIO way.
Comment 9 Martin Pitt 2011-02-08 21:39:51 UTC
Created attachment 180421 [details] [review]
git formatted patch

Thanks for the review!

I changed the patch to use keyword arguments starting with an underscore now. It looks a bit worse, and feels like using private stuff, but if it will potentially conflict with D-Bus method keyword arguments, I guess it's unavoidable. I wasn't aware that D-Bus even supported keyword arguments, that's why I didn't do that in the initial patch.

Supporting async calls sounds good as well, I extended the patch accordingly and added test cases. They look like this in the simplest case:

   def call_done(proxy, result, user_data):
      print result # normal Python array

   myproxy.ListNames('()', _result_handler=call_done)

You can pass user data with the _user_data keyword argument (see test suite). result will either be the call result as a normal Python object, or an Exception object (see test case).

How does that look to you?
Comment 10 Martin Pitt 2011-02-08 21:53:50 UTC
Created attachment 180426 [details] [review]
git formatted patch

Sorry for the thinko, the asynchronous calls should return the entire result tuple as well, not just its first element. Fixed.
Comment 11 Simon McVittie 2011-02-09 10:51:50 UTC
(In reply to comment #9)
> I wasn't aware that D-Bus even supported keyword arguments

It... doesn't?

All D-Bus method arguments are positional and non-optional; the closest thing to Pythonic keyword arguments at the D-Bus level is an argument of type a{sv} (a string-to-variant map).

Arguments can optionally have names; I think what J5 was getting at was that if (in future) you used Introspect() to discover the names, then you could use them to provide Python-like syntax which supports re-ordering the arguments. However, as I mentioned in Comment #8, I think that's a really bad idea.

The main point of keyword arguments within Python is to have optional arguments with default values, and those don't (currently) exist in D-Bus; having arguments which can be rearranged into any order but are all required seems rather useless.

To have a simple concrete example of the above, if you have this D-Bus method:

    Divide(d: Dividend, d: Divisor) -> d: Answer

you can currently do

    >>> proxy.Divide(3, 4)
    0.75

By (ab)using Introspect(), you could in principle allow this:

    >>> proxy.Divide(Divisor=4, Dividend=3)
    0.75

but I don't think you should, and I think obscuring useful things like timeout and result_handler for the hypothetical ability to do this in future is undesirable.
Comment 12 Martin Pitt 2011-02-09 10:59:12 UTC
Simon, thanks for confirming. That's what I thought indeed, and why I didn't hesitate to add keyword arguments to the original implementation. I'd love to revert those to the non _-prefixed style, if John agrees?

I also had another thought: Right now the calls _always_ return a tuple as result, even if the function only returns one value (which is the common case). In order to make them feel more like traditional Python calls, the wrapper could return the element of the result tuple if its length is 1, and the entire tuple if there are more than one. What do you guys think about that?
Comment 13 Simon McVittie 2011-02-09 11:22:58 UTC
(In reply to comment #12)
> I also had another thought: Right now the calls _always_ return a tuple as
> result, even if the function only returns one value (which is the common case).
> In order to make them feel more like traditional Python calls, the wrapper
> could return the element of the result tuple if its length is 1, and the entire
> tuple if there are more than one.

If you're going to match "native Python" behaviour, you should also map an empty result tuple to None (a bare "return" is equivalent to "return None"). I'm not sure whether it's a good idea to do this or not.

dbus-python does this automatic tuple unpacking, but this has the problem that if the remote service returns an unexpected signature, you'll get really counterintuitive results, for instance:

- call a method that you think returns a list of strings and an int ('asi')
- this version of the service is an older API which doesn't have the integer,
  only the list of strings
- you do: strings, flags = Method()
- if the list of strings happens to have length 2, you'll get:
  strings = "foo" (where you expected ["foo", "bar"])
  flags = "bar" (where you expected an integer)

In most cases, returning an unexpected signature will cause TypeError while unpacking, of course.

This might not be such a big deal with GDBusProxy, though, because you can use g_dbus_proxy_set_interface_info to make the expected signatures explicit.
Comment 14 Martin Pitt 2011-02-11 17:21:39 UTC
(In reply to comment #13)
> If you're going to match "native Python" behaviour, you should also map an
> empty result tuple to None (a bare "return" is equivalent to "return None").

Good point.

> dbus-python does this automatic tuple unpacking, but this has the problem that
> if the remote service returns an unexpected signature, you'll get really
> counterintuitive results

That's true, but I guess if you change the D-BUS API of your service, you would need to update your code for that either way. With a tuple the failure becomes more explicit, but in most cases you'd get a TypeError or ValueError anyway later on, when trying to process a tuple as a value or vice versa (unless your return value is also a collection, as you pointed out).

I'm leaning slightly towards changing the API to return single value/None for (value) and () returns, though.

Let's collect some more opinions. John, Tomeu, what do you think?
Comment 15 johnp 2011-02-11 18:18:39 UTC
In response to the keyword argument, the one thing I like about keyword arguments is that they are somewhat self documenting where positional arguments can be ambiguous.  On the other hand I'm not attached to having it.  I've been out of D-Bus development for some time so I was most likely rehashing some discussion we had in the past when we came to the decision that keyword arguments would be for proxy behaviour.  In any case I am fine with restricting keyword arguments.

As for returning non-tupple arguments, here I think the consistency with GI and Python is more important.  As Simon has stated we can use g_dbus_proxy_set_interface_info to enforce a signature.
Comment 16 johnp 2011-02-11 18:29:34 UTC
Looking at the async handler, are we sure we want to have one handler for both success and error?  The reason we had both is so users didn't have to check the type of the return value and could just handle a success or error in different code paths.
Comment 17 johnp 2011-02-11 18:40:33 UTC
I should note that with the reply handler we would use the signature of the python function which allows for errors to be found if interfaces change.  If someone wanted flexibility they could just have an (*argv) signature.

User data also doesn't make much sense in python since you can just bind data to the function object or create a class and set the method as the return though I guess we do the same for signals so it is ok to keep.
Comment 18 Martin Pitt 2011-02-14 07:57:57 UTC

(In reply to comment #16)
> Looking at the async handler, are we sure we want to have one handler for both
> success and error?  The reason we had both is so users didn't have to check the
> type of the return value and could just handle a success or error in different
> code paths.

I see no problem with supporting both styles. If you have an _error_handler set, that will be called, otherwise the _result_handler will get the exception object.

> User data also doesn't make much sense in python since you can just bind data
> to the function object or create a class and set the method as the return

Right, but IMHO that's a lot less convenient to write. It's an optional argument, but I'd like to keep it since an user data argument is so common in GTK as well (and with pygi you have to specify it all the time).

(In reply to comment #15)
> In any case I am fine with restricting keyword arguments.

TBH I'm not quite sure what this means. Do you favor the "_reply_handler" style (safe for all times) or "reply_handler" (looks nicer, but potential conflict with d-bus method keywords)?

Thanks for the review!
Comment 19 Martin Pitt 2011-02-14 09:02:56 UTC
Created attachment 180806 [details] [review]
git formatted patch

This adds an optional explicit _error_handler, and does the (resultvalue) -> resultvalue and () -> None unboxing.

For the latter I now also added a test case for a multiple return value method. Unfortunately D-BUS itself does not have any; Notifications.GetServerInformation() is one that should still be available on pretty much every desktop out there, so I used that; however, as this won't be available in restricted build environments, I made a "service not found" condition nonfatal, and the multi-value test will simply not be done at all. This will get much better once it gets possible to write GDBus servers in Python, but that's currently impossible due to bug 640069.

I kept the "_" keyword prefix for now.
Comment 20 johnp 2011-02-16 17:14:09 UTC
I'm going to review and hopefully commit this today.  As for the _ prefix, that is up to you.  Simon's comment reminded me that we had this discussion long ago and came to the conclusion that keywords would only be used for local parameters, not dbus arguments.   In my opinion keyword arguments are also there for documentation, not just optional arguments but then again I don't see us ever taking the time to enable them for d-bus arguments (they would require the introspection round trip and extra processing).

My only real argument for keeping the _ is that if there was a parameter named timeout or error_handler, etc. it would be a bit confusing to the developer.
Comment 21 johnp 2011-02-16 21:25:44 UTC
Comment on attachment 180806 [details] [review]
git formatted patch

Looks good to me.  Please commit to the pygobject-2-28 branch as well as master.  It is up to you if you want to remove the _ prefix
Comment 22 Martin Pitt 2011-02-19 14:38:47 UTC
Comment on attachment 180806 [details] [review]
git formatted patch

Committed to master and 2-28 with the keyword arguments reverted to the original (non _ prefixed) names. Thanks everyone for the reviews!