[glib/wip/tingping/socket-client-slow-fix: 1/2] gsocketclient: Fix critical on cancellation

[Patrick Griffis <pgriffis src gnome org>]

commit 6e88f9625cf90a046e21c37084debf456f517a8b
Author: Patrick Griffis <pgriffis igalia com>
Date:   Thu Feb 21 15:31:52 2019 -0500

    gsocketclient: Fix critical on cancellation
    
    We need to be more explicit in handling cancellation to avoid
    multiple task returns.
    
    Fixes #1693

 gio/gsocketclient.c            | 32 ++++++++++++++++++-----
 gio/tests/gsocketclient-slow.c | 58 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 81 insertions(+), 9 deletions(-)
---
diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c
index 11e92e909..689d87d38 100644
--- a/gio/gsocketclient.c
+++ b/gio/gsocketclient.c
@@ -1424,9 +1424,14 @@ g_socket_client_async_connect_complete (GSocketClientAsyncConnectData *data)
       data->connection = (GIOStream *)wrapper_connection;
     }
 
-  g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_COMPLETE, data->connectable, data->connection);
-  g_task_return_pointer (data->task, data->connection, g_object_unref);
-  data->connection = NULL;
+  if (!g_task_return_error_if_cancelled (data->task))
+    {
+      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_COMPLETE, data->connectable, 
data->connection);
+      g_task_return_pointer (data->task, g_steal_pointer (&data->connection), g_object_unref);
+    }
+  else
+    g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_COMPLETE, data->connectable, NULL);
+
   g_object_unref (data->task);
 }
 
@@ -1545,12 +1550,24 @@ g_socket_client_proxy_connect_callback (GObject      *object,
 }
 
 static gboolean
-task_completed_or_cancelled (GTask *task)
+task_completed_or_cancelled (GSocketClientAsyncConnectData *data)
 {
+  GTask *task = data->task;
+  GCancellable *cancellable = g_task_get_cancellable (task);
+  GError *error = NULL;
+
   if (g_task_get_completed (task))
     return TRUE;
-  else if (g_task_return_error_if_cancelled (task))
+  else if (cancellable && g_cancellable_set_error_if_cancelled (cancellable, &error))
+    {
+      if (!g_task_had_error (task))
+        {
+          g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_COMPLETE, data->connectable, NULL);
+          g_task_return_error (task, g_steal_pointer (&error));
+        }
+      g_clear_error (&error);
       return TRUE;
+    }
   else
     return FALSE;
 }
@@ -1567,7 +1584,7 @@ g_socket_client_connected_callback (GObject      *source,
   GProxy *proxy;
   const gchar *protocol;
 
-  if (task_completed_or_cancelled (data->task) || g_cancellable_is_cancelled (attempt->cancellable))
+  if (task_completed_or_cancelled (data) || g_cancellable_is_cancelled (attempt->cancellable))
     {
       g_object_unref (data->task);
       connection_attempt_unref (attempt);
@@ -1701,7 +1718,7 @@ g_socket_client_enumerator_callback (GObject      *object,
   ConnectionAttempt *attempt;
   GError *error = NULL;
 
-  if (task_completed_or_cancelled (data->task))
+  if (task_completed_or_cancelled (data))
     {
       g_object_unref (data->task);
       return;
@@ -1835,6 +1852,7 @@ g_socket_client_connect_async (GSocketClient       *client,
    */
 
   data->task = g_task_new (client, cancellable, callback, user_data);
+  g_task_set_check_cancellable (data->task, FALSE); /* We handle this manually */
   g_task_set_source_tag (data->task, g_socket_client_connect_async);
   g_task_set_task_data (data->task, data, (GDestroyNotify)g_socket_client_async_connect_data_free);
 
diff --git a/gio/tests/gsocketclient-slow.c b/gio/tests/gsocketclient-slow.c
index a78ea71d0..181c278a4 100644
--- a/gio/tests/gsocketclient-slow.c
+++ b/gio/tests/gsocketclient-slow.c
@@ -87,7 +87,24 @@ on_timer (GCancellable *cancel)
 }
 
 static void
-test_happy_eyeballs_cancel (void)
+on_event (GSocketClient      *client,
+          GSocketClientEvent  event,
+          GSocketConnectable *connectable,
+          GIOStream          *connection,
+          gboolean           *got_completed_event)
+{
+  if (event == G_SOCKET_CLIENT_COMPLETE)
+    {
+      *got_completed_event = TRUE;
+      /* Due to timing it is impossible to ensure cancellation doesn't
+         happen after we returned the pointer. So this may be non-NULL.
+       */
+      /* g_assert_null (connection); */
+    }
+}
+
+static void
+test_happy_eyeballs_cancel_delayed (void)
 {
   GSocketClient *client;
   GSocketService *service;
@@ -95,6 +112,7 @@ test_happy_eyeballs_cancel (void)
   guint16 port;
   GMainLoop *loop;
   GCancellable *cancel;
+  gboolean got_completed_event = FALSE;
 
   loop = g_main_loop_new (NULL, FALSE);
 
@@ -107,8 +125,42 @@ test_happy_eyeballs_cancel (void)
   cancel = g_cancellable_new ();
   g_socket_client_connect_to_host_async (client, "localhost", port, cancel, on_connected_cancelled, loop);
   g_timeout_add (1, (GSourceFunc) on_timer, cancel);
+  g_signal_connect (client, "event", G_CALLBACK (on_event), &got_completed_event);
   g_main_loop_run (loop);
 
+  g_assert_true (got_completed_event);
+  g_main_loop_unref (loop);
+  g_object_unref (service);
+  g_object_unref (client);
+  g_object_unref (cancel);
+}
+
+static void
+test_happy_eyeballs_cancel_instant (void)
+{
+  GSocketClient *client;
+  GSocketService *service;
+  GError *error = NULL;
+  guint16 port;
+  GMainLoop *loop;
+  GCancellable *cancel;
+  gboolean got_completed_event = FALSE;
+
+  loop = g_main_loop_new (NULL, FALSE);
+
+  service = g_socket_service_new ();
+  port = g_socket_listener_add_any_inet_port (G_SOCKET_LISTENER (service), NULL, &error);
+  g_assert_no_error (error);
+  g_socket_service_start (service);
+
+  client = g_socket_client_new ();
+  cancel = g_cancellable_new ();
+  g_cancellable_cancel (cancel);
+  g_socket_client_connect_to_host_async (client, "localhost", port, cancel, on_connected_cancelled, loop);
+  g_signal_connect (client, "event", G_CALLBACK (on_event), &got_completed_event);
+  g_main_loop_run (loop);
+
+  g_assert_true (got_completed_event);
   g_main_loop_unref (loop);
   g_object_unref (service);
   g_object_unref (client);
@@ -121,7 +173,9 @@ main (int argc, char *argv[])
   g_test_init (&argc, &argv, NULL);
 
   g_test_add_func ("/socket-client/happy-eyeballs/slow", test_happy_eyeballs);
-  g_test_add_func ("/socket-client/happy-eyeballs/cancellation", test_happy_eyeballs_cancel);
+  g_test_add_func ("/socket-client/happy-eyeballs/cancellation/instant", test_happy_eyeballs_cancel_instant);
+  g_test_add_func ("/socket-client/happy-eyeballs/cancellation/delayed", test_happy_eyeballs_cancel_delayed);
+
 
   return g_test_run ();
 }
\ No newline at end of file