GNOME Bugzilla – Bug 603095
Implementation of callbacks
Last modified: 2010-04-17 15:04:12 UTC
Created attachment 148566 [details] [review] Implementation of Callbacks for review Attached is my attempt at implementing callbacks. I am aware of at least 2 instances where the code is not yet complete but I haven't actually run into needing those two cases implemented yet and I am not sure the correct way to do those implementations. (I'd love to hear your opinions on those two). As far as functionality, this works 100% for me in testing with GUPnP.
Review of attachment 148566 [details] [review]: Great work! ::: configure.ac @@ +5,3 @@ AC_CONFIG_MACRO_DIR(m4) +#m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES(yes)]) Forgot to uncomment that? Mind that you can do make V=0 to disable shaving. @@ +54,3 @@ +fi +AC_MSG_RESULT([$have_libffi]) +AM_CONDITIONAL(HAVE_LIBFFI, test "$have_libffi" = "yes") Isn't enough with PKG_CHECK_MODULES(FFI, libffi >= 3.0)? Don't we depend on FFI anyway because of our dependency to gobject-introspection? ::: gi/Makefile.am @@ +17,3 @@ _gi_la_CFLAGS = \ $(PYTHON_INCLUDES) \ + $(GNOME_CFLAGS) Extra whitespace added? (check other occurrences of this) ::: gi/pygi-argument.c @@ +608,3 @@ + +static void _pygi_handle_closure(ffi_cif *cif, void *result, void **args, void *userdata){ I would move these functions to pygi-closure.[hc], I'm going to extend them with my vfunc work. @@ +610,3 @@ +static void _pygi_handle_closure(ffi_cif *cif, void *result, void **args, void *userdata){ + PyGILState_STATE gstate; + PyGINativeClosure *privates = userdata; Not sure what means privates here. I would expect "closure" instead, for example. @@ +627,3 @@ + // Lock the GIL as we are coming into this code without and we + // may be executing python code + gstate = PyGILState_Ensure (); Considered using pyg_gil_state_ensure instead? I think the comment is superfluous, as any user of the Python C API should know about that. @@ +637,3 @@ + + if (pyargs == NULL){ + return; The GIL needs to be released. @@ +642,3 @@ + for (i = 0; i < n_args; i++) + { + arg_info = g_callable_info_get_arg (privates->info, i); arg_info needs to be unref'ed. @@ +651,3 @@ + + PyTuple_SetItem(pyargs, i, pyarg); + } This is not the brace style we are using in the rest of the code. (check for other instances) @@ +670,3 @@ + +PyGINativeClosure* +_pygi_make_native_closure (GICallableInfo* info, GITypeInfo* type_info, PyObject *function) What this type_info refers to? Cannot find any use of it. @@ +684,3 @@ + "__pygi_native_closure"); + privates = PyCObject_AsVoidPtr(cache); + }else{ Watch out spaces around braces (there's other instances of it). @@ +695,3 @@ + closure = + g_callable_info_prepare_closure (info, cif, _pygi_handle_closure, + privates); I would break at the first arg that has not enough space. @@ +703,3 @@ + + PyObject_SetItem(dict, PyString_FromString("__pygi_native_closure"), + PyCObject_FromVoidPtr(privates, NULL)); So a closure is never released? @@ +730,3 @@ + + Superfluous whitespace? @@ +1108,3 @@ } + // g_assert(is_pointer); Should we just remove all these as part of https://bugzilla.gnome.org/show_bug.cgi?id=602628 ? @@ +1312,2 @@ hash_table = g_hash_table_new(hash_func, equal_func); + Superfluous whitespace. ::: tests/runfile @@ +1,3 @@ +run +bt +quit Is this needed?
*** Bug 601937 has been marked as a duplicate of this bug. ***
Created attachment 148718 [details] [review] Patch version 2 Closure stuff pulled out, spurious whitespace changes fixed, simpler autofoo changes and removed extra file in tests directory.
Review of attachment 148718 [details] [review]: Excellent. Please check whitespace and other style considerations and if there's anything in my comments you don't agree with, please tell so I don't have to repeat myself. ::: Makefile.am @@ +3,3 @@ AM_CFLAGS = \ -Wall \ + -g $(FFI_CFLAGS) I would add it only in gi/Makefile.am ::: gi/pygi-argument.c @@ +976,3 @@ case GI_INFO_TYPE_CALLBACK: + { + PyGINativeClosure *privates; What means privates here? Why not using just closure? @@ +980,3 @@ + type_info, + object); + arg.v_pointer = privates->closure; What about _pygi_make_native_closure just returning the function to the ffi wrapper? AFAICS, the code calling it doesn't need anything else in PyGINativeClosure. @@ +1002,3 @@ } + // g_assert(is_pointer); Please ping Simon about pushing his patch in https://bugzilla.gnome.org/show_bug.cgi?id=602628 ::: gi/pygi-closure.c @@ +28,3 @@ +#include <pygobject.h> + +void _pygi_handle_closure(ffi_cif *cif, void *result, void **args, void *userdata){ Should it be static? @@ +57,3 @@ + + if (pyargs == NULL){ + return; Shouldn't we PyGILState_Release ? @@ +90,3 @@ + +PyGINativeClosure* +_pygi_make_native_closure (GICallableInfo* info, GITypeInfo* type_info, PyObject *function) Cannot find any instance where type_info is used. @@ +110,3 @@ + closure->info = (GICallableInfo *) g_base_info_ref ((GIBaseInfo *) info); + closure->function = function; + closure->cif = cif; Why are we storing the cif in PyGINativeClosure? If we don't use it, please drop it from there. @@ +111,3 @@ + closure->function = function; + closure->cif = cif; + closure->type_info = type_info; Same as above. @@ +118,3 @@ + closure->closure = fficlosure; + + Py_XINCREF(function); Shouldn't we rather fail if function == NULL?
Created attachment 148767 [details] [review] Add marshalling and invocation of callbacks (zgold) Will take Zach's work and move it forward so I can work in parallel on vfuncs.
Created attachment 148790 [details] [review] patch ontop of tomeu's work For some reason the following is necessary to make Tomeu's version of the patch work with my demo GUPnP application. If you look at the patch you'll notice it doesn't make any real substantive changes. I'm still confused as to why this is necessary.
(In reply to comment #6) > Created an attachment (id=148790) [details] [review] > patch ontop of tomeu's work > > For some reason the following is necessary to make Tomeu's version of the patch > work with my demo GUPnP application. If you look at the patch you'll notice it > doesn't make any real substantive changes. I'm still confused as to why this > is necessary. My plan is to reproduce it here by running sugar on top of PyGI and find a good test case.
Created attachment 148936 [details] [review] patch on top of tomeu's work this is a replacement for Zach's patch which is damaged (I couldn't mark it as obsolete)
Hey guys, There is definitely some more work to do (especially on testing... I don't know what pygi's upstream testing decisions are yet as far as merging with upstream gi test suite/Everything module are) but the bulk of this is here and as far as I can tell everything is working with some manual testing. I'd appreciate some review of the core changes themselves (all the commits from today). I used a lot of ideas and a couple snippets from gjs (http://git.gnome.org/browse/gjs/tree/gi/function.c?id=a14aa201198bcaa85e3d58582fb2d55ecebca3d3#n480) http://github.com/ZachGoldberg/PyGiCallbacks Thanks! -Zach
Created attachment 152885 [details] [review] Implementing Callbacks Simon / Tomeu, please see these two new patches and review formally. I know they still need a lot of work but to get a solid round of constructive criticism would help guide me immensely. Thanks guys!
Created attachment 152886 [details] [review] Implementing SCope The second patch, implementing scope
Review of attachment 152885 [details] [review]: Thanks. The overall coding style should be reviewed. Please mark as obsolete the previous patches if possible. ::: Makefile.am @@ +3,3 @@ AM_CFLAGS = \ -Wall \ + -g $(FFI_CFLAGS) Add that to _gi_la_CFLAGS in gi/Makefile.am instead. ::: configure.ac @@ +35,3 @@ +#libffi +PKG_CHECK_MODULES(FFI, libffi >= 3.0) Are you sure about the version? ::: gi/pygi-argument.c @@ +874,3 @@ case GI_TYPE_TAG_INTERFACE: { + GIBaseInfo *interface_info; Don't change the names unnecessarily. @@ +887,3 @@ + type_info, + object, + &(arg.v_pointer))) There is no need to pass interface_info since it can be obtained from the type_info. @@ +1655,3 @@ if ((direction == GI_DIRECTION_IN && transfer == GI_TRANSFER_NOTHING) || (direction == GI_DIRECTION_OUT && transfer == GI_TRANSFER_EVERYTHING)) { + // g_free(arg->v_string); Why not? ::: gi/pygi-closure.c @@ +71,3 @@ + + PyTuple_SetItem(pyargs, i, pyarg); + } First, release mechanisms is not implemented. Secondly, this will not work for arrays notably. So please keep argument support in the fridge for now. @@ +75,3 @@ + + retval = PyEval_CallObject((PyObject *)closure->function, pyargs); + Py_DECREF(pyargs); Use PyObject_Call. @@ +80,3 @@ + result = NULL; + PyGILState_Release (gstate); + return; Use a goto instead of repeating the cleanup stuff. @@ +83,3 @@ + } + + *(GArgument*)result = _pygi_argument_from_object(retval, return_type, return_transfer); Keep in the fridge for now too. @@ +93,3 @@ + GITypeInfo* type_info, + PyObject *function, + gpointer* closure) Drop this last argument. @@ +103,3 @@ + + // TODO: Assert that function is in fact a function + Py_XINCREF(function); We can safely assume in this function that function is a valid Python callable. Thus, just use INCREF. @@ +109,3 @@ + cache = (PyObject*) PyObject_GetAttrString(function, + "__pygi_native_closure"); + closure = PyCObject_AsVoidPtr(cache); Drop this "cache" mechanism for now. @@ +117,3 @@ + invoke_closure->function = function; + invoke_closure->cif = cif; + invoke_closure->type_info = type_info; Drop this last field too. @@ +124,3 @@ + invoke_closure->closure = fficlosure; + + *closure = invoke_closure->closure; Those three lines could be reduced to one. ::: gi/pygi-info.c @@ +181,3 @@ case GI_INFO_TYPE_CALLBACK: + type = &PyGICallableInfo_Type; + break; This is absolutely not necessary. You never access callback infos from Python. ::: gi/pygi-private.h @@ +83,3 @@ + return rarray->elt_size; +} +#endif ??? ::: tests/libtestgi.c @@ +2934,3 @@ + callback(); + return; +} Keep only this test for now. @@ +2965,3 @@ + callback(struct_); + return; +} You have to test for every argument type, for every transfer mode. That's gonna be quite a few tests, I know...
Created attachment 153161 [details] [review] Callbacks support fixed w/Simon's comments This patch addresses the issues mentioned in simon's previous review.
p.s. I have broken out tests, scope implementation, argument/return value processing into separate branches. However, in order to avoid spamming everybody with 5 more emails with patches that aren't really ready to be reviewed I have only posted the base patch here. Those patches (rebased of course ontop of the new patch) are available here: http://github.com/ZachGoldberg/PyGiCallbacks/tree/patch
Review of attachment 153161 [details] [review]: ::: gi/pygi-closure.c @@ +85,3 @@ + Py_INCREF(function); + + cif = g_new0 (ffi_cif, 1); Use g_slice_new as it is more efficient. It is never freed though.
Created attachment 153174 [details] [review] Change g_new0 to g_slice_new0
Review of attachment 153174 [details] [review]: It's getting nicer, thanks. I still see quite a few unused variables, and the code style is neither uniform, nor in accord with the rest of the project. That should be fixed. Couldn't we add the scope management stuff at the same time? ::: gi/pygi-closure.c @@ +48,3 @@ + gstate = PyGILState_Ensure (); + + n_args = g_callable_info_get_n_args (closure->info); For now, just return an exception if n_args is greater than 0. @@ +51,3 @@ + return_type = g_callable_info_get_return_type (closure->info); + return_tag = g_type_info_get_tag (return_type); + return_transfer = g_callable_info_get_caller_owns(closure->info); Same if the return TypeTag is not VOID. @@ +59,3 @@ + } + + retval = PyEval_CallObject((PyObject *)closure->function, pyargs); I don't see PyEval_CallObject in Python 2.6.4 documentation. Why don't you use PyObject_Call? @@ +63,3 @@ + + if (retval == NULL){ + goto end; This goto is useless. The Python error must be cleared. ::: gi/pygi-info.c @@ -181,3 @@ - case GI_INFO_TYPE_CALLBACK: - PyErr_SetString(PyExc_NotImplementedError, "GICallbackInfo bindings not implemented"); - return NULL; Don't drop this. We could need to access CallbackInfo's from Python in the future.
Created attachment 153578 [details] [review] Formatting + more simon fixes Hopefully this is the last round of review on the base commit :)
Created attachment 153585 [details] [review] 4 types of scope patch Round 1 for the scope patch.
Created attachment 153710 [details] [review] Add primary support for callbacks Essentially Zach's patch reworked.
Created attachment 153774 [details] [review] Reworked scope patch ontop of Simon's base callback implementation patch /me doesn't like merging
Created attachment 153785 [details] [review] Add scope support I botched the merge with the last patch. Hopefully got everything this time.
Review of attachment 153785 [details] [review]: Introducing global variables is really not desirable. I think you should return both closures (callback and destroy notify callback) at the same time, and handle that in _wrap_g_function_info_invoke. This would avoid the destroy notify callback list. Also, part of the memory management can be done in the release function. This would avoid the free list.
Review of attachment 148767 [details] [review]: This patch is obsolete.
Created attachment 155360 [details] [review] Add support for callbacks taking arguments I've also merged some changed from Tomeu's patch to Bug 602736. I beleive it's robust enough, both in and out args are working, as well as returns values. The only missing thing afaics is array support.
(In reply to comment #25) > Created an attachment (id=155360) [details] [review] > Add support for callbacks taking arguments > > I've also merged some changed from Tomeu's patch to Bug 602736. I beleive it's > robust enough, both in and out args are working, as well as returns values. The > only missing thing afaics is array support. This is fantastic! I had no idea you were working on this, you should've sent me a ping I would've been glad to assist if possible. I haven't had a chance to look at this in detail yet, but I'm sure there will be a bunch of feedback soon. So we now have: 1) a basic callbacks patch which is more or less signed off on 2) A scope patch which implements the three types of scope at the moment which will need another round or two of review before its ready 3) An argument handling patch My list of things remaining: 1) Array argument support 2) Tests. Many of them.
Created attachment 155361 [details] [review] Implement callback scope types I've done quite a bit of refactoring, now the free list is no longer necessary for notified callbacks. There remains only one global variable (to be honest, I didn't digg too much there). The following patch is required since notified callbacks have at least a user_data argument. I wonder how the original version of the patch was supposed to work, it wasn't hooked at all into the invoke code. (btw, I cannot mark others patches as obsolete, someone else will need to do it)
Created attachment 155362 [details] [review] Tests for callbacks Taken from some patch here, and added some, I tried to reduce their number by testing multiple thing simultaneously do we can add more tests.
Created attachment 155567 [details] [review] Update of the scope patch The old patch had a local variable shadowing the global variable so "call" scope closures wheren't freed. Also instead of passing GIArgInfo around, it now just passes the needed (scope) information.
Review of attachment 155360 [details] [review]: Abderrahim, First, I don't know to what your patch applies. Please mention which patches get obsoleted by yours. Secondly, as Zach noted, we agreed on an order to get things done; scope comes before arguments. Thirdly, the patch seems rather incomplete; it doesn't do anything about memory management, for instance.
Review of attachment 155362 [details] [review]: In comment 12, I wrote: one test for every argument type, for every transfer mode.
Sorry, I didn't receive a notification for this. (In reply to comment #30) > Review of attachment 155360 [details] [review]: > > Abderrahim, > > First, I don't know to what your patch applies. Please mention which patches > get obsoleted by yours. on top of your attachement 153710. it obsoletes the patch with the same functionality (attachement 153785) > Secondly, as Zach noted, we agreed on an order to get things done; scope comes > before arguments. In comment 27 I wrote : "The following patch is required since notified callbacks have at least a user_data argument." (I actually meant the preceding). > Thirdly, the patch seems rather incomplete; it doesn't do anything about memory > management, for instance. Right. I think this needs someone very familiar with the inner workings of pygi. Anyway, good luck with the hackfest :-) (btw, another feature I'd like to have is passing callbacks to python functions. is this fine or should I file a new bug?)
Created attachment 158905 [details] [review] Implementation of basic callback support This patch was originally written by Zach Goldberg <zach@zachgoldberg.com> with modifications and review by Simon van der Linden <svdlinden@src.gnome.org>
Comment on attachment 153785 [details] [review] Add scope support This patch will soon be replaced by a complete rework with work done by myself & colin walterss
Created attachment 158916 [details] [review] Implementation callback support with scoping and basic argument support. This patch was originally written by Zach Goldberg <zach@zachgoldberg.com> with modifications and review by Simon van der Linden <svdlinden@src.gnome.org> and Colin Walters <walters@verbum.org>. This impementation enforces the assumption that any one function signature can only have one (callback, userdata, destronotify) tuple. This allows us to move callback creation into the actual function invoke pipeline and also to keep just one destroy notify callback around, vastly simplifying the code.
Review of attachment 158916 [details] [review]: Pretty good! The tests seem borked, can you make check? ::: gi/pygi-argument.c @@ +30,3 @@ #include <pygobject.h> +#include <girffi.h> Not needed? ::: gi/pygi-closure.c @@ +94,3 @@ + PyGILState_Release(state); + + /* If this was an async type closure we need to free it now that we have used it */ AFAICS from the code below, the closures that get free'd now are the ones with GI_SCOPE_TYPE_CALL. @@ +146,3 @@ + if (!global_destroy_notify){ + + static const ffi_type *destroy_notify_args[] = {&ffi_type_pointer}; Guess we don't need destroy_notify_args? @@ +169,3 @@ + +gboolean +_pygi_scan_for_callbacks (PyGIBaseInfo *self, As _pygi_scan_for_callbacks isn't a method, it shouldn't have an arg called "self". @@ +179,3 @@ + *callback_index = G_MAXUINT8; + *user_data_index = G_MAXUINT8; + *destroy_notify_index = G_MAXUINT8; What about changing to use gints and -1 as the special value? ::: tests/test_gi.py @@ +1422,3 @@ + + + def testCallbackAsync() : missing self, extra space @@ +1428,3 @@ + + Everything.test_callback_async(callback, 44); + let i = Everything.test_callback_thaw_async(); Just checking I'm actually reading the code?
Created attachment 158949 [details] [review] Implementation callback support with scoping and basic argument support. This patch was originally written by Zach Goldberg <zach@zachgoldberg.com> with modifications and review by Simon van der Linden <svdlinden@src.gnome.org> and Colin Walters <walters@verbum.org>. This impementation enforces the assumption that any one function signature can only have one (callback, userdata, destronotify) tuple. This allows us to move callback creation into the actual function invoke pipeline and also to keep just one destroy notify callback around, vastly simplifying the code.
Created attachment 158950 [details] [review] Implementation callback support with scoping and basic argument support. and prevents a memory leak in pygi_create_callback. Thanks Tomeu!
Created attachment 158954 [details] [review] Implementation callback support with scoping and basic argument support.
Pushed as http://git.gnome.org/browse/pygi/commit/?id=610dd1eec