GNOME Bugzilla – Bug 602417
Document lifecycles of GSimpleAsyncResult and friends
Last modified: 2010-09-27 15:59:12 UTC
I just noticed that some code I'm writing using GAsync* is leaking a bunch of stuff. Turns out that I'm leaking every GSimpleAsyncResult, mainly because I wasn't sure where I was meant to be discarding my reference. I've discovered by asking around that I can unref it and throw it away after calling _complete() or _complete_in_idle() and the right thing will happen. It would be useful if the preamble to GSimpleAsyncResult gave an example of the typical ways one implements foo_async and foo_finish using it. Similarly, I'm still a little unsure about who owns which references when I implement GAsyncInitable, so a known-good example of implementing that for a simple object would be nice.
Created attachment 148131 [details] [review] Give an example of using GSimpleAsyncResult Here's a first cut at a GSAR example.
Created attachment 148172 [details] [review] Apply English pedantry to GAsyncInitable's docs • "asynchronous" was misspelled as "asyncronous" in various places; • punctuation was missing; • g_async_initable_new_async() had a stray "and"; • references to g_async_initable_new_finish() were missing a "the".
Created attachment 148173 [details] [review] Document a simple implementation of GAsyncInitable
Review of attachment 148173 [details] [review]: ::: gio/gasyncinitable.c @@ +69,3 @@ + * + * if (self->priv->success) + * g_simple_async_result_set_op_res_gboolean (simple, TRUE); I don't think you need to set a boolean here - we always want to set an error on non-success. So your finish function should just do if (g_simple_async_result_propagate_error (simple, error)) return FALSE; return TRUE;
Review of attachment 148131 [details] [review]: ::: gio/gsimpleasyncresult.c @@ +153,3 @@ + * BAKER_ERROR_TOO_SMALL, + * "%ucm radius cakes are silly", + * radius); There's a problem with g_simple_async_report_error_in_idle() (and other functions) insofar that it doesn't allow you to set the tag. (I think _report_error_in_idle(), _report_gerror_in_idle() and _new_error() should probably be deprecated?). Suggest to create the GSimpleAsyncResult up-front and use g_simple_async_result_set_error() instead.
Review of attachment 148131 [details] [review]: It wouldn't hurt to add notes to the _complete() type calls that they take a reference to the result for as long as is needed to complete the call. Similar notes would be nice for the functions that take a GError*. ::: gio/gsimpleasyncresult.c @@ +129,3 @@ + * else + * g_simple_async_result_set_op_res_gpointer (result, + * g_object_ref (cake), you should note at the top of baked_cb that the callback is not given its own reference to 'cake' and that cake would normally be destroyed after baked_cb returned. @@ +153,3 @@ + * BAKER_ERROR_TOO_SMALL, + * "%ucm radius cakes are silly", + * radius); I've always been slightly annoyed by this problem. I tollerate it, though, because I really enjoy the convenience of using these functions (and I think that good example code should make use of them). Maybe we could change g_simple_async_result_is_valid() to always pass if the source tag is NULL. @@ +166,3 @@ + * { + * g_simple_async_result_set_op_res_gpointer (simple, + * g_object_ref (cake), this time you're definitely leaking the cake (assuming the cache returns a reference -- which really, it ought to). either pass 'cake' directly in, with a note that you're "transfering your reference", or better: g_object_unref(cake) before you return. @@ +184,3 @@ + * Cake *cake; + * + * if (!g_simple_async_result_propagate_error (simple, error)) logic here is backwards. this function returns TRUE if there is an error. @@ +188,3 @@ + * + * g_return_val_if_fail (g_simple_async_result_is_valid (result, + * G_OBJECT (self), you should move this call all the way up to the top -- even before your G_SIMPLE_ASYNC_RESULT() cast. part of the purpose of this function is to check that the 'result' is a valid GSimpleAsyncResult, so you should do it before you do anything under that assumption.
Review of attachment 148173 [details] [review]: i don't feel comfortable making more detailed comments on this patch because i don't fully understand how this stuff works myself :) ::: gio/gasyncinitable.c @@ +69,3 @@ + * + * if (self->priv->success) + * g_simple_async_result_set_op_res_gboolean (simple, TRUE); it's true. conceptually, this C prototype: gboolean do_foo (..., GError **error); is equivalent to this higher-level language declaration: void do_foo (...) throws GLib.Error; so really, you're not returning a bool -- you're returning nothing.
Review of attachment 148131 [details] [review]: ::: gio/gsimpleasyncresult.c @@ +132,3 @@ + * g_object_unref); + * + * g_simple_async_result_complete (result); Calling g_simple_async_result_complete() here is correct if baked_cb is called as a callback from the mainloop, which is most likely. However, if its not you need to call complete_from_idle(), which would be nice to point out. @@ +153,3 @@ + * BAKER_ERROR_TOO_SMALL, + * "%ucm radius cakes are silly", + * radius); I don't agree. These functions are very convenient, and its a standard pattern in case where you're implementing a virtual function like GFile or GInputStream that the _finish() function handles the error case so that all implementations don't have to do that. This means it would not work to set the tag. @@ +193,3 @@ + * + * cake = CAKE (g_simple_async_result_get_op_res_gpointer (simple)); + * return g_object_ref (cake); This is all correct and stuff, but it might be nice to have a more wider description on the exact requirements of the finish function. First of all, its only valid for it to be called once, so it is allowed to "steal" the object from the async result instead of returning a copy/ref. This is useful if the constructed object isn't refcounted as that allows you to avoid a copy. Its also allowed to *not* call the finish function from the callback. So, implementations must handle this by freeing the created objects when the async result is freed. Its also allowed to ref the async result from the callback and call the finish function later.
Review of attachment 148173 [details] [review]: I think this example is a bit contrived, with construction state stored in the object and whatnot. A better example would be something that does a single async operation in the initialization, like for example: http://cgit.freedesktop.org/~david/gdbus-standalone/tree/gdbus/gdbusproxy.c
(In reply to comment #9) > A better example would be something that does a single async operation in the > initialization, like for example: > http://cgit.freedesktop.org/~david/gdbus-standalone/tree/gdbus/gdbusproxy.c Well, that code doesn't implement g_async_initable_init_async() correctly. :-) According to the documentation, it must be idempotent, but if you init a GDBusProxy twice it will call GetAll() twice and leak one hash table of properties.
Created attachment 150558 [details] [review] g_simple_async_result_is_valid: allow tag to be NULL Because g_simple_async_report_[g]error_in_idle() don't take a source tag parameter, code that uses them can't currently use g_simple_async_result_is_valid() (at least, not for the error case). So, let's make g_simple_async_result_is_valid() pass if the tag is NULL.
Comment on attachment 150558 [details] [review] g_simple_async_result_is_valid: allow tag to be NULL (sorry, git bz tricked me, but i hit ^C too late)
Created attachment 150559 [details] [review] g_simple_async_result_is_valid: allow tag to be NULL Because g_simple_async_report_[g]error_in_idle() don't take a source tag parameter, code that uses them can't currently use g_simple_async_result_is_valid() (at least, not for the error case). So, let's make g_simple_async_result_is_valid() pass if the tag is NULL.
Created attachment 150560 [details] [review] Give an example of using GSimpleAsyncResult
Created attachment 150561 [details] [review] Document that _complete() et al. ref the GSimpleAsyncResult
Created attachment 150562 [details] [review] Document that _finish() may be called at most once. In addition, rephrase the relevant paragraphs to make them a bit clearer.
Created attachment 150563 [details] [review] Correct a typo in GAsyncResult's documentation
Created attachment 154218 [details] [review] [PATCH 2/3] Give an example of using GSimpleAsyncResult This updated patch includes a note about why baked_cb() can use _complete(), and when you should instead use _complete_in_idle().
Created attachment 154219 [details] [review] Document a simple implementation of GAsyncInitable An updated version which addresses the comments about not needing to set a boolean on the GSimpleAsyncResult.
The outstanding three patches are visible at http://git.collabora.co.uk/?p=user/wjt/glib;a=shortlog;h=refs/heads/document-async if you're more into that kind of thing. The first example depends on the patch to make _is_valid() accept NULL tags; the second example is stand-alone. I haven't changed the example to chain down to another async operation; I could do that, but I don't think it would necessarily make the example any clearer.
These should be all committed now.Thanks!
Awesome, thanks. :)
(In reply to comment #13) > Created an attachment (id=150559) [details] [review] > g_simple_async_result_is_valid: allow tag to be NULL > > Because g_simple_async_report_[g]error_in_idle() don't take a source tag > parameter, code that uses them can't currently use > g_simple_async_result_is_valid() (at least, not for the error case). > So, let's make g_simple_async_result_is_valid() pass if the tag is > NULL. As discuss today with Will, this patch does not do 100% what he expected. The current patch let you disable tag checking by passing NULL to the tag parameter. But when using g_simple_async_report_* it's the tag inside the GSimpleAsyncResult that is NULL. Which mean, to achieve the initial goal we also need to disable tag checking if the result does not holds a source_tag (e.g. when you are using g_simple_async_report_*). I'll post a patch in few minutes that enable this.
Created attachment 171219 [details] [review] Allow stored tag to be NULL in GSimpleAsyncResult Allow stored tag to be NULL in GSimpleAsyncResult This allow using g_simple_async_report_(error|gerror)_in_idle() with g_simple_async_result_is_valid() giving a non-NULL source tag. With this change, _is_valid() will ignore the source tag if the GSimpleAsyncResult was created without one.