GNOME Bugzilla – Bug 666270
Support GHashTable and GError as closure arguments
Last modified: 2012-01-30 15:10:45 UTC
In the library which I want to use from python [0] there's the following callback definition: typedef void (*SignonAuthSessionProcessCb) (SignonAuthSession *self, GHashTable *session_data, const GError *error, gpointer user_data); In my python code, this asynchronous callback is successfully invoked, but the "session_data" and "error" parameters are always "None". After some debugging, I found that the marshalling of GHashTable and GError in pygi-closure.c is not handled (variables of these types are always set to NULL). [0] http://code.google.com/p/accounts-sso/source/browse/?repo=libsignon-glib
Created attachment 203577 [details] [review] Proposed fix Marshalling of GHashTable and GError from C to python is already implemented; the attached patch trivially brings it into use for closures too. I tested this with the before mentioned library and it works nicely, both for GHashTable and GError.
Thanks for this! This should go along with a test case, though, to ensure that it works and stays working.
Created attachment 205852 [details] [review] Add regress test methods for callbacks taking GError and GHashTable It seems this simple patch isn't sufficient. I created some regression tests around this, and it seems to have quite some problems. As I don't know any details about how you use these callbacks in your code, I just created two very simple test functions which just take a callback which take a GError, or a GHashTable respectively. This is the patch to gobject-introspection which adds the two new test functions.
Created attachment 205853 [details] [review] Add test cases for callbacks taking GError/GHashTable This adds pygobject test cases for calling the two new Regress functions (from the previous g-i patch). Without your patch, the arguments indeed are None, as you said: testCallbackGError (test_everything.TestCallbacks) ... Traceback (most recent call last):
+ Trace 229487
self.assertEqual(error.message, 'regression test error')
FAIL [...] testCallbackHashTable (test_everything.TestCallbacks) ... Traceback (most recent call last): [...] AssertionError: None != {'foo': 1, 'bar': 2} and the two cases fail on "self.assertTrue(TestCallbacks.called)" as the callbacks error out before setting called = True. With your simple patch, however, it doesn't look much better. The GError one callback fails with File "/home/martin/upstream/pygobject/tests/test_everything.py", line 360, in callback self.assertEqual(error.code, Gio.IOErrorEnum.NOT_SUPPORTED) [...] AssertionError: 0 != <enum G_IO_ERROR_NOT_SUPPORTED of type GIOErrorEnum> FAIL i. e. the error.code field is not valid (error.domain and error.message seem to be correct). On top of that, it seems to cause memory corruption, as one of the following test cases segfaults: testCallbackUserdataRefCount (test_everything.TestCallbacks) ... make[2]: *** [check-local] Segmentation fault (core dumped) When I comment out the g_error_free() in regress_test_gerror_callback() in the g-i patch, the segfault doesn't happen. So this might interact badly with some pygobject magic handling of GErrors, or is just due to the too simple handling of the GError object in your _pygi_closure_convert_ffi_arguments() patch? The GHashTable test callback gets a bogus dictionary passed: testCallbackHashTable (test_everything.TestCallbacks) ... Traceback (most recent call last): File "/home/martin/upstream/pygobject/tests/test_everything.py", line 369, in callback self.assertEqual(data, mydict) [...] AssertionError: {'\x0b': 40728480, '\n': 40728504} != {'foo': 1, 'bar': 2} - {'\n': 40728504, '\x0b': 40728480} + {'bar': 2, 'foo': 1} and modifying it doesn't work of course. So I don't think we can apply this until this works. Does that patch actually work for you? Perhaps you can add some more context what and how you call?
Created attachment 205854 [details] [review] Add regress test methods for callbacks taking GError and GHashTable I forgot to annotate the GHashTable data types in test_hash_table_callback() as well (just did it in the TestCallbackHashtable type). With that, the GHashTable test works fine now. So that leaves the GError problems: the invalid code and the crash.
Created attachment 205860 [details] [review] Add regress test methods for callbacks taking GError and GHashTable Actually throw G_IO_ERROR_NOT_SUPPORTED instead of G_IO_ERROR_FAILED in the g-i regression test functions. For the purpose of a test case, it's better to use an error code which is not actually zero. Now the pygobject testCallbackGError case actually works, it just causes the segfault. The segfault happens because pygobject ignores the "(transfer none)" annotation for GError, and thus the resulting double free causes a segfault. I guess the marshaller should use a separate case: for GError and use g_error_copy() for this?
Comment on attachment 205853 [details] [review] Add test cases for callbacks taking GError/GHashTable Argh, obsoleted the wrong attachment..
Seems to be some overlap between this bug (and patches) and https://bugzilla.gnome.org/show_bug.cgi?id=666098 and the patches in that bug
Created attachment 205865 [details] [review] Second patch for gobject-introspection gobject-introspection patch to add another method for regression tests; similar to the above, but in this case the GError transfer is "full".
Created attachment 205866 [details] [review] pygobject: fix indentation
Created attachment 205867 [details] [review] pygobject: respect transfer type when demarshalling GErrors
Created attachment 205868 [details] [review] pygobject: second test case for GError: transfer full Now all patches are uploaded.
Comment on attachment 205866 [details] [review] pygobject: fix indentation Indentation patch is trivial, pushed this.
Comment on attachment 205865 [details] [review] Second patch for gobject-introspection The second g-i patch is missing the actual definition of TestCallbackOwnedGError. Please run make check in g-i, to see this. The test fails because TestCallbackOwnedGError is missing in the gir, and test_owned_gerror_callback() is generated on a different position.
Comment on attachment 203577 [details] [review] Proposed fix Removing "needs work" from original patch, as the second pygobject patch fixes the transfer ownership handling for GError.
Created attachment 205874 [details] [review] gobject-introspection: second patch, for transferred GErrors (In reply to comment #14) > (From update of attachment 205865 [details] [review]) > The second g-i patch is missing the actual definition of > TestCallbackOwnedGError. Fixed.
Thanks Mardy! These patches look fine to me, and they are working nicely now. The second g-i one is missing a "transfer full" annotation for the TestCallbackOwnedGError type (as that's what we are intend to test). For final upstream committing they ought to get merged (at least the two parts for the test cases).
Created attachment 205879 [details] [review] [g-i] Add regress test methods for callbacks taking GError and GHashTable This merges the two g-i patches to add the three Regress functions and fixes the annotation.
Created attachment 205882 [details] [review] [1/1] Support GHashTable and GError as callback/closure arguments This pygobject patch consolidates the first patch and the two test case patches and removes an unused variable in the new test cases, and now also has a consistent commit log.
Created attachment 205883 [details] [review] [2/2] Respect transfer-type when demarshalling GErrors This second patch just updates the commit log for the "transfer full" patch.
Thanks Tomeu for the review!
Hi Martin and Mardy, I think this crash I am seeing might be caused by your patches: https://bugzilla.gnome.org/show_bug.cgi?id=669018
Hi Christian, that's seems related indeed. If you don't want python-gobject to free the GError, you need to set the "transfer none" annotation on the signal.
Thanks Mardy, For posterity, the correct link for 22 is : https://bugzilla.gnome.org/show_bug.cgi?id=668862 which is the actual bug report for the crasher.
I subscribed to bug 668862 now and asked a question there. Thanks for the pointer.