Merge branch 'mcatanzaro/#2211' into 'master'

Various improvements in GSocketClient

Closes #2211 and #1994

See merge request GNOME/glib!1686

1 files changed, 161 insertions, 88 deletions

diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c
index ca01b68f4..ce3c186fb 100644
--- a/gio/gsocketclient.c
+++ b/gio/gsocketclient.c
@@ -828,7 +828,7 @@ g_socket_client_class_init (GSocketClientClass *class)
    * multiple times (or not at all) for a given connectable (in
    * particular, if @client ends up attempting to connect to more than
    * one address). However, if @client emits the #GSocketClient::event
-   * signal at all for a given connectable, that it will always emit
+   * signal at all for a given connectable, then it will always emit
    * it with %G_SOCKET_CLIENT_COMPLETE when it is done.
    *
    * Note that there may be additional #GSocketClientEvent values in
@@ -945,7 +945,7 @@ g_socket_client_class_init (GSocketClientClass *class)
 
 static void
 g_socket_client_emit_event (GSocketClient       *client,
-			    GSocketClientEvent  event,
+			    GSocketClientEvent   event,
 			    GSocketConnectable  *connectable,
 			    GIOStream           *connection)
 {
@@ -953,6 +953,72 @@ g_socket_client_emit_event (GSocketClient       *client,
 		 event, connectable, connection);
 }
 
+/* Originally, GSocketClient returned whatever error occured last. Turns
+ * out this doesn't work well in practice. Consider the following case:
+ * DNS returns an IPv4 and IPv6 address. First we'll connect() to the
+ * IPv4 address, and say that succeeds, but TLS is enabled and the TLS
+ * handshake fails. Then we try the IPv6 address and receive ENETUNREACH
+ * because IPv6 isn't supported. We wind up returning NETWORK_UNREACHABLE
+ * even though the address can be pinged and a TLS error would be more
+ * appropriate. So instead, we now try to return the error corresponding
+ * to the latest attempted GSocketClientEvent in the connection process.
+ * TLS errors take precedence over proxy errors, which take precedence
+ * over connect() errors, which take precedence over DNS errors.
+ *
+ * Note that the example above considers a sync codepath, but this is an
+ * issue for the async codepath too, where events and errors may occur
+ * in confusing orders.
+ */
+typedef struct
+{
+  GError *tmp_error;
+  GError *best_error;
+  GSocketClientEvent best_error_event;
+} SocketClientErrorInfo;
+
+static SocketClientErrorInfo *
+socket_client_error_info_new (void)
+{
+  return g_new0 (SocketClientErrorInfo, 1);
+}
+
+static void
+socket_client_error_info_free (SocketClientErrorInfo *info)
+{
+  g_assert (info->tmp_error == NULL);
+  g_clear_error (&info->best_error);
+  g_free (info);
+}
+
+static void
+consider_tmp_error (SocketClientErrorInfo *info,
+                    GSocketClientEvent     event)
+{
+  if (info->tmp_error == NULL)
+    return;
+
+  /* If we ever add more GSocketClientEvents in the future, then we'll
+   * no longer be able to use >= for this comparison, because future
+   * events will compare greater than G_SOCKET_CLIENT_COMPLETE. Until
+   * then, this is convenient. Note G_SOCKET_CLIENT_RESOLVING is 0 so we
+   * need to use >= here or those errors would never be set. That means
+   * if we get two errors on the same GSocketClientEvent, we wind up
+   * preferring the last one, which is fine.
+   */
+  g_assert (event <= G_SOCKET_CLIENT_COMPLETE);
+  if (event >= info->best_error_event)
+    {
+      g_clear_error (&info->best_error);
+      info->best_error = info->tmp_error;
+      info->tmp_error = NULL;
+      info->best_error_event = event;
+    }
+  else
+    {
+      g_clear_error (&info->tmp_error);
+    }
+}
+
 /**
  * g_socket_client_connect:
  * @client: a #GSocketClient.
@@ -991,9 +1057,10 @@ g_socket_client_connect (GSocketClient       *client,
 {
   GIOStream *connection = NULL;
   GSocketAddressEnumerator *enumerator = NULL;
-  GError *last_error, *tmp_error;
+  SocketClientErrorInfo *error_info;
+  gboolean ever_resolved = FALSE;
 
-  last_error = NULL;
+  error_info = socket_client_error_info_new ();
 
   if (can_use_proxy (client))
     {
@@ -1018,44 +1085,38 @@ g_socket_client_connect (GSocketClient       *client,
 
       if (g_cancellable_is_cancelled (cancellable))
 	{
-	  g_clear_error (error);
-	  g_clear_error (&last_error);
-	  g_cancellable_set_error_if_cancelled (cancellable, error);
+	  g_clear_error (&error_info->best_error);
+	  g_cancellable_set_error_if_cancelled (cancellable, &error_info->best_error);
 	  break;
 	}
 
-      tmp_error = NULL;
-      g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVING,
-				  connectable, NULL);
+      if (!ever_resolved)
+	{
+	  g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVING,
+				      connectable, NULL);
+	}
       address = g_socket_address_enumerator_next (enumerator, cancellable,
-	      					  &tmp_error);
+						  &error_info->tmp_error);
+      consider_tmp_error (error_info, G_SOCKET_CLIENT_RESOLVING);
+      if (!ever_resolved)
+	{
+	  g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVED,
+				      connectable, NULL);
+	  ever_resolved = TRUE;
+	}
 
       if (address == NULL)
 	{
-	  if (tmp_error)
-	    {
-	      g_clear_error (&last_error);
-	      g_propagate_error (error, tmp_error);
-	    }
-	  else if (last_error)
-	    {
-	      g_propagate_error (error, last_error);
-	    }
-	  else
-            g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED,
-                                 _("Unknown error on connect"));
+          /* Enumeration is finished. */
+          g_assert (&error_info->best_error != NULL);
 	  break;
 	}
-      g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVED,
-				  connectable, NULL);
 
       using_proxy = (G_IS_PROXY_ADDRESS (address) &&
 		     client->priv->enable_proxy);
 
-      /* clear error from previous attempt */
-      g_clear_error (&last_error);
-
-      socket = create_socket (client, address, &last_error);
+      socket = create_socket (client, address, &error_info->tmp_error);
+      consider_tmp_error (error_info, G_SOCKET_CLIENT_CONNECTING);
       if (socket == NULL)
 	{
 	  g_object_unref (address);
@@ -1067,14 +1128,15 @@ g_socket_client_connect (GSocketClient       *client,
       g_socket_client_emit_event (client, G_SOCKET_CLIENT_CONNECTING, connectable, connection);
 
       if (g_socket_connection_connect (G_SOCKET_CONNECTION (connection),
-				       address, cancellable, &last_error))
+				       address, cancellable, &error_info->tmp_error))
 	{
           g_socket_connection_set_cached_remote_address ((GSocketConnection*)connection, NULL);
 	  g_socket_client_emit_event (client, G_SOCKET_CLIENT_CONNECTED, connectable, connection);
 	}
       else
 	{
-	  clarify_connect_error (last_error, connectable, address);
+	  clarify_connect_error (error_info->tmp_error, connectable, address);
+          consider_tmp_error (error_info, G_SOCKET_CLIENT_CONNECTING);
 	  g_object_unref (connection);
 	  connection = NULL;
 	}
@@ -1095,9 +1157,10 @@ g_socket_client_connect (GSocketClient       *client,
               g_critical ("Trying to proxy over non-TCP connection, this is "
                           "most likely a bug in GLib IO library.");
 
-              g_set_error_literal (&last_error,
+              g_set_error_literal (&error_info->tmp_error,
                   G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
                   _("Proxying over a non-TCP connection is not supported."));
+              consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
 
 	      g_object_unref (connection);
 	      connection = NULL;
@@ -1115,7 +1178,9 @@ g_socket_client_connect (GSocketClient       *client,
 						  connection,
 						  proxy_addr,
 						  cancellable,
-						  &last_error);
+						  &error_info->tmp_error);
+	      consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
+
 	      g_object_unref (connection);
 	      connection = proxy_connection;
 	      g_object_unref (proxy);
@@ -1125,9 +1190,10 @@ g_socket_client_connect (GSocketClient       *client,
 	    }
 	  else
 	    {
-	      g_set_error (&last_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
+	      g_set_error (&error_info->tmp_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
 			   _("Proxy protocol “%s” is not supported."),
 			   protocol);
+	      consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
 	      g_object_unref (connection);
 	      connection = NULL;
 	    }
@@ -1137,7 +1203,7 @@ g_socket_client_connect (GSocketClient       *client,
 	{
 	  GIOStream *tlsconn;
 
-	  tlsconn = g_tls_client_connection_new (connection, connectable, &last_error);
+	  tlsconn = g_tls_client_connection_new (connection, connectable, &error_info->tmp_error);
 	  g_object_unref (connection);
 	  connection = tlsconn;
 
@@ -1147,16 +1213,21 @@ g_socket_client_connect (GSocketClient       *client,
                                                             client->priv->tls_validation_flags);
 	      g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKING, connectable, connection);
 	      if (g_tls_connection_handshake (G_TLS_CONNECTION (tlsconn),
-					      cancellable, &last_error))
+					      cancellable, &error_info->tmp_error))
 		{
 		  g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKED, connectable, connection);
 		}
 	      else
 		{
+		  consider_tmp_error (error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
 		  g_object_unref (tlsconn);
 		  connection = NULL;
 		}
 	    }
+          else
+            {
+              consider_tmp_error (error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
+            }
 	}
 
       if (connection && !G_IS_SOCKET_CONNECTION (connection))
@@ -1173,6 +1244,10 @@ g_socket_client_connect (GSocketClient       *client,
     }
   g_object_unref (enumerator);
 
+  if (!connection)
+    g_propagate_error (error, g_steal_pointer (&error_info->best_error));
+  socket_client_error_info_free (error_info);
+
   g_socket_client_emit_event (client, G_SOCKET_CLIENT_COMPLETE, connectable, connection);
   return G_SOCKET_CONNECTION (connection);
 }
@@ -1350,7 +1425,7 @@ typedef struct
 
   GSList *connection_attempts;
   GSList *successful_connections;
-  GError *last_error;
+  SocketClientErrorInfo *error_info;
 
   gboolean enumerated_at_least_once;
   gboolean enumeration_completed;
@@ -1370,7 +1445,7 @@ g_socket_client_async_connect_data_free (GSocketClientAsyncConnectData *data)
   g_slist_free_full (data->connection_attempts, connection_attempt_unref);
   g_slist_free_full (data->successful_connections, connection_attempt_unref);
 
-  g_clear_error (&data->last_error);
+  g_clear_pointer (&data->error_info, socket_client_error_info_free);
 
   g_slice_free (GSocketClientAsyncConnectData, data);
 }
@@ -1493,14 +1568,6 @@ g_socket_client_enumerator_callback (GObject      *object,
 				     gpointer      user_data);
 
 static void
-set_last_error (GSocketClientAsyncConnectData *data,
-		GError *error)
-{
-  g_clear_error (&data->last_error);
-  data->last_error = error;
-}
-
-static void
 enumerator_next_async (GSocketClientAsyncConnectData *data,
                        gboolean                       add_task_ref)
 {
@@ -1509,7 +1576,8 @@ enumerator_next_async (GSocketClientAsyncConnectData *data,
   if (add_task_ref)
     g_object_ref (data->task);
 
-  g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVING, data->connectable, NULL);
+  if (!data->enumerated_at_least_once)
+    g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVING, data->connectable, NULL);
   g_debug ("GSocketClient: Starting new address enumeration");
   g_socket_address_enumerator_next_async (data->enumerator,
 					  data->enumeration_cancellable,
@@ -1529,7 +1597,7 @@ g_socket_client_tls_handshake_callback (GObject      *object,
 
   if (g_tls_connection_handshake_finish (G_TLS_CONNECTION (object),
 					 result,
-					 &data->last_error))
+					 &data->error_info->tmp_error))
     {
       g_object_unref (attempt->connection);
       attempt->connection = G_IO_STREAM (object);
@@ -1542,7 +1610,9 @@ g_socket_client_tls_handshake_callback (GObject      *object,
     {
       g_object_unref (object);
       connection_attempt_unref (attempt);
-      g_debug ("GSocketClient: TLS handshake failed: %s", data->last_error->message);
+
+      g_debug ("GSocketClient: TLS handshake failed: %s", data->error_info->tmp_error->message);
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
       try_next_connection_or_finish (data, TRUE);
     }
 }
@@ -1562,7 +1632,7 @@ g_socket_client_tls_handshake (ConnectionAttempt *attempt)
   g_debug ("GSocketClient: Starting TLS handshake");
   tlsconn = g_tls_client_connection_new (attempt->connection,
 					 data->connectable,
-					 &data->last_error);
+					 &data->error_info->tmp_error);
   if (tlsconn)
     {
       g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (tlsconn),
@@ -1577,6 +1647,8 @@ g_socket_client_tls_handshake (ConnectionAttempt *attempt)
   else
     {
       connection_attempt_unref (attempt);
+
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
       try_next_connection_or_finish (data, TRUE);
     }
 }
@@ -1592,19 +1664,19 @@ g_socket_client_proxy_connect_callback (GObject      *object,
   g_object_unref (attempt->connection);
   attempt->connection = g_proxy_connect_finish (G_PROXY (object),
                                                 result,
-                                                &data->last_error);
+                                                &data->error_info->tmp_error);
   if (attempt->connection)
     {
       g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATED, data->connectable, attempt->connection);
+      g_socket_client_tls_handshake (attempt);
     }
   else
     {
       connection_attempt_unref (attempt);
+
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
       try_next_connection_or_finish (data, TRUE);
-      return;
     }
-
-  g_socket_client_tls_handshake (attempt);
 }
 
 static void
@@ -1672,9 +1744,10 @@ try_next_successful_connection (GSocketClientAsyncConnectData *data)
       g_critical ("Trying to proxy over non-TCP connection, this is "
           "most likely a bug in GLib IO library.");
 
-      g_set_error_literal (&data->last_error,
+      g_set_error_literal (&data->error_info->tmp_error,
           G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
           _("Proxying over a non-TCP connection is not supported."));
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
     }
   else if (g_hash_table_contains (data->client->priv->app_proxies, protocol))
     {
@@ -1701,11 +1774,10 @@ try_next_successful_connection (GSocketClientAsyncConnectData *data)
     }
   else
     {
-      g_clear_error (&data->last_error);
-
-      g_set_error (&data->last_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
+      g_set_error (&data->error_info->tmp_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
           _("Proxy protocol “%s” is not supported."),
           protocol);
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
     }
 
   data->connection_in_progress = FALSE;
@@ -1736,7 +1808,7 @@ try_next_connection_or_finish (GSocketClientAsyncConnectData *data,
       return;
     }
 
-  complete_connection_with_error (data, data->last_error);
+  complete_connection_with_error (data, g_steal_pointer (&data->error_info->best_error));
 }
 
 static void
@@ -1746,7 +1818,6 @@ g_socket_client_connected_callback (GObject      *source,
 {
   ConnectionAttempt *attempt = user_data;
   GSocketClientAsyncConnectData *data = attempt->data;
-  GError *error = NULL;
 
   if (task_completed_or_cancelled (data) || g_cancellable_is_cancelled (attempt->cancellable))
     {
@@ -1762,20 +1833,20 @@ g_socket_client_connected_callback (GObject      *source,
     }
 
   if (!g_socket_connection_connect_finish (G_SOCKET_CONNECTION (source),
-					   result, &error))
+					   result, &data->error_info->tmp_error))
     {
       if (!g_cancellable_is_cancelled (attempt->cancellable))
         {
-          clarify_connect_error (error, data->connectable, attempt->address);
-          set_last_error (data, error);
-          g_debug ("GSocketClient: Connection attempt failed: %s", error->message);
+          clarify_connect_error (data->error_info->tmp_error, data->connectable, attempt->address);
+          consider_tmp_error (data->error_info, G_SOCKET_CLIENT_CONNECTING);
+          g_debug ("GSocketClient: Connection attempt failed: %s", data->error_info->tmp_error->message);
           connection_attempt_remove (attempt);
           connection_attempt_unref (attempt);
           try_next_connection_or_finish (data, FALSE);
         }
       else /* Silently ignore cancelled attempts */
         {
-          g_clear_error (&error);
+          g_clear_error (&data->error_info->tmp_error);
           g_object_unref (data->task);
           connection_attempt_unref (attempt);
         }
@@ -1833,7 +1904,6 @@ g_socket_client_enumerator_callback (GObject      *object,
   GSocketAddress *address = NULL;
   GSocket *socket;
   ConnectionAttempt *attempt;
-  GError *error = NULL;
 
   if (task_completed_or_cancelled (data))
     {
@@ -1842,7 +1912,7 @@ g_socket_client_enumerator_callback (GObject      *object,
     }
 
   address = g_socket_address_enumerator_next_finish (data->enumerator,
-						     result, &error);
+						     result, &data->error_info->tmp_error);
   if (address == NULL)
     {
       if (G_UNLIKELY (data->enumeration_completed))
@@ -1851,7 +1921,7 @@ g_socket_client_enumerator_callback (GObject      *object,
       data->enumeration_completed = TRUE;
       g_debug ("GSocketClient: Address enumeration completed (out of addresses)");
 
-      /* As per API docs: We only care about error if its the first call,
+      /* As per API docs: We only care about error if it's the first call,
          after that the enumerator is done.
 
          Note that we don't care about cancellation errors because
@@ -1862,20 +1932,11 @@ g_socket_client_enumerator_callback (GObject      *object,
       if ((data->enumerated_at_least_once && !data->connection_attempts && !data->connection_in_progress) ||
           !data->enumerated_at_least_once)
         {
-          g_debug ("GSocketClient: Address enumeration failed: %s", error ? error->message : NULL);
-          if (data->last_error)
-            {
-              g_clear_error (&error);
-              error = data->last_error;
-              data->last_error = NULL;
-            }
-          else if (!error)
-            {
-              g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_FAILED,
-                _("Unknown error on connect"));
-            }
-
-          complete_connection_with_error (data, error);
+          g_debug ("GSocketClient: Address enumeration failed: %s",
+                   data->error_info->tmp_error ? data->error_info->tmp_error->message : NULL);
+          consider_tmp_error (data->error_info, G_SOCKET_CLIENT_RESOLVING);
+          g_assert (data->error_info->best_error);
+          complete_connection_with_error (data, g_steal_pointer (&data->error_info->best_error));
         }
 
       /* Enumeration should never trigger again, drop our ref */
@@ -1883,17 +1944,19 @@ g_socket_client_enumerator_callback (GObject      *object,
       return;
     }
 
-  data->enumerated_at_least_once = TRUE;
   g_debug ("GSocketClient: Address enumeration succeeded");
-  g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVED,
-			      data->connectable, NULL);
-
-  g_clear_error (&data->last_error);
+  if (!data->enumerated_at_least_once)
+    {
+      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVED,
+				  data->connectable, NULL);
+      data->enumerated_at_least_once = TRUE;
+    }
 
-  socket = create_socket (data->client, address, &data->last_error);
+  socket = create_socket (data->client, address, &data->error_info->tmp_error);
   if (socket == NULL)
     {
       g_object_unref (address);
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_CONNECTING);
       enumerator_next_async (data, FALSE);
       return;
     }
@@ -1936,6 +1999,15 @@ g_socket_client_enumerator_callback (GObject      *object,
  *
  * This is the asynchronous version of g_socket_client_connect().
  *
+ * You may wish to prefer the asynchronous version even in synchronous
+ * command line programs because, since 2.60, it implements
+ * [RFC 8305](https://tools.ietf.org/html/rfc8305) "Happy Eyeballs"
+ * recommendations to work around long connection timeouts in networks
+ * where IPv6 is broken by performing an IPv4 connection simultaneously
+ * without waiting for IPv6 to time out, which is not supported by the
+ * synchronous call. (This is not an API guarantee, and may change in
+ * the future.)
+ *
  * When the operation is finished @callback will be
  * called. You can then call g_socket_client_connect_finish() to get
  * the result of the operation.
@@ -1956,6 +2028,7 @@ g_socket_client_connect_async (GSocketClient       *client,
   data = g_slice_new0 (GSocketClientAsyncConnectData);
   data->client = client;
   data->connectable = g_object_ref (connectable);
+  data->error_info = socket_client_error_info_new ();
 
   if (can_use_proxy (client))
     {