From 69c641318226dc71813ff256437c278936c43914 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 23 Mar 2023 16:36:37 +0000 Subject: [PATCH 1/8] gsocketclient: Rename an internal variable and change it to a counter This introduces no functional changes, but makes it a little clearer what the variable signifies, and provides a little more information when debugging things. Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index 4f7cb90dc..7546e8293 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -1501,7 +1501,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; @@ -1660,7 +1663,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, @@ -2015,8 +2018,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) || - !data->enumerated_at_least_once) + if ((data->n_addresses_enumerated > 0 && !data->connection_attempts && !data->connection_in_progress) || + 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 +2034,11 @@ g_socket_client_enumerator_callback (GObject *object, } g_debug ("GSocketClient: Address enumeration succeeded"); - 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; - } + if (data->n_addresses_enumerated == 0) + g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVED, + data->connectable, NULL); + + data->n_addresses_enumerated++; socket = create_socket (data->client, address, &data->error_info->tmp_error); if (socket == NULL) From 240b8bbc2dbb182a2c71747bd3d3f5361d0563c4 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 23 Mar 2023 17:39:54 +0000 Subject: [PATCH 2/8] gsocketclient: Clarify some internal comments Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index 7546e8293..3f5870475 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -1799,6 +1799,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) { @@ -1884,13 +1886,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); From bc6d03c3c9573f78dd391ff0dbe6201f2073cc69 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 23 Mar 2023 18:35:11 +0000 Subject: [PATCH 3/8] gsocketclient: Document async operation timeout/completion behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These calls are where the `GSocketClient` Happy Eyeballs code relies on other components within GLib (and glib-networking) to complete asynchronous operations in a timely manner. `GSocketClient` doesn’t add its own timeouts to monitor these async operations, so if the implementations are buggy then a `GSocketClient` operation could stall forever. Make that a bit clearer. Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 14 ++++++++++++++ gio/gsocketconnection.c | 4 ++++ 2 files changed, 18 insertions(+) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index 3f5870475..df6d0c6fb 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -1727,6 +1727,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), @@ -1854,6 +1858,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, @@ -2079,6 +2089,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, diff --git a/gio/gsocketconnection.c b/gio/gsocketconnection.c index c799ac08c..c535438fb 100644 --- a/gio/gsocketconnection.c +++ b/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 From 320c9d6d0de5ee51992b72a6ef231dc205ffbbcc Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 24 Apr 2023 16:29:41 +0100 Subject: [PATCH 4/8] gsocketclient: Add some additional debug prints These make it a bit easier to track the ongoing tasks, as the tasks and/or their closures are not tracked in a big list somewhere. Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index df6d0c6fb..6ae968b48 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -2071,6 +2071,9 @@ g_socket_client_enumerator_callback (GObject *object, attempt->connection = (GIOStream *)g_socket_connection_factory_create_connection (socket); attempt->timeout_source = g_timeout_source_new (HAPPY_EYEBALLS_CONNECTION_ATTEMPT_TIMEOUT_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)); @@ -2200,6 +2203,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); } From a581de2ee72369b1e1e7f90841d51d068d48da3b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 2 May 2023 12:19:35 +0100 Subject: [PATCH 5/8] gsocketclient: Make connection_attempt_remove() safe to call twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As spotted by Michael Catanzaro in https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3394#note_1730123, on the code path where a `ConnectionAttempt` is cancelled, it will currently be removed from the `connection_attempts` list by the cancellation code, and then *again* by the `if (task_completed_or_cancelled ())` code in `g_socket_client_connected_callback()`. That would previously have resulted in a double-unref of the `ConnectionAttempt`. So change `connection_attempt_remove()` to be a no-op if the attempt isn’t found in `connection_attempts`. Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index 6ae968b48..eb552e17b 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -1587,8 +1587,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); - connection_attempt_unref (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 From ac6864ded7d941f4f322e74132fd3109c0b9696b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 29 Nov 2023 15:50:03 +0000 Subject: [PATCH 6/8] =?UTF-8?q?gsocketclient:=20Rename=20=E2=80=98connecti?= =?UTF-8?q?on=20timeout=E2=80=99=20to=20=E2=80=98connection=20delay?= =?UTF-8?q?=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes it match the terminology from RFC 8305 better, which refers to a ‘connection attempt delay’. This is a delay because it determines the spacing between trying additional connection attempts. It’s not a timeout because it shouldn’t cause cancellation of any ongoing connection attempts. This commit introduces no functional changes. Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index eb552e17b..55b336cb6 100644 --- a/gio/gsocketclient.c +++ b/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: @@ -1539,7 +1541,7 @@ typedef struct GIOStream *connection; GProxyAddress *proxy_addr; GSocketClientAsyncConnectData *data; /* unowned */ - GSource *timeout_source; + GSource *delay_timeout_source; /* (owned) */ 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; @@ -1575,10 +1577,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_unref (attempt->timeout_source); + g_source_destroy (attempt->delay_timeout_source); + g_source_unref (attempt->delay_timeout_source); } g_free (attempt); } @@ -1933,10 +1935,10 @@ g_socket_client_connected_callback (GObject *source, return; } - if (attempt->timeout_source) + if (attempt->delay_timeout_source) { - g_source_destroy (attempt->timeout_source); - g_clear_pointer (&attempt->timeout_source, g_source_unref); + g_source_destroy (attempt->delay_timeout_source); + g_clear_pointer (&attempt->delay_timeout_source, g_source_unref); } if (!g_socket_connection_connect_finish (G_SOCKET_CONNECTION (source), @@ -1979,17 +1981,17 @@ 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; 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; } @@ -2073,7 +2075,7 @@ 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); @@ -2081,8 +2083,8 @@ g_socket_client_enumerator_callback (GObject *object, 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_attach (attempt->timeout_source, g_task_get_context (data->task)); + g_source_set_callback (attempt->delay_timeout_source, on_connection_attempt_delay_reached, attempt, NULL); + 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)) @@ -2178,7 +2180,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. From f5f418b0574e8ec12e2942abdea0209585172f14 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 29 Nov 2023 16:18:00 +0000 Subject: [PATCH 7/8] gsocketclient: Track whether the connection attempt delay is reached Just for debugging purposes, track whether the Connection Attempt Delay (https://datatracker.ietf.org/doc/html/rfc8305#section-8) has been reached for each attempt. This makes it a bit easier to diagnose `GSocketClient` problems in a debugger. Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index 55b336cb6..e95df575a 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -1542,6 +1542,7 @@ typedef struct GProxyAddress *proxy_addr; GSocketClientAsyncConnectData *data; /* unowned */ 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; @@ -1985,6 +1986,9 @@ 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: Connection attempt delay reached, trying another enumeration"); From da48656ee7b8844cd3fa8ebbb931fe1451d17d1e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 29 Nov 2023 16:19:06 +0000 Subject: [PATCH 8/8] gsocketclient: Add a missing connection_attempt_remove() call The connection attempt should always be removed before being unreffed. Signed-off-by: Philip Withnall --- gio/gsocketclient.c | 1 + 1 file changed, 1 insertion(+) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index e95df575a..1127e4599 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -1932,6 +1932,7 @@ 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; }