GNOME Bugzilla – Bug 556489
callback annotations
Last modified: 2015-02-07 17:03:26 UTC
During discussion with Mike at the summit the issue of callbacks, and in particular their scope came up. Many functions take a GDestroyNotify, but not all. A common case is foo_container_foreach() style functions where the callback is only used during that particular function.
While thinking about this one - are there any major APIs out there that take multiple DestroyNotify (besides g_hash_table_new_full)? Or can we basically assume that a single DestroyNotify is always paired with either a user data or a (callback, data) pair? We may need some annotations to bind these things together, though I think some simple rules could cover the vast majority of APIs. Anyways, initial strawman on scope: /** * gtk_container_foreach: * @container: a #GtkContainer * @callback: (scope call): a callback * @callback_data: callback user data Note "(scope call)". Mike you said there were ones associated with the lifetime of the object? Do we need (scope object) ?
This change will need to be reflected in the typelib, too. Probably: typedef enum { GI_SCOPE_INVALID, GI_SCOPE_CALL, GI_SCOPE_OBJECT } GIScope; GIScope g_arg_info_get_scope (GIArgInfo *info);
I have definitely encountered APIs where 2 callbacks where specified that used a single callback data and destroy notify parameter. Can't recall off the top of my head from where, but they exist. The five interesting scopes that I'm aware of are: Call - callback parameter persists for the duration of the method call and can be released on return. Async - callback persists until it is invoked. The callback is only invoked once. Add/Remove - callback is added by one method and removed by another method. Notified - callback persists until a DestroyNotify delegate is invoked. Infinite - callback persists forever. It's arguable that all non-call scope callbacks that don't have destroy notification are API bugs. Owen more or less implied this in Boston. I think it's reasonable to support an Async scope too, though, however. The "invoke-once" async callback is a pretty common paradigm, and expecting those to all be destroy notified is a little extreme. I think it would also be reasonable to report to library authors that non-call, non-async, non-notified callbacks are a bindability issue and a destroy notification alternative should be provided, perhaps as a *Full method. I should also point out that it would be nice to associate the callback_data parameter to the callback. In a language like C#, where callback delegates can be instance methods and have access to instance data, you don't have to pass a second data parameter to callbacks, so we hide all "callback data" parameters in the public API and just pass NULL to the native methods.
Mike, thanks for the comments. I agree with you the Async use case makes sense. Also agreed on the user_data bit. Let me expand the scope of this bug a bit to include that. Let's examine a few cases and figure out where we need annotations and what would be a good default. Here's a random one from Gio: void g_loadable_icon_load_async (GLoadableIcon *icon, int size, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); There's no GDestroyNotify. After reading the GIO docs, this one is in the "Async" category. Actually, we should say that GObject libraries going forward should use GAsyncResult, and I think it's reasonable for bindings to handle it specially. Looking at the Mono Gtk.metadata there's a total of 6 async cases in there, 4 of those on GtkClipboard. Not too many - maybe a good path is to get patches into GTK+ to add different functions which use GAsyncResult, deprecate the old ones and use the Shadows: annotations to override them in the bindings. I don't see any usage of "infinite" in Gtk.metadata? Now on the topic of user_data - my current thought is that first we require that APIs write exactly "gpointer user_data". This is almost a standard, we should just take it all the way. Finally, I suggest that if there is a consecutive grouping of (function pointer, GLib.DestroyNotify, gpointer user_data) or (function pointer, gpointer user_data, GLib.DestroyNotify) we recognize all of those by default as being paired together. (There is also GLib.FreeFunc which is used by GConf). Taken together this would cover a vast swath of APIs without too much work. Here's another case: void gtk_item_factory_popup_with_data (GtkItemFactory *ifactory, gpointer popup_data, GDestroyNotify destroy, ... In this one there is no callback - it's actually really only useful for C. Good candidate for an "IntrospectionSkip" annotation or the like. Do we know of any interesting methods that take multiple user data or multiple DestroyNotify? In a quick glance through gir files (though I should write code for this), I don't see one. There is g_hash_table_new_full but that one's not very interesting for most bindings, and if it is you just write an override.
Now, this information needs to be reflected in the .gir and .typelib. Here's a strawman: <method name="foreach" c:identifier="gtk_container_foreach"> <return-value> <type name="none" c:type="void"/> </return-value> <parameters> <parameter name="callback" closure="user_data"> <type name="Callback" c:type="GtkCallback"/> </parameter> <parameter name="user_data"> <type name="any" c:type="gpointer"/> </parameter> </parameters> </method> Note the closure="user_data". Another example, this time with a DestroyNotify: <method name="add_action" c:identifier="notify_notification_add_action"> <return-value> <type name="none" c:type="void"/> </return-value> <parameters> <parameter name="action" transfer-ownership="none"> <type name="utf8" c:type="char*"/> </parameter> <parameter name="label" transfer-ownership="none"> <type name="utf8" c:type="char*"/> </parameter> <parameter name="callback" destroy="free_func" closure="user_data"> <type name="ActionCallback" c:type="NotifyActionCallback"/> </parameter> <parameter name="user_data" closure="callback"> <type name="any" c:type="gpointer"/> </parameter> <parameter name="free_func"> <type name="GLib.FreeFunc" c:type="GFreeFunc"/> </parameter> </parameters> </method> Proposed repository API: /** * Return: -1 if this arg is not a user data pointer. If it is a user * data, return the argument index of the function pointer callback. */ int g_arg_info_get_closure (GIArgInfo *info); /** * Return: -1 if this arg is not a callback with an associated DestroyNotify. * Otherwise, return the argument index of its associated DestroyNotify. * For arguments which are callbacks but this function returns -1, assume * that the callback is scoped to the execution of the function. */ int g_arg_info_get_destroy_notify(GIArgInfo *info); Along with the associated .typelib changes.
(In reply to comment #3) > Async - callback persists until it is invoked. The callback is only invoked > once. more precisely, the callback is invoked *exactly* once, even if the async operation is cancelled. (libsoup used to get this wrong with some of its async APIs.)
If writing guidelines for API designers: is it better for APIs to offer a callback/dnotify, or offer a GClosure variant and a convenience wrapper for C developers that takes a callback function and creates a GClosure from it internally?
Good question. I am leaning towards requiring bindings to implement callbacks+dnotify, since there are a lot of APIs like that and it's not really that hard to do assuming you have a FFI library.
Closure seems like a nice direction to push things. We have a closure-based signal connection mechanism in gtk-sharp that we could probably build on. We also have an automated mechanism for <Callback, Data, DNotify> triplets in method signatures. Closure obviously wraps those up into one neat little parameter. If I were going to push for an ideal API, it would probably be Closure.
How much more ideal is the closure-based API? There are cases where having an explicit destroynotify is useful even from C (when it's difficult to figure out in any other way when it's safe to free/unref the user_data), and it's a lot easier to use a callback,data,dnotify API (from C) than to make a closure by hand.
Having GClosure is definitely nicer, no doubt about it; but according to my grep of gir-repository there are over ~120 entry points in the stack which take GDestroyNotify, and there are ~60 in gir-repository. Clearly not all of these are of the same importance; I doubt anyone is using the current GnomeKeyring gir if at all, but still we're talking about quite a lot to be writing wrapper functions for. So it would be good land Andreas' forthcoming patch for supporting the triplicate pair, and encourage bindings to support it. However, I'd be fine with encouraging people to have both for new APIs, or even starting to change some key older ones. There's always going to be a tension here between what introspection can support in increasingly heuristic ways versus what we ask C authors to do. I guess I have the sense that we need to pick our battles to some extent, for example asking C authors to stop using homebrew refcounted structs is a big one.
Random thought: Is there some guideline/document for ensuring a library is easily introspectable? Something that could be put on library.go and can be referenced when new libraries are decided/designed/discussed.
Yep, see: http://live.gnome.org/GObjectIntrospection/WritingBindingableAPIs Obviously just a start, needs work and should probably be merged into the GObject manual. The other thing to do if it's a public free software library is to add it to gir-repository for now.
It's more ideal in that it's easier to "parse". You have a single parameter that encapsulates all the data and notification. What happens when you see this signature: void Foo (BarCallback bar, BazCallback baz, gpointer data, GDestroyNotify notify); are both callbacks notified? do they share the data? Or is bar maybe an async notification to tell you the baz callback has been installed? Closure takes out the ambiguity. You have to rely on documentation or additional markup to figure out what's going on with independent DNotify and data parameters. And yes, we have seen signatures in the wild with multiple notified delegates to a single method. My understanding of the suggestion was that the recommendation would be to provide the closure variant in addition to the triplet for C convenience.
There are two answers here; first we could have introspection warn+skip (and thus fail when bug 554919 is landed) in ambiguous cases like that. For introspection purposes we shouldn't care whether they share the data (everyone should just pass null there). Andreas' patch would also allow us to annotate that both callbacks do indeed share the GDestroyNotify. The second answer is that for these cases we should encourage people to use GClosure, but still support the triple for the "normal" case. I guess the main thing we're debating isn't whether introspection should support heuristics on the triple (I don't think there's disagreement it should), it's whether bindings can be expected to implement it. Finding such functions is a bit tricky with grep, at some point I need to write the 'gapiquery' shell. Do you know offhand how many there are and how important they may be?
Languages that don't generate code on the fly need to use user_data and we should therefore also keep track of the associations between user_data and callback parameters.
Another (relatively minor) factor here is just bloat; if someone has a callback/data/dnotify triplet in the API, usually they have a struct internally to store the triplet, and that struct usually has to be refcounted to be remotely reentrancy-safe. Then the natural way to use a callback/data/dnotify in the language binding is to have another struct which is the data, and in fact may contain a GClosure. It's a bit nicer if an API just uses a closure internally instead of a custom struct, and then there isn't the extra level of indirection, and there isn't a need to reinvent refcounting of these structs to make them safe, etc. (if GHookList or whatever it's called were easier to understand, people could even use that for callback lists, but it isn't.)
Is there a minmimal exmaple we can reference of a callback (usable from C) implemented with a GClosure internally? My intuition is that it is pretty painful ... you have to build GValues, supply a marshaler, etc.
Not sure there's a reason it *should* be painful given the right APIs (granted, it probably is painful now). I guess it's true nobody ever tries to do it, probably for a reason. In theory you should be able to make invoking the closure (or list of closures) look like g_signal_emit() with the varargs. Marshaler is a little work, but not that bad (people do it for signals all the time), and ideally libgobject would have a generic ffi-based marshaler. Anyway, I guess "fix GClosure API and add a list-of-closures/register-closure API" is sort of off topic, sorry about that.
Created attachment 124059 [details] [review] Implements callback argument grouping with 'closure' and 'destroy' parameters
Comment on attachment 124059 [details] [review] Implements callback argument grouping with 'closure' and 'destroy' parameters Thanks for the patch, it's looking pretty good! >diff --git a/girepository/girepository.h b/girepository/girepository.h >+typedef enum { >+ GI_SCOPE_INVALID, >+ GI_SCOPE_CALL, >+ GI_SCOPE_OBJECT, >+ GI_SCOPE_ASYNC, >+ GI_SCOPE_NOTIFIED >+} GIScope; GIScopeType is a better name I think (and GI_SCOPE_TYPE_ prefix) >diff --git a/girepository/girmodule.c b/girepository/girmodule.c >- header->major_version = 1; >+ header->major_version = 2; > header->minor_version = 0; This shouldn't be needed. We're not promising any compatibility. If you're using this in your bindings, just bump the minor version. Don't forgot to update the text document for the specs. >diff --git a/girepository/girnode.c b/girepository/girnode.c >- size = 12; >+ size = 16; >- *offset += 8; >+ *offset += 12; Can these be updated to use sizeof(SomeStruct) instead? >+ gboolean has_closure; >+ gboolean has_destroy; >+ GIScope scope; >+ >+ gint closure; >+ gint destroy; What about setting closure to -1 if it's not present? 32 bit seems a bit excessive. In practice there are few functions with more than 16 parameters. So maybe 8+8 bits, just to be sure? >- if (header->major_version != 1 || header->minor_version != 0) >+ if (header->major_version != 2 || header->minor_version != 0) Ditto, see above. >diff --git a/giscanner/ast.py b/giscanner/ast.py > class Parameter(TypeContainer): >+ self.closure_name = closure_name >+ self.destroy_name = destroy_name These two are unused, you can remove them. >diff --git a/giscanner/transformer.py b/giscanner/transformer.py >+ def _type_is_callback(self, type): >+ return (isinstance(type, Callback) \ >+ or isinstance(self._typedefs_ns.get(type.name), Callback)) No need for the \. Would probably help to just use ifs instead. >+ def _augment_callback_params(self, params): >+ >+ def handle_closure(param, i, x): This is not really how you normally do things in python, would be better if it was refactored into methods. >+ if x.type.name == 'any' and x.name == 'user_data': >+ param.closure_name = x.name >+ param.closure_index = i >+ return True >+ return False i = param_index x = closure_param >+ >+ def handle_destroy(param, i, x): Ditto >+ j = i + 1 >+ if j == len(params): >+ continue >+ had_closure = handle_closure(param, j, params[j]) >+ had_destroy = handle_destroy(param, j, params[j]) >+ j += 1 >+ if j == len(params) or not (had_closure or had_destroy): >+ continue >+ if not had_closure: >+ handle_closure(param, j, params[j]) >+ if not had_destroy: >+ handle_destroy(param, j, params[j]) I don't really understand this part. Could you add a comment explaining why that is done?
(In reply to comment #21) > (From update of attachment 124059 [details] [review] [edit]) [..] > >+ gboolean has_closure; > >+ gboolean has_destroy; > >+ GIScope scope; > >+ > >+ gint closure; > >+ gint destroy; > > What about setting closure to -1 if it's not present? > 32 bit seems a bit excessive. In practice there are few functions > with more than 16 parameters. So maybe 8+8 bits, just to be sure? Actually, this comment was meant for the gtypelib.h, but the the same thing apply there. Memory usage is so not relevant in girnode.h, as it is only used during build time.
We should do sizeof() changes in independent patches I think. Regarding the version bump - I like it personally because I've had to track down some weird bugs and crashes that were caused by having a stale typelib around.
(In reply to comment #21) > (From update of attachment 124059 [details] [review] [edit]) > >+ if x.type.name == 'any' and x.name == 'user_data': probably want x.name.endswith('data'). There are lots of "gpointer data", "gpointer callback_data", "gpointer frobnicate_operation_data", etc
Created attachment 124118 [details] [review] New version of the patch, with various tweaks as suggested by jdahlin > GIScopeType is a better name I think (and GI_SCOPE_TYPE_ prefix) > Changed. > >diff --git a/girepository/girmodule.c b/girepository/girmodule.c > > >- header->major_version = 1; > >+ header->major_version = 2; > > header->minor_version = 0; > > This shouldn't be needed. We're not promising any compatibility. > Not changed yet, as walters disagrees. > Don't forgot to update the text document for the specs. > Done. > >diff --git a/girepository/girnode.c b/girepository/girnode.c > > >- size = 12; > >+ size = 16; > > >- *offset += 8; > >+ *offset += 12; > > Can these be updated to use sizeof(SomeStruct) instead? > Not changed, should go into seperate patch (as walters noted). > What about setting closure to -1 if it's not present? > 32 bit seems a bit excessive. In practice there are few functions > with more than 16 parameters. So maybe 8+8 bits, just to be sure? > Ok, changed both in ArgBlob and GIrNodeParam. > >+ self.closure_name = closure_name > >+ self.destroy_name = destroy_name > > These two are unused, you can remove them. > Done. > >+ def _type_is_callback(self, type): > >+ return (isinstance(type, Callback) \ > >+ or isinstance(self._typedefs_ns.get(type.name), Callback)) > > No need for the \. Would probably help to just use ifs instead. > Changed. > > >+ def _augment_callback_params(self, params): > >+ > >+ def handle_closure(param, i, x): > > This is not really how you normally do things in python, would be better if > it was refactored into methods. > > ... > > I don't really understand this part. > Could you add a comment explaining why that is done? > Comments added, nested funcs made into methods.
(In reply to comment #24) > (In reply to comment #21) > > (From update of attachment 124059 [details] [review] [edit] [edit]) > > >+ if x.type.name == 'any' and x.name == 'user_data': > > probably want x.name.endswith('data'). There are lots of "gpointer data", > "gpointer callback_data", "gpointer frobnicate_operation_data", etc > Indeed; I'll shortly post an update of the patch that does this, along with a matching testcase.
Comment on attachment 124118 [details] [review] New version of the patch, with various tweaks as suggested by jdahlin This looks good. My C is not good enough to know if -1 makes sense to refer when the data type is unsigned. Maybe 255 is better? Anyway, you can commit it as it is. The annotation wiki page should also be updated, do it yourself or remind me to do so in the future.
The patch seems to work well, thanks a lot for implementing this. A minor issue I've noticed is that it sets 'destroy' even without 'closure' if there is a GDestroyNotify parameter but no user_data parameter. An example where this happens is g_memory_output_stream_new in GIO.
(In reply to comment #27) > (From update of attachment 124118 [details] [review] [edit]) > This looks good. > My C is not good enough to know if -1 makes sense to refer when the data type > is unsigned. Maybe 255 is better? > Anyway, you can commit it as it is. > I'm using gint8, not guint8, so this should not be an issue (unless you want to have guint8...). > The annotation wiki page should also be updated, do it yourself or remind me to > do so in the future. > I'll do so when the code is in the repo.
2008-01-03 Andreas Rottmann <a.rottmann@gmx.at> Bug 556489 – callback annotations * giscanner/transformer.py * tools/generate.c (write_callable_info): Write out the new scope, closure and destroy attributes. * giscanner/transformer.py (Transformer._type_is_callback): New method, checking if a given type is a callback. (Transformer._augment_callback_params): New method; adds information (closure, destroy) to callback parameters. (Transformer._handle_closure, Transformer._handle_destroy): New methods, auxiliary to _augment_callback_params. (Transformer._create_function): Call _augment_callback_params(). (Transformer._create_parameter): Handle scope option. (Transformer._create_typedef_callback): New method, creates a callback, and registers it in the typedef namespace (Transformer._create_typedef): Use _create_typedef_callback() instead of the plain _create_callback(). * giscanner/ast.py (Parameter): Added callback-related fields. * giscanner/girwriter.py: Write out new Parameter fields. * girepository/girnode.h (GIrNodeParam): Added fields scope, closure and destroy. * girepository/gtypelib.h (ArgBlob): Ditto. * girepository/girparser.c (start_parameter): Handle new fields. * girepository/girmodule.c (g_ir_module_build_typelib): Adjust arg_blob_size, bump major version due to this change. * girepository/girnode.c (g_ir_node_get_full_size_internal) (g_ir_node_build_typelib) * girepository/gtypelib.c (g_typelib_check_sanity): ArgBlob size adjustments. (g_ir_node_build_typelib): Fill in new ArgBlob flags from param. * girepository/girepository.h (GIScope): New enumeration, listing the different possible scopes for callbacks. * girepository/ginfo.c (g_arg_info_get_scope) (g_arg_info_get_closure, g_arg_info_get_destroy): Accessors for callback-related argument indices (callback scope, closure for a callback, destroy notification for a callback). * tests/scanner/: Added testcases for new features. Fixed in r998.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]