Merge branch 'socket-client-cleanups' into 'main'

gsocketclient: Document delays/timeouts better

See merge request GNOME/glib!3394

gio/gsocketclient.c

@@ -60,8 +60,10 @@
 
 /* As recommended by RFC 8305 this is the time it waits
  * on a connection before starting another concurrent attempt.
+ *
+ * See https://datatracker.ietf.org/doc/html/rfc8305#section-8
  */
-#define HAPPY_EYEBALLS_CONNECTION_ATTEMPT_TIMEOUT_MS 250
+#define HAPPY_EYEBALLS_CONNECTION_ATTEMPT_DELAY_MS 250
 
 /**
  * GSocketClient:
@@ -1501,7 +1503,10 @@ typedef struct
   GSList *successful_connections;
   SocketClientErrorInfo *error_info;
 
-  gboolean enumerated_at_least_once;
+  /* Number of times g_socket_address_enumerator_next_async() has successfully
+   * returned an address. */
+  guint n_addresses_enumerated;
+
   gboolean enumeration_completed;
   gboolean connection_in_progress;
   gboolean completed;
@@ -1536,7 +1541,8 @@ typedef struct
   GIOStream *connection;
   GProxyAddress *proxy_addr;
   GSocketClientAsyncConnectData *data; /* unowned */
-  GSource *timeout_source;
+  GSource *delay_timeout_source;  /* (owned) */
+  gboolean delay_reached;
   GCancellable *cancellable;
   GCancellable *task_cancellable;  /* (owned); this is equal to g_task_get_cancellable (ConnectionAttempt.data->task), but with a longer lifetime */
   gulong cancelled_id;
@@ -1572,10 +1578,10 @@ connection_attempt_unref (gpointer pointer)
       attempt->cancelled_id = 0;
       g_clear_object (&attempt->cancellable);
       g_clear_object (&attempt->proxy_addr);
-      if (attempt->timeout_source)
+      if (attempt->delay_timeout_source)
         {
-          g_source_destroy (attempt->timeout_source);
+          g_source_destroy (attempt->delay_timeout_source);
-          g_source_unref (attempt->timeout_source);
+          g_source_unref (attempt->delay_timeout_source);
         }
       g_free (attempt);
     }
@@ -1584,8 +1590,12 @@ connection_attempt_unref (gpointer pointer)
 static void
 connection_attempt_remove (ConnectionAttempt *attempt)
 {
-  attempt->data->connection_attempts = g_slist_remove (attempt->data->connection_attempts, attempt);
+  GSList *attempt_link = g_slist_find (attempt->data->connection_attempts, attempt);
+  if (attempt_link != NULL)
+    {
+      attempt->data->connection_attempts = g_slist_delete_link (attempt->data->connection_attempts, attempt_link);
       connection_attempt_unref (attempt);
+    }
 }
 
 static void
@@ -1660,7 +1670,7 @@ enumerator_next_async (GSocketClientAsyncConnectData *data,
   if (add_task_ref)
     g_object_ref (data->task);
 
-  if (!data->enumerated_at_least_once)
+  if (data->n_addresses_enumerated == 0)
     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,
@@ -1724,6 +1734,10 @@ G_GNUC_BEGIN_IGNORE_DEPRECATIONS
                                                     data->client->priv->tls_validation_flags);
 G_GNUC_END_IGNORE_DEPRECATIONS
       g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_TLS_HANDSHAKING, data->connectable, G_IO_STREAM (tlsconn));
+
+      /* This operation will time out if the underlying #GSocket times out on
+       * any part of the TLS handshake. It does not have a higher-level
+       * timeout. */
       g_tls_connection_handshake_async (G_TLS_CONNECTION (tlsconn),
 					G_PRIORITY_DEFAULT,
 					g_task_get_cancellable (data->task),
@@ -1796,6 +1810,8 @@ task_completed_or_cancelled (GSocketClientAsyncConnectData *data)
     return FALSE;
 }
 
+/* Returns %TRUE if it’s popped a connection of data->successful_connections and
+ * successfully started the next ongoing async operation for that connection. */
 static gboolean
 try_next_successful_connection (GSocketClientAsyncConnectData *data)
 {
@@ -1849,6 +1865,12 @@ try_next_successful_connection (GSocketClientAsyncConnectData *data)
 
       g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATING, data->connectable, attempt->connection);
       g_debug ("GSocketClient: Starting proxy connection");
+
+      /* FIXME: The #GProxy implementations do not have well-defined timeout
+       * behaviour, so this operation may theoretically never complete or time
+       * out. In practice, all #GProxy implementations use a #GSocket and a
+       * default timeout on that will eventually be hit. But there is no
+       * higher-level timeout. */
       g_proxy_connect_async (proxy,
                              connection,
                              proxy_addr,
@@ -1881,13 +1903,15 @@ try_next_connection_or_finish (GSocketClientAsyncConnectData *data,
   if (data->connection_in_progress)
     return;
 
-  /* Keep trying successful connections until one works, each iteration pops one */
+  /* Try to pop and make progress on the next successful_connection. */
   while (data->successful_connections)
     {
       if (try_next_successful_connection (data))
         return;
     }
 
+  /* If there are no more successful_connections which we can make progress on,
+   * try the next address enumeration. */
   if (!data->enumeration_completed)
     {
       enumerator_next_async (data, FALSE);
@@ -1908,14 +1932,15 @@ g_socket_client_connected_callback (GObject      *source,
   if (task_completed_or_cancelled (data) || g_cancellable_is_cancelled (attempt->cancellable))
     {
       g_object_unref (data->task);
+      connection_attempt_remove (attempt);
       connection_attempt_unref (attempt);
       return;
     }
 
-  if (attempt->timeout_source)
+  if (attempt->delay_timeout_source)
     {
-      g_source_destroy (attempt->timeout_source);
+      g_source_destroy (attempt->delay_timeout_source);
-      g_clear_pointer (&attempt->timeout_source, g_source_unref);
+      g_clear_pointer (&attempt->delay_timeout_source, g_source_unref);
     }
 
   if (!g_socket_connection_connect_finish (G_SOCKET_CONNECTION (source),
@@ -1958,17 +1983,20 @@ g_socket_client_connected_callback (GObject      *source,
 }
 
 static gboolean
-on_connection_attempt_timeout (gpointer data)
+on_connection_attempt_delay_reached (gpointer data)
 {
   ConnectionAttempt *attempt = data;
 
+  g_assert (!attempt->delay_reached);
+  attempt->delay_reached = TRUE;
+
   if (!attempt->data->enumeration_completed)
     {
-      g_debug ("GSocketClient: Timeout reached, trying another enumeration");
+      g_debug ("GSocketClient: Connection attempt delay reached, trying another enumeration");
       enumerator_next_async (attempt->data, TRUE);
     }
 
-  g_clear_pointer (&attempt->timeout_source, g_source_unref);
+  g_clear_pointer (&attempt->delay_timeout_source, g_source_unref);
   return G_SOURCE_REMOVE;
 }
 
@@ -2015,8 +2043,8 @@ g_socket_client_enumerator_callback (GObject      *object,
 
          If this fails and nothing is in progress then we will complete task here.
        */
-      if ((data->enumerated_at_least_once && !data->connection_attempts && !data->connection_in_progress) ||
+      if ((data->n_addresses_enumerated > 0 && !data->connection_attempts && !data->connection_in_progress) ||
-          !data->enumerated_at_least_once)
+          data->n_addresses_enumerated == 0)
         {
           g_debug ("GSocketClient: Address enumeration failed: %s",
                    data->error_info->tmp_error ? data->error_info->tmp_error->message : NULL);
@@ -2031,12 +2059,11 @@ g_socket_client_enumerator_callback (GObject      *object,
     }
 
   g_debug ("GSocketClient: Address enumeration succeeded");
-  if (!data->enumerated_at_least_once)
+  if (data->n_addresses_enumerated == 0)
-    {
     g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVED,
                                 data->connectable, NULL);
-      data->enumerated_at_least_once = TRUE;
+
-    }
+  data->n_addresses_enumerated++;
 
   socket = create_socket (data->client, address, &data->error_info->tmp_error);
   if (socket == NULL)
@@ -2053,13 +2080,16 @@ g_socket_client_enumerator_callback (GObject      *object,
   attempt->address = address;
   attempt->cancellable = g_cancellable_new ();
   attempt->connection = (GIOStream *)g_socket_connection_factory_create_connection (socket);
-  attempt->timeout_source = g_timeout_source_new (HAPPY_EYEBALLS_CONNECTION_ATTEMPT_TIMEOUT_MS);
+  attempt->delay_timeout_source = g_timeout_source_new (HAPPY_EYEBALLS_CONNECTION_ATTEMPT_DELAY_MS);
+
+  g_debug ("%s: starting connection attempt %p for GSocketClientAsyncConnectData %p",
+           G_STRFUNC, attempt, data);
 
   if (G_IS_PROXY_ADDRESS (address) && data->client->priv->enable_proxy)
     attempt->proxy_addr = g_object_ref (G_PROXY_ADDRESS (address));
 
-  g_source_set_callback (attempt->timeout_source, on_connection_attempt_timeout, attempt, NULL);
+  g_source_set_callback (attempt->delay_timeout_source, on_connection_attempt_delay_reached, attempt, NULL);
-  g_source_attach (attempt->timeout_source, g_task_get_context (data->task));
+  g_source_attach (attempt->delay_timeout_source, g_task_get_context (data->task));
   data->connection_attempts = g_slist_append (data->connection_attempts, connection_attempt_ref (attempt));
 
   if (g_task_get_cancellable (data->task))
@@ -2073,6 +2103,10 @@ g_socket_client_enumerator_callback (GObject      *object,
   g_socket_connection_set_cached_remote_address ((GSocketConnection *)attempt->connection, address);
   g_debug ("GSocketClient: Starting TCP connection attempt");
   g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_CONNECTING, data->connectable, attempt->connection);
+
+  /* If client->priv->timeout is set, this async operation will time out after
+   * then. Otherwise it will continue until the kernel timeouts for a
+   * non-blocking connect() call (if any) are hit. */
   g_socket_connection_connect_async (G_SOCKET_CONNECTION (attempt->connection),
 				     address,
 				     attempt->cancellable,
@@ -2151,7 +2185,7 @@ g_socket_client_connect_async (GSocketClient       *client,
         - Each connection is independent and kept in a ConnectionAttempt object.
           - They each hold a ref on the main task and have their own cancellable.
         - Multiple attempts may happen in parallel as per Happy Eyeballs.
-        - Upon failure or timeouts more connection attempts are made.
+        - Upon failure or attempt delays being reached more connection attempts are made.
           - If no connections succeed the task errors.
         - Upon success they are kept in a list of successful connections.
 
@@ -2180,6 +2214,10 @@ g_socket_client_connect_async (GSocketClient       *client,
                                  g_object_ref (data->enumeration_cancellable), g_object_unref);
     }
 
+  g_debug ("%s: starting new g_socket_client_connect_async() with GTask %p "
+           "and GSocketClientAsyncConnectData %p",
+           G_STRFUNC, data->task, data);
+
   enumerator_next_async (data, FALSE);
 }
 

gio/gsocketconnection.c

@@ -177,6 +177,10 @@ static gboolean g_socket_connection_connect_callback (GSocket      *socket,
  * This clears the #GSocket:blocking flag on @connection's underlying
  * socket if it is currently set.
  *
+ * If #GSocket:timeout is set, the operation will time out and return
+ * %G_IO_ERROR_TIMED_OUT after that period. Otherwise, it will continue
+ * indefinitely until operating system timeouts (if any) are hit.
+ *
  * Use g_socket_connection_connect_finish() to retrieve the result.
  *
  * Since: 2.32