GNOME Bugzilla – Bug 764163
g_task_had_error doesn't remember the error after g_task_propagate_*
Last modified: 2016-05-04 07:36:32 UTC
The use of past tense in the method name (ie. g_task_had_error) and in the documentation (ie. "Tests if task resulted in an error") makes one assume that it won't forget about any errors that might have occured. Except, in reality, it does, once you have called any of the g_task_propagate_* methods. We should either fix the code or the documentation.
Created attachment 324696 [details] [review] gtask: Don't forget about the error after g_task_propagate_*
Created attachment 324698 [details] [review] gio/tests/task: Ensure that g_task_had_error doesn't forget the error
Review of attachment 324696 [details] [review]: I don't know if that matters, but this will change behaviour of calling g_task_propagate_error() twice on the same GTask (before, the second call was returning FALSE and no GError set, after this patch, it will return TRUE and set a GError),
(In reply to Christophe Fergeau from comment #3) > Review of attachment 324696 [details] [review] [review]: > > I don't know if that matters, but this will change behaviour of calling > g_task_propagate_error() g_task_propagate_error is not public API, and its internal users didn't appear to be unduly affected. > twice on the same GTask (before, the second call > was returning FALSE and no GError set, after this patch, it will return TRUE > and set a GError), Hmm... g_task_propagate_pointer is public and is supposed to be only called once: https://developer.gnome.org/gio/stable/GTask.html#g-task-propagate-pointer
I think this is a documentation bug, unless you have a strong use case for having it keep working after the propagate call? The expected use case was mostly for when the return-value-on-error is also a valid return value on non-error (eg, GSList *get_foo() returns NULL on error, but will also return NULL if the list is empty), and you have to distinguish between the cases in the finish function. Eg: GError *my_error = NULL; gpointer ret; ret = g_task_propagate_pointer (task, &my_error); if (my_error) { // do some cleanup here or whatever ... g_error_propagate (error, my_error); } return ret; Using g_task_had_error() lets you simplify that to: if (g_task_had_error (task)) { // do some cleanup here or whatever ... } return g_task_propagate_pointer (task, error);
Here is an example, but I can live with replacing g_task_had_error with "if (array == NULL)": gboolean foo_object_do_something_finish (FooObject *self, GAsyncResult *res, gsize *out_var_1, gsize *out_var_2, GError **error) { GTask *task = G_TASK (res); gboolean ret_val = FALSE; gsize *array; /* g_return_val_if_fail guards */ array = g_task_propagate_pointer (task, error); if (g_task_had_error (task)) goto out; ret_val = TRUE; if (out_var_1 != NULL) *out_var_1 = array[0]; if (out_var_2 != NULL) *out_var_2 = array[1]; out: return ret_val; }
I don't like keeping the error around. If there absolutely is a strong case for using had_error() after stealing the GError away, I'd prefer we had a separate boolean for it. We could also entertain the (strange) possibility of allowing: g_task_return_error (task, NULL); I'd also be OK with a docs clarification.
Created attachment 326948 [details] [review] gtask: Don't forget about the error after g_task_propagate_*
Comment on attachment 326948 [details] [review] gtask: Don't forget about the error after g_task_propagate_* hm... odd, but I guess it simplifies things since error could be set from multiple places
Comment on attachment 324698 [details] [review] gio/tests/task: Ensure that g_task_had_error doesn't forget the error Pushed to master.
Comment on attachment 326948 [details] [review] gtask: Don't forget about the error after g_task_propagate_* Pushed to master.