GNOME Bugzilla – Bug 693576
GTask callbacks not introspection-friendly
Last modified: 2018-05-24 15:02:37 UTC
I'm updating Vala's GIO bindings, and I've noticed that g_task_run_in_thread and g_task_run_in_thread_sync aren't very useful for GObject Introspection consumers because the task data handled separately from the callback. I see two relatively simple options. The first is to add introspection-friendly versions (g_task_run_in_thread_with_data and g_task_run_in_thread_with_data_sync, perhaps) which would look something like this: void g_task_run_in_thread_with_data (GTask *task, GTaskThreadFunc task_func, gpointer task_data, GDestroyNotify task_data_destroy) { g_task_set_task_data (task, task_data, task_data_destroy); g_task_run_in_thread (task, task_func); g_task_set_task_data (task, NULL); }; (I know this would clobber existing task_data, but that wouldn't be hard to fix and you get the idea.) With some relatively straightforward annotations we could skip all the currently existing functions which deal with task_data and rename the *_with_data. The other option would be to just make GTask always work like this. I'm not sure how useful setting the task_data separately is (I haven't played with the API)... if it's not very useful I think this would be a good option, since it feels more like most other gnome-ish APIs.
(In reply to comment #0) > The other option would be to just make GTask always work like this. I'm not > sure how useful setting the task_data separately is It's used in many places that don't use run_in_thread, so this wouldn't work. g_simple_async_result_run_in_thread() is marked (skip), so I guess this issue was never solved there. The fact that @source_object, @task_data, and @cancellable are passed to the GTaskThreadFunc is just a convenience; you can get them all back by calling the appropriate g_task_get_* methods anyway. So we could make alternate versions of the run-in-thread methods that take an alternate callback type too... Is this the only introspectability issue you came across while binding GTask? If we're going to need multiple changes it would be good to think about them all together... At the time I was writing GTask, I wasn't sure that it would be usefully bindable without some explicit work on the bindings side. Eg, you would not be able to use g_task_return_pointer() directly to return an object in garbage-collected bindings. Although, one thought I had was: void g_task_return_value (GTask *task, GValue *value); gboolean g_task_propagate_value (GTask *task, GValue *value, GError **error); which I think would solve things for gjs at least, and presumably other bindings as well. g_task_attach_source() is probably also problematic... although that's just a convenience function, not a necessity.
(In reply to comment #1) > (In reply to comment #0) > > The other option would be to just make GTask always work like this. I'm not > > sure how useful setting the task_data separately is > > It's used in many places that don't use run_in_thread, so this wouldn't work. > > g_simple_async_result_run_in_thread() is marked (skip), so I guess this issue > was never solved there. > > The fact that @source_object, @task_data, and @cancellable are passed to the > GTaskThreadFunc is just a convenience; you can get them all back by calling the > appropriate g_task_get_* methods anyway. So we could make alternate versions of > the run-in-thread methods that take an alternate callback type too... Hm. I was thinking the task data was equivalent to the user_data traditionally passed to callbacks, but abandoning this idea simplifies things. Instead of a way to set task_data and run the callback, how about just adding a gpointer for user_data? That way bindings can still use that parameter to pass around stuff they need (i.e., context information for closures). Adding a second version would work, too, but I think just doing this would be fine for C as well: typedef void (*GTaskThreadFunc) (GTask *task, gpointer source_object, gpointer task_data, GCancellable *cancellable, gpointer user_data); void g_task_run_in_thread (GTask *task, GTaskThreadFunc task_func, gpointer user_data); void g_task_run_in_thread_sync (GTask *task, GTaskThreadFunc task_func, gpointer user_data); (I'm assuming scope=async would be appropriate here, otherwise a GDestroyNotify would probably be necessary as well). > Is this the only introspectability issue you came across while binding GTask? > If we're going to need multiple changes it would be good to think about them > all together... set_task_data and set_source_tag need to transfer ownership. I'm a bit uneasy about return/propogate_int/bool. I feel like you should just use GINT_TO_POINTER and GPOINTER_TO_INT just like you do for all the containers in glib, and bindings should already be able to handle it. > At the time I was writing GTask, I wasn't sure that it would be usefully > bindable without some explicit work on the bindings side. Eg, you would not be > able to use g_task_return_pointer() directly to return an object in > garbage-collected bindings. Although, one thought I had was: > > void g_task_return_value (GTask *task, GValue *value); > gboolean g_task_propagate_value (GTask *task, GValue *value, GError **error); > > which I think would solve things for gjs at least, and presumably other > bindings as well. This isn't really a problem for Vala, so I'm not really the best person to ask. I'll CC Colin, but I think if they can handle set_task_data and get_task_data correctly they should be able to handle this (which, judging by the fact that they aren't marked as introspectable=false in the GIR, they can). > g_task_attach_source() is probably also problematic... although that's just a > convenience function, not a necessity. Yes it is. The only thing I can think of would be creating a new callback type and having the real GSourceFunc just invoke that with whatever params you want.
(In reply to comment #2) > Adding a second version > would work, too, but I think just doing this would be fine for C as well: I guess new-API freeze isn't until next week, but I still feel like it might be too late to make incompatible changes... Anyone else have opinions? > (I'm assuming scope=async would be appropriate here, otherwise a GDestroyNotify > would probably be necessary as well). mmm... that depends on exactly what we guarantee about scope=async... if it just means "the function will be run exactly once", then yes, but if it also implies "the function will not be run until after the caller returns", then some bindings might do things that would mess up if we claimed scope=async here. > set_task_data and set_source_tag need to transfer ownership. The source_tag is really just an integer that is represented as a pointer... it never gets dereferenced, so there's not really any "ownership" to it. Not sure how to best represent this... I'm not sure what the right annotation is for set_task_data() either, since it already takes a GDestroyNotify... are there other examples of similar APIs that have a transfer annotation? > I'm a bit uneasy about return/propogate_int/bool. I feel like you should just > use GINT_TO_POINTER and GPOINTER_TO_INT just like you do for all the containers > in glib, and bindings should already be able to handle it. I think a GValue-based version would be cleaner... (and would let you return larger-than-int-sized types) > > g_task_attach_source() is probably also problematic... although that's just a > > convenience function, not a necessity. > > Yes it is. The only thing I can think of would be creating a new callback type > and having the real GSourceFunc just invoke that with whatever params you want. But then you'd be losing whatever params the real callback was trying to pass you (eg, the GIOCondition). I guess g_task_attach_source_with_closure() would handle this...
Created attachment 235889 [details] [review] GTask: add GValue-based return/propagate methods, for bindings Since g_task_return_pointer() / g_task_propagate_pointer() isn't necessarily bindings-friendly, add GValue-based versions as well.
(In reply to comment #3) > (In reply to comment #2) > > Adding a second version > > would work, too, but I think just doing this would be fine for C as well: > > I guess new-API freeze isn't until next week, but I still feel like it might be > too late to make incompatible changes... Anyone else have opinions? FWIW the result is the same in introspection and Vala. We can just hide run_in_thread and rename run_in_thread_with_data (or whatever) to run_in_thread. > > (I'm assuming scope=async would be appropriate here, otherwise a GDestroyNotify > > would probably be necessary as well). > > mmm... that depends on exactly what we guarantee about scope=async... if it > just means "the function will be run exactly once", then yes, but if it also > implies "the function will not be run until after the caller returns", then > some bindings might do things that would mess up if we claimed scope=async > here. Nope, it only implies the func is run exactly once. scope=async should be fine. > > set_task_data and set_source_tag need to transfer ownership. > > The source_tag is really just an integer that is represented as a pointer... it > never gets dereferenced, so there's not really any "ownership" to it. Not sure > how to best represent this... Eh, probably as it is. It's likely to crash unless people manually keep a reference, though, and people used to higher-level languages aren't used to thinking about this kind of stuff. Making it transfer ownership without also passing a GDestroyNotify will always leak, though... > I'm not sure what the right annotation is for set_task_data() either, since it > already takes a GDestroyNotify... are there other examples of similar APIs that > have a transfer annotation? I found a few examples of similar APIs which /don't/ have a transfer annotation, but AFAICT they're all marked as not introspectable in the GIR (g_object_set_data_full, g_object_set_qdata_full, pango_attr_shape_new_with_data, g_simple_async_result_set_op_res_gpointer). I guess this just isn't supported in introspection right now. That said, it /does/ transfer ownership, so it would be nice to annotate it that way for Vala, increased clarity in the C docs, and in case G-I ever does support it. > > I'm a bit uneasy about return/propogate_int/bool. I feel like you should just > > use GINT_TO_POINTER and GPOINTER_TO_INT just like you do for all the containers > > in glib, and bindings should already be able to handle it. > > I think a GValue-based version would be cleaner... (and would let you return > larger-than-int-sized types) This does make things accessible from G-I consumers other than Vala.
I guess g_task_set_task_data() is mostly just making up for the fact that C doesn't have closures or function pointers with an implicit "this"; if you've got one of those, you don't really need task_data, in the same way you don't need user_data.
Created attachment 236108 [details] [review] GTask: add more bindings-friendly run-in-thread methods GTaskThreadFunc/g_task_run_in_thread/g_task_run_in_thread_sync have parameters that aren't very bindings-friendly. So add alternate versions with more traditional parameters.
Created attachment 236109 [details] [review] GTask: add properties Add properties to GTask to make it more bindings-friendly.
Created attachment 236110 [details] [review] GTask: add a closure-based version of g_task_attach_source
I'm not really sure about the last one (g_task_attach_source_with_closure), especially the part where it ends up having slightly different semantics than the plain C version.
Comment on attachment 236110 [details] [review] GTask: add a closure-based version of g_task_attach_source As mentioned above, I don't like this patch since the new function has subtly different semantics from the original.
Comment on attachment 236108 [details] [review] GTask: add more bindings-friendly run-in-thread methods Thinking about this more, the need for this function is really Vala-specific; gtkmm would be fine with the current functions, and everything else wouldn't be able to deal with having the callback be run in a different thread. So, I think if the existing run_in_thread methods don't work well for Vala, then Vala should override them itself (which maybe it has already done in the two years since this bug was filed...). Does anyone know of any examples of using GTask (or even GSimpleAsyncResult) from a language binding? I wasn't able to find any...
Created attachment 338067 [details] [review] task: Add scope to run_in_thread() and run_in_thread_sync() This is required by GObject-Introspection and has been verified to work with PyGObject. gir/gio-2.0.c:38589: Warning: Gio: g_task_run_in_thread: argument task_func: Missing (scope) annotation for callback without GDestroyNotify (valid: call, async)
Created attachment 338068 [details] [review] task: Add return/propagate API for GValue This is an alternative version which just uses the pointer variant with g_value_unset().
Created attachment 338112 [details] [review] task: Add return/propagate API for GValue v2 This changes the GValue to be (transfer none) which is more compatible with bindings as a GValue can be allocated with malloc() or GSlice. This patch now includes a test case and has been tested to work with PyGObject.
Review of attachment 338112 [details] [review]: This patch looks good to me. Any GLib maintainers mind taking a quick review? This would help us a bunch in Builder to simplify async operations in our plugins.
Comment on attachment 338112 [details] [review] task: Add return/propagate API for GValue v2 Is there any problem with my original patch? Or did you just write the patch first and then discover my old patch here? >+ if (result == NULL) >+ { >+ g_value_init (value, G_TYPE_POINTER); >+ g_value_set_pointer (value, NULL); >+ } Is this necessary for pygobject? It seems to me that even if you want to return NULL, you would still be thinking in terms of a particular type (and so should pass a GValue* of the right type with a NULL v_pointer). >+ if (g_task_propagate_error (task, error)) >+ { >+ g_value_init (value, G_TYPE_POINTER); >+ g_value_set_pointer (value, NULL); >+ return FALSE; >+ } Again, is this needed for pygobject? Normally when functions set a GError, you're not allowed to assume that out parameters have been initialized, so this shouldn't be needed.
(In reply to Dan Winship from comment #17) > Comment on attachment 338112 [details] [review] [review] > task: Add return/propagate API for GValue v2 > > Is there any problem with my original patch? Or did you just write the patch > first and then discover my old patch here? > I wrote my original patch before looking at the bug and modified it after seeing your patch. This mainly reduces the amount of changes to something easier to review and hopefully get committed. > >+ if (result == NULL) > >+ { > >+ g_value_init (value, G_TYPE_POINTER); > >+ g_value_set_pointer (value, NULL); > >+ } > > Is this necessary for pygobject? It seems to me that even if you want to > return NULL, you would still be thinking in terms of a particular type (and > so should pass a GValue* of the right type with a NULL v_pointer). > PyGObject will automatically create a GValue for you, however in this case it just passes NULL. > >+ if (g_task_propagate_error (task, error)) > >+ { > >+ g_value_init (value, G_TYPE_POINTER); > >+ g_value_set_pointer (value, NULL); > >+ return FALSE; > >+ } > > Again, is this needed for pygobject? Normally when functions set a GError, > you're not allowed to assume that out parameters have been initialized, so > this shouldn't be needed. No it shouldn't be needed, I did it initially because sometimes out args are set to dummy/error values (i.e. NULL). But in this cause it would require all users to unset() the value on error which is likely to be forgotten. I'll remove it in an update.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/668.