GNOME Bugzilla – Bug 783825
Suggest that asynchronous operations should invoke the callback in a later iteration of the GMainLoop
Last modified: 2017-10-26 12:09:14 UTC
The documentation doesn't explicitly say that the GAsyncReadyCallback should not be invoked in the same iteration of the GMainLoop from which the asynchronous operation was called. Having the callback invoked immediately will be confusing for the user code. It wouldn't know if the async operation will proceed to some extent and then return in a later iteration, or whether it would take a short cut and return immediately. There is code in the wild written on the assumption that the callback might be invoked immediately. See: https://gitlab.com/libosinfo/libosinfo/blob/master/osinfo/osinfo_install_script.c#L1167 The idea is that osinfo_install_script_generate_async() might invoke its callback before g_main_loop_run() was reached. Hence the g_main_loop_is_running() guard. GTask guarantees that the invocation happens in a later iteration, but GSimpleAsyncResult is a bit lax. On closer inspection, g_simple_async_report_* methods invoke the callback in an idle handler. Since one of the motivations to return immediately are error conditions detected right at the beginning, which these methods seem to be aimed at, it appears that the API tried to hint at this being the right thing to do. It will be good to clarify this in the documentation. Although, I am not sure what would be the best place to do so.
(In reply to Debarshi Ray from comment #0) > There is code in the wild written on the assumption that the callback might > be invoked immediately. See: > https://gitlab.com/libosinfo/libosinfo/blob/master/osinfo/ > osinfo_install_script.c#L1167 I think in general we have to assume that arbitrary API which uses GAsyncReadyCallback can call the callback synchronously or after one or more main context iterations. Since there have been no restrictions on how GAsyncReadyCallback is used since it was first introduced, there can be code in the wild which does anything. > The idea is that osinfo_install_script_generate_async() might invoke its > callback before g_main_loop_run() was reached. Hence the > g_main_loop_is_running() guard. Aside: I find it’s clearer (and functionally equivalent) to use some explicit termination conditions and g_main_context_iteration() for making an async operation synchronous: --- g_autoptr(GAsyncResult) result = NULL; blah_blah_async (obj, async_result_cb, &result); while (result == NULL) g_main_context_iteration (NULL, TRUE); success = blah_blah_finish (obj, result, error); static void async_result_cb (GObject *obj, GAsyncResult *result, gpointer user_data) { GAsyncResult **result_out = user_data; *result_out = g_object_ref (result); } --- That avoids race conditions and makes the loop and its exit condition more explicit. Calling g_main_context_iteration() in a loop is basically what g_main_loop_run() does. > It will be good to clarify this in the documentation. Although, I am not > sure what would be the best place to do so. I would be happy to review a patch to the GAsyncReadyCallback documentation which said something like: > GAsyncReadyCallback callbacks from GTask are guaranteed to be invoked from an idle source in the thread-default main context where the GTask was created. No guarantees are made about callbacks from non-GTask APIs. They may be called synchronously, or from an idle source.
(In reply to Philip Withnall from comment #1) > (In reply to Debarshi Ray from comment #0) > > There is code in the wild written on the assumption that the callback might > > be invoked immediately. See: > > https://gitlab.com/libosinfo/libosinfo/blob/master/osinfo/ > > osinfo_install_script.c#L1167 > > I think in general we have to assume that arbitrary API which uses > GAsyncReadyCallback can call the callback synchronously or after one or more > main context iterations. Since there have been no restrictions on how > GAsyncReadyCallback is used since it was first introduced, there can be code > in the wild which does anything. Arbitrary code can definitely do whatever it wants. My point is to suggest some best practices to abide by when implementing GAsyncResult based asynchronous operations. For example, the general expectation is that async_result_cb will be invoked in the thread's default GMainContext at the point where this call is made: blah_blah_async (obj, async_result_cb, data); Some async operations clearly mention this in their documentation (eg., g_file_copy_async) but most don't. I think that it works better if specific implementations document any odd deviation from the idiomatic behaviour, instead of repeating the same thing many times over. Again GTask does a better job at this than GSimpleAsyncResult, but even though one could do odd things with it, g_simple_async_result_complete suggests not using it from a worker thread. I guess, I'd have been happier if it also suggested not calling it from the same iteration of the GMainLoop from which the operation was initiated. > I would be happy to review a patch to the GAsyncReadyCallback documentation > which said something like: > > GAsyncReadyCallback callbacks from GTask are guaranteed to be invoked from > an idle source in the thread-default main context where the GTask was > created. Ok. > No guarantees are made about callbacks from non-GTask APIs. They > may be called synchronously, or from an idle source. I am not sure about this part. The GAsyncResult interface already makes an effort to describe the structure of an async operation and certain expectations. Maybe we should add these bits there? For example, an async operation might decide not to invoke the callback on cancellation, or return some other exception than (G_IO_ERROR, G_IO_ERROR_CANCELLED). But it is convenient for users of such APIs to be able to assume that unless otherwise noted, things will adhere to the idiomatic form.
(In reply to Debarshi Ray from comment #2) > For example, an async operation might decide not to invoke the callback on > cancellation, or return some other exception than (G_IO_ERROR, > G_IO_ERROR_CANCELLED). But it is convenient for users of such APIs to be > able to assume that unless otherwise noted, things will adhere to the > idiomatic form. If you think there are certain things missing from the description of the standard behaviour of async functions, please add them. Just be careful to not make any promises about API which we can’t keep due to those promises not being there when GAsyncResult was initially introduced.
Created attachment 353898 [details] [review] docs: Explain the invocation of GAsyncReadyCallbacks from GTask
Created attachment 353899 [details] [review] docs: Clarify the thread-default GMainContext where the result is sent
Created attachment 353900 [details] [review] docs: Don't be vague about where GTask dispatches the result
Created attachment 353901 [details] [review] docs: Mention the idiomatic way of invoking a GAsyncReadyCallback
> It will be good to clarify this in the documentation. Although, I am not sure > what would be the best place to do so. gasyncresult.c is where the high-level concepts of gio asyncness are documented > No guarantees are made about callbacks from non-GTask APIs. They > may be called synchronously, or from an idle source. FWIW I disagree with this. An API that calls a GAsyncReadyCallback synchronously from its "start" function is buggy. If there are APIs that do this then people can work around that bug, just as they might work around other bugs, but it should definitely be considered a bug in the API.
Dan, what do you think about the attached patches? Philip?
Review of attachment 353898 [details] [review]: ::: gio/giotypes.h @@ +269,3 @@ + * iteration of the + * [thread-default main context][g-main-context-push-thread-default] + * where the #GTask was created. Maybe expand this to say that “All other users of #GAsyncReadyCallback must likewise call it asynchronously in a later iteration of the main context.”
Review of attachment 353899 [details] [review]: ++
Review of attachment 353900 [details] [review]: ++
Review of attachment 353901 [details] [review]: ::: gio/gasyncresult.c @@ +37,3 @@ * an asynchronous operation, provide a #GAsyncReadyCallback to the * asynchronous function. This callback will be triggered when the + * operation has completed, and is usually run in a later iteration of Dan changed my mind. I think we should phrase this as ‘and must be run in a later iteration’, and just treat old code which doesn’t do this as buggy. Saying ‘is usually’ doesn’t give the reader the information that any *modern* code *must* do that, and it’s only *older* code which *might* do it wrong.
Thanks for the reviews! (In reply to Philip Withnall from comment #10) > Review of attachment 353898 [details] [review] [review]: > > ::: gio/giotypes.h > @@ +269,3 @@ > + * iteration of the > + * [thread-default main context][g-main-context-push-thread-default] > + * where the #GTask was created. > > Maybe expand this to say that “All other users of #GAsyncReadyCallback must > likewise call it asynchronously in a later iteration of the main context.” Ok. (In reply to Philip Withnall from comment #13) > Review of attachment 353901 [details] [review] [review]: > > ::: gio/gasyncresult.c > @@ +37,3 @@ > * an asynchronous operation, provide a #GAsyncReadyCallback to the > * asynchronous function. This callback will be triggered when the > + * operation has completed, and is usually run in a later iteration of > > Dan changed my mind. I think we should phrase this as ‘and must be run in a > later iteration’, and just treat old code which doesn’t do this as buggy. > Saying ‘is usually’ doesn’t give the reader the information that any > *modern* code *must* do that, and it’s only *older* code which *might* do it > wrong. Ok.
(In reply to Debarshi Ray from comment #14) > Thanks for the reviews! Yeah, sorry about the delay. Do you have any other patches pending review?
Created attachment 362339 [details] [review] docs: Explain how GAsyncReadyCallbacks are, and should be, invoked
Created attachment 362340 [details] [review] docs: Mention the idiomatic way of invoking a GAsyncReadyCallback
Review of attachment 362339 [details] [review]: ++
Review of attachment 362340 [details] [review]: ++