GNOME Bugzilla – Bug 764157
Port to GTask from GSimpleAsyncResult
Last modified: 2019-07-30 12:37:59 UTC
GTask has existed as a replacement for GSimpleAsyncResult since glib-2.36. In glib-2.46 GSimpleAsyncResult was marked as deprecated, which leads to a lot of compile-time warnings. Now is a good time to port things to use GTask. Christophe has an initial set of patches at: https://gitlab.com/teuf/gnome-online-account/tree/gtask
Created attachment 324693 [details] [review] Port goa_provider_ensure_credentials() to GTask
Review of attachment 324693 [details] [review]: ::: src/goabackend/goaprovider.c @@ +735,3 @@ + gint *out_expires_in, + GAsyncResult *res, + GError **error) Nitpick: would be nicer to do it in a separate commit. @@ -746,3 @@ g_return_val_if_fail (error == NULL || *error == NULL, FALSE); - g_warn_if_fail (g_simple_async_result_get_source_tag (simple) == goa_provider_ensure_credentials); Can't we continue to do the same with g_task_get_source_tag? @@ +749,3 @@ + ret = g_task_propagate_boolean (task, error); + if (g_task_had_error (task)) This is affected by glib bug 764163
Created attachment 324699 [details] [review] Port goa_provider_get_all() to GTask
Review of attachment 324699 [details] [review]: ::: src/goabackend/goaprovider.c @@ +1222,3 @@ + g_return_val_if_fail (g_task_is_valid (result, NULL), FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); Nitpick: The assertion about 'error' should ideally be in a separate patch. @@ +1226,3 @@ + task = G_TASK (result); + providers = g_task_propagate_pointer (task, error); + if (g_task_had_error (task)) This is affected by bug 764163
We can try to work around bug 764163 by putting the g_task_had_error before calling g_task_propagate_*
Created attachment 324784 [details] [review] Reindent goa_provider_ensure_credentials_finish parameters
Created attachment 324785 [details] [review] Port goa_provider_ensure_credentials() to GTask Still untested, but should address your comments. I'll modify the rest of my patches accordingly (wrt get_source_tag() and had_error()) during the week-end (fingers crossed!)
Review of attachment 324693 [details] [review]: ::: src/goabackend/goaprovider.c @@ -746,3 @@ g_return_val_if_fail (error == NULL || *error == NULL, FALSE); - g_warn_if_fail (g_simple_async_result_get_source_tag (simple) == goa_provider_ensure_credentials); Most likely. Since it's an additional call after g_task_new() (as opposed to GSimpleAsyncResult), I've been lazy and did not add this :) (especially as I never saw this kind of warnings trigger).
Created attachment 324811 [details] [review] Reindent goa_provider_ensure_credentials_finish parameters
Created attachment 324812 [details] [review] Port goa_provider_ensure_credentials() to GTask
Created attachment 324813 [details] [review] Add 'error' precondition to goa_provider_get_all_finish() Ensure that the 'error' parameter is either NULL, or pointing to a NULL value.
Created attachment 324814 [details] [review] Port goa_provider_get_all() to GTask
Previous iteration had complie warnings, reattaching.
Review of attachment 324814 [details] [review]: ::: src/goabackend/goaprovider.c @@ +1231,2 @@ + g_return_val_if_fail (g_task_is_valid (result, NULL), FALSE); + g_return_val_if_fail (g_task_get_source_tag (task) != goa_provider_get_all, FALSE); Should be ==, not != @@ +1245,1 @@ *out_providers = g_list_copy_deep (providers, (GCopyFunc) g_object_ref, NULL); g_task_propagate_pointer() gives ownership of the pointer to the caller, so there is no need to re-duplicate this, this causes a leak (I have this fixed locally, but I need to squash it/test/repost).
Created attachment 324925 [details] [review] Reindent goa_provider_ensure_credentials_finish parameters
Created attachment 324926 [details] [review] Port goa_provider_ensure_credentials() to GTask
Created attachment 324927 [details] [review] Add 'error' precondition to goa_provider_get_all_finish() Ensure that the 'error' parameter is either NULL, or pointing to a NULL value.
Created attachment 324928 [details] [review] Port goa_provider_get_all() to GTask
Created attachment 324929 [details] [review] Port goa_provider_factory_get_providers() to GTask
Created attachment 324930 [details] [review] Port goa_mail_auth_{run,starttls}() to GTask
Created attachment 324931 [details] [review] Port goa_mail_client_check() to GTask This reworks the lifetime of CheckData a little bit, rather than explicitly freeing it every time the async operation ends with success or error, CheckData is set a GTask task-data. The GTask is passed around rather than being contained in CheckData, and is unref'ed every time CheckData was freed. When the last GTask reference is dropped, the corresponding CheckData memory will be freed.
Review of attachment 324925 [details] [review]: Thanks, Christophe.
Review of attachment 324926 [details] [review]: Looks good to me. Pushed.
Review of attachment 324927 [details] [review]: ++
Review of attachment 324928 [details] [review]: Looks good, apart from some minor details: ::: src/goabackend/goaprovider.c @@ +1176,3 @@ data = g_slice_new0 (GetAllData); + data->task = g_task_new (NULL, NULL, callback, user_data); + g_task_set_source_tag(data->task, goa_provider_get_all); Nitpick: missing whitespace. @@ +1239,3 @@ + { + return FALSE; + } Nitpick: the braces are not needed. @@ +1245,3 @@ + *out_providers = providers; + } + else I would prefer to NULLify providers in the previous branch and make the g_list_free_full unconditional because it will play well with g_autoptr when we finally move to it.
Created attachment 326956 [details] [review] Port goa_provider_get_all() to GTask
Review of attachment 324929 [details] [review]: ::: src/goabackend/goaproviderfactory.c @@ +131,1 @@ + g_return_val_if_fail (g_task_is_valid (result, G_OBJECT (factory)), FALSE); g_task_is_valid takes a gpointer, so the G_OBJECT cast is not needed. @@ +131,2 @@ + g_return_val_if_fail (g_task_is_valid (result, G_OBJECT (factory)), FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); Will be nicer to use a separate patch for this like you already did elsewhere. @@ +134,1 @@ + task = G_TASK (result); Good catch about doing the cast after g_return_if_fail (g_task_is_valid (...) ...) We need to fix the previous patches. @@ +139,3 @@ + { + return FALSE; + } Nitpick: the braces are not needed. @@ +145,3 @@ + *out_providers = providers; + } + else Same thing about NULLifying providers and making the g_list_free_full unconditional. ::: src/goabackend/goatelepathyfactory.c @@ +131,3 @@ g_return_if_fail (GOA_IS_TELEPATHY_FACTORY (factory)); + task = g_task_new (G_OBJECT (factory), NULL, callback, user_data); Nitpick: the cast is not needed.
Created attachment 327007 [details] [review] Port goa_provider_factory_get_providers() to GTask Fixed and pushed.
Created attachment 327008 [details] [review] providerfactory: Add 'error' precondition Split and pushed.
Created attachment 327010 [details] [review] provider: Avoid an extra CRITICAL
Review of attachment 324930 [details] [review]: ::: src/goabackend/goamailauth.c @@ +43,3 @@ +mail_auth_run_in_thread_func (GTask *task, + gpointer object, + gpointer task_data G_GNUC_UNUSED, It should be 'G_GNUC_UNUSED gpointer task_data' as per the documentation, but the compiler doesn't mind this form either. Perhaps newer GCC versions accept both. *shrug* @@ +44,3 @@ + gpointer object, + gpointer task_data G_GNUC_UNUSED, + GCancellable *cancellable G_GNUC_UNUSED) 'cancellable' is used. :) @@ +52,3 @@ { + g_task_return_boolean (task, TRUE); + } It would be more consistent to flip the if-else branches. In most places we handle the error in the if, and either fall through or use an else for the success. Plus we don't need the braces. @@ +65,3 @@ + gpointer object, + gpointer task_data G_GNUC_UNUSED, + GCancellable *cancellable G_GNUC_UNUSED) Ditto. @@ +73,3 @@ { + g_task_return_boolean (task, TRUE); + } Ditto. @@ +216,3 @@ g_return_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable)); + task = g_task_new (G_OBJECT (self), cancellable, callback, user_data); We don't need the G_OBJECT cast. @@ +229,3 @@ GError **error) { + GTask *task = G_TASK (res); It would be nicer to move the G_TASK cast after the g_task_is_valid as you did in the other patch. @@ +263,3 @@ g_return_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable)); + task = g_task_new (G_OBJECT (self), cancellable, callback, user_data); Ditto. @@ +276,3 @@ GError **error) { + GTask *task = G_TASK (res); Ditto.
Created attachment 327024 [details] [review] mail-auth: Port goa_mail_auth_{run,starttls}() to GTask
Review of attachment 324931 [details] [review]: Thanks, Christophe. Looks good apart from some minor comments: ::: src/goabackend/goamailclient.c @@ -70,3 @@ GDataOutputStream *output; GIOStream *tls_conn; - GSimpleAsyncResult *res; Nice. We should do the same clean-up in goa_provider_get_all. @@ -248,3 @@ g_quark_to_string (error->domain), error->code); - g_simple_async_result_take_error (data->res, error); See below. @@ -256,3 @@ if (server_identity == NULL) { - g_simple_async_result_take_error (data->res, error); See below. @@ -264,3 @@ if (data->tls_conn == NULL) { - g_simple_async_result_take_error (data->res, error); See below. @@ -287,3 @@ - g_simple_async_result_set_op_res_gboolean (data->res, FALSE); - g_simple_async_result_complete_in_idle (data->res); - mail_client_check_data_free (data); Normally I'd like that we are consolidating the g_task_return_error call in one place. However, in this case, keeping them at their respective error sites will let us clean up the intricate goto dance that ensured mail_client_check_data_free was only called in the error path. It will be possible because the lifecycle of the operation is now encapsulated in the GTask, which is ref-counted. Obviously this is a trade-off. I, personally, prefer the following pattern because it is so widely used: val = foo_object_op_finish (..., &error); if (val == NULL) { /* do something with error */ goto out; } /* success: carry on */ out: /* clean up and exit */ @@ +377,2 @@ data = g_slice_new0 (CheckData); + task = g_task_new (G_OBJECT (self), cancellable, callback, user_data); The G_OBJECT cast is not needed, and would be nice to add the source-tag.
Created attachment 327040 [details] [review] mail-client: Port goa_mail_client_check() to GTask
Created attachment 327041 [details] [review] mail-client: Simplify the exit paths
Let's keep it open for the remaining users of GSimpleAsyncResult.
Created attachment 331538 [details] [review] kerberos: Port sign_in_identity to GTask
Review of attachment 331538 [details] [review]: ::: src/goabackend/goakerberosprovider.c @@ +236,3 @@ g_strdup (preauth_source), g_free); + g_task_run_in_thread (operation_result, (GTaskThreadFunc)sign_in_thread); Nitpick: a space after the type cast would be nice. @@ -845,3 @@ - const char *object_path; - - object_path = g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (result)); I think there was a memory leak hiding here. I attached a fix to bug 768845 Not your fault, though. :) @@ -873,3 @@ } - g_simple_async_result_complete_in_idle (operation_result); This shouldn't be removed. on_initial_sign_in_done is the callback for the sign_in_identity async call, but operation_result is the GAsyncResult created in perform_initial_sign_in. It holds the chain of async calls together. We need to complete it so that the callback, on_account_signed_in, is reached.
Created attachment 331599 [details] [review] kerberos: Port sign_in_identity() to GTask Fixed up the above issues.
Comment on attachment 331599 [details] [review] kerberos: Port sign_in_identity() to GTask Pushed to master.
Btw, Christophe's patches are from https://gitlab.com/teuf/gnome-online-account.git
Created attachment 331602 [details] [review] kerberos: Port perform_initial_sign_in to GTask
Review of attachment 331602 [details] [review]: ::: src/goabackend/goakerberosprovider.c @@ -889,3 @@ g_cancellable_cancel (cancellable); - g_simple_async_result_complete_in_idle (operation_result); We can't just remove this. Otherwise the callback, on_account_signed_in, won't be called if prompt == NULL && error == NULL. I think a g_task_return_error_if_cancelled call after cancelling the GCancellable will work. @@ +958,3 @@ + (GAsyncReadyCallback) on_account_signed_in, + request); + g_task_set_task_data (operation_result, object, NULL); This should be g_task_set_source_tag, if we are to stay as close to the old code as possible. However, you are right. Using source_tag like this looks wrong/dangerous. Attachment 331599 [details] also had such an example. Otherwise, there is nothing to stop the GoaObject from getting unref-ed while the async call is in flight. I think we haven't hit a memory error because the system modal that is popped up limits user interaction and nothing interferes with the objects. Ideally, we should take a new ref, set it as task_data and use g_object_unref as a GDestroyNotify. We can do that in a separate patch.
Created attachment 331691 [details] [review] kerberos: Remove redundant branch
Created attachment 331693 [details] [review] kerberos: Port perform_initial_sign_in() to GTask
Created attachment 331699 [details] [review] kerberos: Remove unused callback user_data arguments
Review of attachment 331599 [details] [review]: I failed to notice one thing earlier: ::: src/goabackend/goakerberosprovider.c @@ +829,2 @@ error = NULL; + object_path = g_task_propagate_pointer (G_TASK (result), &error); We should transfer the GError thrown by sign_in_identity to the outer GAsyncResult.
Created attachment 331730 [details] [review] kerberos: Re-throw the GError from sign_in_identity and don't leak it
Created attachment 331731 [details] [review] kerberos: Port goa_identity_service_handle_sign_in to GTask
Review of attachment 331731 [details] [review]: ::: src/goaidentity/goaidentityservice.c @@ +296,1 @@ + object_path = g_task_propagate_pointer (G_TASK (result), &error); g_task_propagate_pointer will transfer ownership of the character array. So, it can't be a const char *, and we need to g_free it. @@ +296,2 @@ + object_path = g_task_propagate_pointer (G_TASK (result), &error); + if (g_task_had_error (G_TASK (result))) Nitpick: better to check for 'error != NULL' because of bug 764163.
Created attachment 331732 [details] [review] kerberos: Port goa_identity_service_handle_sign_in() to GTask
(In reply to Debarshi Ray from comment #43) > However, you are right. Using source_tag like this looks wrong/dangerous. > Attachment 331599 [details] also had such an example. Actually, I had changed this locally for the sign_in_identity() commit as well, but never pushed this anywhere :(
Comment on attachment 331730 [details] [review] kerberos: Re-throw the GError from sign_in_identity and don't leak it Pushed to master.
Created attachment 332496 [details] [review] goa_identity_manager_get_identity_finish should return a new ref This overlaps with the patches in bug 769267
Review of attachment 332496 [details] [review]: Yup, makes sense, and looks good!
Comment on attachment 332496 [details] [review] goa_identity_manager_get_identity_finish should return a new ref Pushed to master. Thanks, Christophe.
Created attachment 342866 [details] [review] identity: Port on_got_identity_for_sign_out to GTask Here are some more patches from Christoph's branch.
Review of attachment 342866 [details] [review]: ::: src/goaidentity/goaidentityservice.c @@ +444,3 @@ g_debug ("GoaIdentityService: Identity could not be signed out: %s", error->message); + g_task_return_error (operation_result, error); We will end up calling both g_task_return_error and g_task_return_boolean in this case. This is wrong because unlike g_simple_async_result_take_error, g_task_return_error will actually invoke the callback. One option is to move the goa_identity_manager_sign_identity_out_finish-related code further below. @@ +465,3 @@ on_got_identity_for_sign_out (GoaIdentityManager *manager, GAsyncResult *result, + GTask *operation_result) This makes me realize that there is an unrelated bug in this function. We cannot return in case of an error without passing the error to 'operation_result' and unreffing it. Filed as bug 776871 @@ +507,1 @@ + task = g_task_new (G_OBJECT (self), The cast is not needed because g_task_new accepts a gpointer. @@ +1528,3 @@ g_debug ("Kerberos account %s was disabled and should now be signed out", account_identity); + task = g_task_new (G_OBJECT (self), Ditto.
Created attachment 342935 [details] [review] identity: Port on_got_identity_for_sign_out to GTask Fixed the above issues.
Created attachment 342938 [details] [review] identity: Port on_got_ticket to GTask
Review of attachment 342938 [details] [review]: ::: src/goaidentity/goaidentityservice.c @@ +1397,3 @@ const char *account_identity; + object = g_task_get_task_data (operation_result); It is quite amazing that we have been passing this GoaObject around without owning a reference to it, and things haven't crashed so far! We should eventually fix it. @@ +1457,3 @@ + (GAsyncReadyCallback) + on_ticketing_done, + object); Nitpick: I'd stick to only task_data for 'object'.
Review of attachment 342938 [details] [review]: Forgot to set the 'status'.
Created attachment 342943 [details] [review] identity: Port sign_in to GTask
Review of attachment 342943 [details] [review]: ::: src/goaidentity/goaidentityservice.c @@ +152,3 @@ + /* Workaround for bgo#764163 */ + had_error = g_task_had_error (G_TASK (result)); + identity = g_task_propagate_pointer (G_TASK (result), &error); We'll need to unref 'identity' because g_task_propagate_pointer transfers the ownership to the caller.
Created attachment 342946 [details] [review] identity: Port on_got_ticket to GTask Fixed the above issues.
Created attachment 342947 [details] [review] identity: Port sign_in to GTask Added the unref.
Created attachment 342949 [details] [review] identity: Port add_temporary_account to GTask
Review of attachment 342949 [details] [review]: ::: src/goaidentity/goaidentityservice.c @@ +816,3 @@ + /* Workaround for bgo#764163 */ + had_error = g_task_had_error (G_TASK (result)); + object = g_task_propagate_pointer (G_TASK (result), &error); We need to unref 'object'.
Created attachment 342951 [details] [review] identity: Port add_temporary_account to GTask Fixed the above leak.
Created attachment 342952 [details] [review] identity: Port goa_identity_service_handle_exchange_secret_keys to GTask
Review of attachment 342952 [details] [review]: ::: src/goaidentity/goaidentityservice.c @@ +527,3 @@ + /* FIXME: 'result' expects ownership of 'invocation', which is no longer the + * case after this method */ + g_object_ref (invocation); This isn't needed. The GDBus documentation was a bit foggy about the ownership of the GDBusMethodInvocation (bug 767952). Plus, we were leaking the invocation in the pre-GTask version of this code (bug 776897). @@ +583,3 @@ + output_key, + (GDestroyNotify) + g_free); We need to unref 'operation_result' after the g_task_return_pointer.
Review of attachment 342952 [details] [review]: ::: src/goaidentity/goaidentityservice.c @@ +532,3 @@ + /* Workaround for bgo#764163 */ + had_error = g_task_had_error (G_TASK (result)); + output_key = g_task_propagate_pointer (G_TASK (result), &error); We need to g_free output_key.
Created attachment 342955 [details] [review] Port goa_identity_service_handle_exchange_secret_keys to GTask Fixed the above issues.
Review of attachment 342955 [details] [review]: Sorry, I was wrong about the ownership of 'operation_result' in attachment 342952 [details] [review] ::: src/goaidentity/goaidentityservice.c @@ +590,2 @@ + out: + g_object_unref (operation_result); We shouldn't unref 'operation_result' here. It's lifecycle is controlled by the g_bus_watch_name call and the watched_client_connections hash table. However, there is an actual bug in goa_identity_service_handle_exchange_secret_keys, where we are not unreffing 'operation_result' after the g_bus_watch_name call.
(In reply to Debarshi Ray from comment #74) > Review of attachment 342955 [details] [review] [review]: > > Sorry, I was wrong about the ownership of 'operation_result' in attachment > 342952 [details] [review] > > ::: src/goaidentity/goaidentityservice.c > @@ +590,2 @@ > + out: > + g_object_unref (operation_result); > > We shouldn't unref 'operation_result' here. It's lifecycle is controlled by > the g_bus_watch_name call and the watched_client_connections hash table. > > However, there is an actual bug in > goa_identity_service_handle_exchange_secret_keys, where we are not unreffing > 'operation_result' after the g_bus_watch_name call. I attached a fix for the leak to bug 776897
Created attachment 342965 [details] [review] Port goa_identity_service_handle_exchange_secret_keys to GTask
Created attachment 343179 [details] [review] httpclient: Split the idle handler into two parts
Created attachment 343180 [details] [review] httpclient: Add a comment
Created attachment 343181 [details] [review] httpclient: Port goa_http_client_check() to GTask
Created attachment 343182 [details] [review] httpclient: Be more strict about the GMainContext doing the clean up
Created attachment 343184 [details] [review] httpclient: Port goa_http_client_check() to GTask Minor cosmetic tweak.
Created attachment 343186 [details] [review] httpclient: Be more strict about the GMainContext doing the clean up
Review of attachment 342965 [details] [review]: seems fine, i have a few comments, but I don't really expect any of them to lead to changes in the patch... ::: src/goaidentity/goaidentityservice.c @@ +531,3 @@ GError *error; + gboolean had_error; + char *output_key = NULL; don't think this needs to be NULL initialized. we set it unconditionally below, and it looks a little weird to defer initializing error, but also initialize output_key at declaration time. @@ +538,3 @@ + /* Workaround for bgo#764163 */ + had_error = g_task_had_error (G_TASK (result)); + output_key = g_task_propagate_pointer (G_TASK (result), &error); So my personal opinion is having "Workaround for bgo#..." copy and pasted throughout the code base is a little ugly. if (error != NULL) seems just as good to me as calling g_task_had_error and I don't even think it would be a workaround to do it that way… Also, in general, I think calling g_task_had_error can run into a race condition. It and g_task_propagate_pointer both check the cancellable by default, so there may be a situation where had_error succeeds and propagate_pointer fails immediately afterward (if the cancellable gets canceled). Probably doesn't matter in this case, though. Anyway, that's really an aside, because we already follow this had_error pattern extensively, and it doesn't really matter either way I guess. Consistently should probably win out, and they can all get mopped up at the same time when we bump req to glib 2.50 @@ -590,3 @@ - (GDestroyNotify) - g_free); - g_simple_async_result_complete_in_idle (operation_result); so we're losing the complete_in_idle here, but it's not clear to me why I did that instead of complete anyway. we don't use G_DBUS_INTERFACE_SKELETON_FLAGS_HANDLE_METHOD_INVOCATIONS_IN_THREAD or anything like that, so probably no big semantic change here @@ -605,1 @@ - cancellable = g_object_get_data (G_OBJECT (operation_result), "cancellable"); ah this is probably my favorite part of going to GTask :-) I hate how the cancellable isn't fetchable from the GSimpleAsyncResult api. @@ +621,2 @@ cancellable = g_cancellable_new (); + operation_result = g_task_new (self, cancellable, (GAsyncReadyCallback) on_secret_keys_exchanged, NULL); so while it's true a GTask implements a GAsyncResult, the words task and result aren't really synonyms... it might be better to rename it operation_task, or exchange_secret_keys_task or something. again doesn't really matter, though, and maybe not worth the code churn.
Review of attachment 343179 [details] [review]: looks nicer to me!
Review of attachment 343180 [details] [review]: ::: src/goabackend/goahttpclient.c @@ +210,3 @@ + * handler, and freeing CheckData will disconnect the handler. Since + * disconnecting from inside the handler will cause a deadlock, we + * use an idle handler to break them up. maybe link to https://bugzilla.gnome.org/show_bug.cgi?id=705395
Review of attachment 343186 [details] [review]: sounds good. do you mean to have both bug links there? I wonder if git bz will get confused.
Review of attachment 343184 [details] [review]: seems okay to me ::: src/goabackend/goahttpclient.c @@ -78,3 @@ typedef struct { GCancellable *cancellable; this could probably go away too right ? @@ +171,3 @@ + * will only unref the GTask in an idle handler. Therefore, we don't + * need to temporarily add our own reference to it. + data = g_task_get_task_data (task); personally not a huge fan of duplicated comments... maybe a function like stop_client_check_task that took the task and called soup_session_abort with the comment. @@ +273,3 @@ { data->cancellable = g_object_ref (cancellable); + data->cancellable_id = g_cancellable_connect (cancellable, not really sure why you changed this line, but doesn't hurt (and is useful if you do want to get rid of data->cancellable)
(In reply to Ray Strode [halfline] from comment #83) > Review of attachment 342965 [details] [review] [review]: Thanks for the reviews, halfline! > ::: src/goaidentity/goaidentityservice.c > @@ +531,3 @@ > GError *error; > + gboolean had_error; > + char *output_key = NULL; > > don't think this needs to be NULL initialized. we set it unconditionally > below, and it looks a little weird to defer initializing error, but also > initialize output_key at declaration time. Yeah. Although this gets us very close to what g_auto* would want us to do. :) > @@ +538,3 @@ > + /* Workaround for bgo#764163 */ > + had_error = g_task_had_error (G_TASK (result)); > + output_key = g_task_propagate_pointer (G_TASK (result), &error); > > So my personal opinion is having "Workaround for bgo#..." copy and pasted > throughout the code base is a little ugly. if (error != NULL) seems just as > good to me as calling g_task_had_error and I don't even think it would be a > workaround to do it that way… > > Also, in general, I think calling g_task_had_error can run into a race > condition. It and g_task_propagate_pointer both check the cancellable by > default, so there may be a situation where had_error succeeds and > propagate_pointer fails immediately afterward (if the cancellable gets > canceled). Probably doesn't matter in this case, though. Ok, that's a good observation. I changed it to use: "if (error != NULL)" > Anyway, that's really an aside, because we already follow this had_error > pattern extensively, and it doesn't really matter either way I guess. > Consistently should probably win out, and they can all get mopped up at the > same time when we bump req to glib 2.50 The g_task_had_error is mostly accidental. There's no need to keep on using it. > @@ -590,3 @@ > - (GDestroyNotify) > - g_free); > - g_simple_async_result_complete_in_idle (operation_result); > > so we're losing the complete_in_idle here, but it's not clear to me why I > did that instead of complete anyway. we don't use > G_DBUS_INTERFACE_SKELETON_FLAGS_HANDLE_METHOD_INVOCATIONS_IN_THREAD or > anything like that, so probably no big semantic change here Yes, this ought to be fine. GTask takes care of all those little details of where and how to dispatch the result. If we ever start using HANDLE_METHOD_INVOCATIONS_IN_THREAD (doesn't make sense as things are right now), then I'd probably initiate the call chain from the main thread so that everything carries on as usual. > @@ -605,1 @@ > - cancellable = g_object_get_data (G_OBJECT (operation_result), > "cancellable"); > > ah this is probably my favorite part of going to GTask :-) I hate how the > cancellable isn't fetchable from the GSimpleAsyncResult api. > > @@ +621,2 @@ > cancellable = g_cancellable_new (); > + operation_result = g_task_new (self, cancellable, (GAsyncReadyCallback) > on_secret_keys_exchanged, NULL); > > so while it's true a GTask implements a GAsyncResult, the words task and > result aren't really synonyms... it might be better to rename it > operation_task, or exchange_secret_keys_task or something. again doesn't > really matter, though, and maybe not worth the code churn. Yeah, I'd vote for doing the rename in a separate commit. That would mess up "git blame" a bit, but I prefer to err on the side of smaller diffs.
Created attachment 362552 [details] [review] Port goa_identity_service_handle_exchange_secret_keys to GTask Made the above changes and pushed to master.
Review of attachment 343179 [details] [review]: ::: src/goabackend/goahttpclient.c @@ +108,1 @@ g_object_unref (data->res); Eek! Looks like I left an extra g_simple_async_result_complete_in_idle lying around above.
(In reply to Ray Strode [halfline] from comment #85) > Review of attachment 343180 [details] [review] [review]: > > ::: src/goabackend/goahttpclient.c > @@ +210,3 @@ > + * handler, and freeing CheckData will disconnect the handler. Since > + * disconnecting from inside the handler will cause a deadlock, we > + * use an idle handler to break them up. > > maybe link to https://bugzilla.gnome.org/show_bug.cgi?id=705395 Done.
Created attachment 362563 [details] [review] httpclient: Split the idle handler into two parts
Created attachment 362564 [details] [review] httpclient: Add a comment
(In reply to Ray Strode [halfline] from comment #87) > Review of attachment 343184 [details] [review] [review]: > > [...] > > ::: src/goabackend/goahttpclient.c > @@ -78,3 @@ > typedef struct > { > GCancellable *cancellable; > > this could probably go away too right ? I wish. The need to call g_cancellable_disconnect in http_client_check_data_free (ie. the task_data_destroy) made it necessary to keep it. Couldn't think of a non-hacky alternative. Thoughts? > @@ +171,3 @@ > + * will only unref the GTask in an idle handler. Therefore, we don't > + * need to temporarily add our own reference to it. > + data = g_task_get_task_data (task); > > personally not a huge fan of duplicated comments... > > maybe a function like stop_client_check_task that took the task and called > soup_session_abort with the comment. Removed the comments by a trivial rearrangement of the code. > @@ +273,3 @@ > { > data->cancellable = g_object_ref (cancellable); > + data->cancellable_id = g_cancellable_connect (cancellable, > > not really sure why you changed this line, but doesn't hurt (and is useful > if you do want to get rid of data->cancellable) I wanted to reduce the use of "data->cancellable" to only those cases where it is absolutely necessary.
Created attachment 362569 [details] [review] httpclient: Port goa_http_client_check() to GTask
Comment on attachment 362569 [details] [review] httpclient: Port goa_http_client_check() to GTask I ended up pushing this to master. I have been guilty of letting these patches bit rot for so long that I don't want to prolong the wait. While I am open to ideas regarding the fate of "data->cancellable", I'd rather push this and reduce this flood of warnings. :)
Review of attachment 362569 [details] [review]: ::: src/goabackend/goahttpclient.c @@ +126,3 @@ { goa_utils_set_error_ssl (&error, cert_flags); + g_task_return_error (task, error); I think this regresses the fix for bug 743044 Ever since we ported from the deprecated SoupSessionAsync to SoupSession, the response callbacks are invoked in the next iteration of the main loop after soup_session_abort has returned. This means that there must be running GMainLoop to invoke http_client_check_cancelled_cb so that the reference on the GTask can be dropped. Unfortunately, we can't rely on the presence of a running GMainLoop once the GTask has returned. The user's callback might stop it, like it happens in goa_http_client_check_sync.
Created attachment 374163 [details] [review] httpclient: Simplify the clean up
Created attachment 374164 [details] [review] ewsclient: Simplify the clean up
Created attachment 374165 [details] [review] httpclient: Ensure that the GTask is always cleaned up
Created attachment 374166 [details] [review] httpclient: Simplify the clean up
Created attachment 374167 [details] [review] ewsclient: Simplify the clean up
Created attachment 374168 [details] [review] httpclient: Ensure that the GTask is always cleaned up
Created attachment 374171 [details] [review] ewsclient: Port goa_ews_client_autodiscover() to GTask
I discovered a bug in GTask while working on this: https://gitlab.gnome.org/GNOME/glib/issues/1608
Created attachment 374228 [details] [review] kerberos-identity-manager: Try not to use GIOSchedulerJob
Created attachment 374229 [details] [review] kerberos-identity-manager: Port to GThreadPool
These last two patches are also in the gnome-online-accounts:wip/rishi/thread-pool branch.
Created attachment 374230 [details] [review] kerberos-identity-manager: Port to GThreadPool
Created attachment 374231 [details] [review] kerberos-identity-manager: Port to GTask
Created attachment 374232 [details] [review] kerberos-identity-manager: Try not to use GIOSchedulerJob
Created attachment 374233 [details] [review] kerberos-identity-manager: Port to GThreadPool
Created attachment 374234 [details] [review] kerberos-identity-manager: Port to GTask
I ran the previous iteration of the patches over the weekend, and now I am going to run the latest iteration of the patches, which only contain minor tweaks, overnight until tomorrow. If they hold good, no crashes no CRITICALs etc., I will merge them tomorrow. Would be really good to see this bug closed. :)
Done!