[gnome-online-accounts] httpclient: Make the GCancellable handling thread-safe

[Debarshi Ray <debarshir src gnome org>]

commit e9114fe4b56b6a7ddecb7502d60406951a3d322a
Author: Debarshi Ray <debarshir gnome org>
Date:   Tue Nov 27 16:25:31 2018 +0100

    httpclient: Make the GCancellable handling thread-safe
    
    A GCancellable can be cancelled from a different thread than the one
    where the GTask for the goa_http_client_check operation was created.
    Since the GCancellable::cancelled handler is invoked in the same thread
    as the g_cancellable_cancel call, unguarded access to the GError in
    CheckData is unsafe.
    
    Instead of introducing a GMutex, the GError can be set in the response
    callback which is guaranteed to be invoked in the same thread where the
    GTask was created. Ensuring that the GError is always accessed from the
    same thread makes it considerably easier to reason about its state.
    eg., it can be guaranteed that it won't be set if the status of the
    response isn't SOUP_STATUS_CANCELLED.

 src/goabackend/goahttpclient.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
---
diff --git a/src/goabackend/goahttpclient.c b/src/goabackend/goahttpclient.c
index c763d3f1..ee654565 100644
--- a/src/goabackend/goahttpclient.c
+++ b/src/goabackend/goahttpclient.c
@@ -143,43 +143,43 @@ http_client_check_cancelled_cb (GCancellable *cancellable, gpointer user_data)
 
   data = g_task_get_task_data (task);
 
-  if (data->error == NULL)
-    {
-      gboolean cancelled;
-
-      cancelled = g_cancellable_set_error_if_cancelled (cancellable, &data->error);
-
-      /* The callback will be invoked after we have returned to the
-       * main loop.
-       */
-      soup_session_abort (data->session);
-
-      g_return_if_fail (cancelled);
-    }
+  /* The callback will be invoked after we have returned to the main
+   * loop.
+   */
+  soup_session_abort (data->session);
 }
 
 static void
 http_client_check_response_cb (SoupSession *session, SoupMessage *msg, gpointer user_data)
 {
   CheckData *data;
+  GCancellable *cancellable;
   GTask *task = G_TASK (user_data);
 
   data = (CheckData *) g_task_get_task_data (task);
+  cancellable = g_task_get_cancellable (task);
 
   /* status == SOUP_STATUS_CANCELLED, if we are being aborted by the
    * GCancellable or due to an SSL error.
    */
   if (msg->status_code == SOUP_STATUS_CANCELLED)
     {
-      g_return_if_fail (data->error != NULL);
+      /* If we are being aborted by the GCancellable then there might
+       * or might not be an error. The GCancellable can be triggered
+       * from a different thread, so it depends on the exact ordering
+       * of events across threads.
+       */
+      if (data->error == NULL)
+        g_cancellable_set_error_if_cancelled (cancellable, &data->error);
+
       goto out;
     }
   else if (msg->status_code != SOUP_STATUS_OK)
     {
       g_warning ("goa_http_client_check() failed: %u — %s", msg->status_code, msg->reason_phrase);
-      if (data->error == NULL)
-        goa_utils_set_error_soup (&data->error, msg);
+      g_return_if_fail (data->error == NULL);
 
+      goa_utils_set_error_soup (&data->error, msg);
       goto out;
     }