GNOME Bugzilla – Bug 662383
(skip) return values cause SystemError: error return without exception set
Last modified: 2011-10-21 15:51:48 UTC
Created attachment 199629 [details] reproducer I'm currently writing a test suite for udisks2, which is using gdbus-codegen to generate the libudisks library. I noticed that many calls fail like Traceback (most recent call last):
+ Trace 228881
self._do_fs_check('btrfs')
self._do_udisks_check(type)
self.fs_create(None, type, options)
block.call_create_filesystem_sync(type, options, None)
return info.invoke(*args, **kwargs)
I isolated this into a small test script which calls call_mount_sync()/call_umount_sync() on a block device through udisks2: $ GI_TYPELIB_PATH=udisks LD_LIBRARY_PATH=udisks/.libs python ~/test.py /dev/sdb calling through gdbus-codegen glue Traceback (most recent call last): File "/home/martin/test.py", line 23, in <module> fs.call_unmount_sync(GLib.Variant('a{sv}', {}), None) File "/usr/lib/python2.7/dist-packages/gi/types.py", line 43, in function return info.invoke(*args, **kwargs) SystemError: error return without exception set This crashes in fs.call_unmount_sync(GLib.Variant('a{sv}', {}), None) However, when I don't use the generated call_XX_sync() wrapper, and instead use the GDBuxProxy object directly (through the pygobject overrides), it works fine: fs.Unmount(('(a{sv})'), {}) This makes me believe that there is a bug somewhere in gdbus-codegen-generated glue code which returns the wrong return boolean in some cases? This is with glib 2.30.0.
Oh, forgot to mention: If you call the test script with just one argument (block device), it uses the gdbus-codegen glue. If you specify a second one, it uses the GDBuxProxy.call_sync() method, which works.
On second look, I peeked into udisks_filesystem_call_unmount_sync(), and the return code seems alright, and changing it doesn't even make a difference. error is properly initialized and *error == NULL, so there was no exception. The SystemError is raised in python itself, in ./Python/ceval.c. However, I'm afraid I don't understand ceval.c at all, so I can't say what it's complaining about. So this might not be a glib bug after all.
The Google sphere is full with similarly ominous errors in other programs. Common that I saw in more than just one place are: (1) it looks related to multi-threaded programs. I think the call_sync stuff presumably sets up a local main loop somewhere, which might set off a separate thread. Anyway, adding GLib.threads_init() doesn't change things. (2) many, if not all reports say that this is probably due to a broken python extension, which would be pygobject in this case.
(In reply to comment #3) > (1) it looks related to multi-threaded programs. I think the call_sync stuff > presumably sets up a local main loop somewhere, which might set off a separate > thread. Anyway, adding GLib.threads_init() doesn't change things. Yeah, it ends up calling g_dbus_connection_send_message_with_reply_sync() that sets up its own private GMainContext, passes work to the (private) GDBus worker thread and then sits in the main-loop waiting for completion from the worker thread. But all this is an implementation detail - the key thing is that it simply blocks the calling thread until a reply is received (or until the timeout is reached).
I confirm that dropping the (skip) from * Returns: (skip): %TRUE if the call succeded, %FALSE if @error is set. indeed fixes this, i. e. pygobject is expecting that error flag but doesn't get it. When I simply remove that from udisks-generated.c and rebuild, the example works perfectly.
Created attachment 199646 [details] [review] test case This only happens if the method has no other (out) arguments, which is why the existing regress_test_obj_skip_return_val() test case did not catch it. I added a new test case without (out) arguments: http://git.gnome.org/browse/gobject-introspection/commit/?id=d6ab510feb72ac0ff30f0942e874e2d52f3e82f6 With that, a reproducer is trivial, attaching patch against pygobject which adds it to the test suite.
Created attachment 199647 [details] [review] test case this time with correct test case name
Created attachment 199659 [details] [review] Fix "Returns: (skip)" method calls without (out) arguments This is the bug fix including the test case.
Created attachment 199660 [details] [review] Fix "Returns: (skip)" method calls without (out) arguments Whoops, I had the test case part for real exception raising commented out for debugging. This re-enables it.
Created attachment 199661 [details] [review] Fix "Returns: (skip)" method calls without (out) arguments Erk, third time is the charm!
Created attachment 199663 [details] [review] Fix "Returns: (skip)" method calls without (out) arguments This is a stronger version of the patch which now asserts our logic. I. e. if we do not have any (out) arguments and a skipped return value, we don't ask whether our current py_return is NULL, but assert it, to catch bad (future?) code paths where we would unexpectedly have an existing return value.
Comment on attachment 199663 [details] [review] Fix "Returns: (skip)" method calls without (out) arguments Discussed with Tomeu on IRC, he acked. Committed to master.