From c256af1c2d87afaa24c17ada66a4452ab48fa945 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 24 Apr 2023 16:30:45 +0100 Subject: gthreadedresolver: Add some additional debug prints These make it a bit easier to track the ongoing resolver tasks, as the tasks and/or their closures are not tracked in a big list somewhere. Signed-off-by: Philip Withnall --- gio/gthreadedresolver.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c index 68b5c20d7..021409d3c 100644 --- a/gio/gthreadedresolver.c +++ b/gio/gthreadedresolver.c @@ -254,6 +254,10 @@ lookup_by_name_with_flags_async (GResolver *resolver, data = lookup_data_new (hostname, flags_to_family (flags)); task = g_task_new (resolver, cancellable, callback, user_data); + + g_debug ("%s: starting new lookup for %s with GTask %p, LookupData %p", + G_STRFUNC, hostname, task, data); + g_task_set_source_tag (task, lookup_by_name_with_flags_async); g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, data, (GDestroyNotify)lookup_data_free); -- cgit v1.2.1 From bf92bae481be3619c8102340bdf8402f4827586d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 29 Mar 2023 17:13:49 +0100 Subject: gresolver: Add GResolver:timeout property Without a timeout, some lookup requests can go on forever, typically due to bugs in underlying systems. This can have particularly significant effects on the Happy Eyeballs algorithm in `GSocketClient`, which relies on multiple name lookups as its first step. Signed-off-by: Philip Withnall Helps: #2866 --- docs/reference/gio/gio-sections-common.txt | 2 + gio/gresolver.c | 116 ++++++++++++++++++++++++++++- gio/gresolver.h | 5 ++ 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/docs/reference/gio/gio-sections-common.txt b/docs/reference/gio/gio-sections-common.txt index 63d167678..f5737ef42 100644 --- a/docs/reference/gio/gio-sections-common.txt +++ b/docs/reference/gio/gio-sections-common.txt @@ -1934,6 +1934,8 @@ g_resolver_lookup_service g_resolver_lookup_service_async g_resolver_lookup_service_finish g_resolver_free_targets +g_resolver_get_timeout +g_resolver_set_timeout GResolverRecordType g_resolver_lookup_records diff --git a/gio/gresolver.c b/gio/gresolver.c index 6a735e8d9..ac6fa4f7a 100644 --- a/gio/gresolver.c +++ b/gio/gresolver.c @@ -57,6 +57,12 @@ * making it easy to connect to a remote host/service. */ +typedef enum { + PROP_TIMEOUT = 1, +} GResolverProperty; + +static GParamSpec *props[PROP_TIMEOUT + 1] = { NULL, }; + enum { RELOAD, LAST_SIGNAL @@ -65,11 +71,11 @@ enum { static guint signals[LAST_SIGNAL] = { 0 }; struct _GResolverPrivate { + unsigned timeout_ms; + #ifdef G_OS_UNIX GMutex mutex; time_t resolv_conf_timestamp; /* protected by @mutex */ -#else - int dummy; #endif }; @@ -151,6 +157,42 @@ g_resolver_real_lookup_service_finish (GResolver *resolver, return srv_records_to_targets (records); } +static void +g_resolver_get_property (GObject *object, + guint prop_id, + GValue *value, + GParamSpec *pspec) +{ + GResolver *self = G_RESOLVER (object); + + switch ((GResolverProperty) prop_id) + { + case PROP_TIMEOUT: + g_value_set_uint (value, g_resolver_get_timeout (self)); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + } +} + +static void +g_resolver_set_property (GObject *object, + guint prop_id, + const GValue *value, + GParamSpec *pspec) +{ + GResolver *self = G_RESOLVER (object); + + switch ((GResolverProperty) prop_id) + { + case PROP_TIMEOUT: + g_resolver_set_timeout (self, g_value_get_uint (value)); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + } +} + static void g_resolver_finalize (GObject *object) { @@ -168,6 +210,8 @@ g_resolver_class_init (GResolverClass *resolver_class) { GObjectClass *object_class = G_OBJECT_CLASS (resolver_class); + object_class->get_property = g_resolver_get_property; + object_class->set_property = g_resolver_set_property; object_class->finalize = g_resolver_finalize; /* Automatically pass these over to the lookup_records methods */ @@ -175,6 +219,31 @@ g_resolver_class_init (GResolverClass *resolver_class) resolver_class->lookup_service_async = g_resolver_real_lookup_service_async; resolver_class->lookup_service_finish = g_resolver_real_lookup_service_finish; + /** + * GResolver:timeout: + * + * The timeout applied to all resolver lookups, in milliseconds. + * + * This may be changed through the lifetime of the #GResolver. The new value + * will apply to any lookups started after the change, but not to any + * already-ongoing lookups. + * + * If this is `0`, no timeout is applied to lookups. + * + * No timeout was applied to lookups before this property was added in + * GLib 2.78. + * + * Since: 2.78 + */ + props[PROP_TIMEOUT] = + g_param_spec_uint ("timeout", + P_("Timeout"), + P_("Timeout (ms) applied to all resolver lookups"), + 0, G_MAXUINT, 0, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS | G_PARAM_EXPLICIT_NOTIFY); + + g_object_class_install_properties (object_class, G_N_ELEMENTS (props), props); + /** * GResolver::reload: * @resolver: a #GResolver @@ -1244,6 +1313,49 @@ g_resolver_get_serial (GResolver *resolver) return result; } +/** + * g_resolver_get_timeout: + * @resolver: a #GResolver + * + * Get the timeout applied to all resolver lookups. See #GResolver:timeout. + * + * Returns: the resolver timeout, in milliseconds, or `0` for no timeout + * Since: 2.78 + */ +unsigned +g_resolver_get_timeout (GResolver *resolver) +{ + GResolverPrivate *priv = g_resolver_get_instance_private (resolver); + + g_return_val_if_fail (G_IS_RESOLVER (resolver), 0); + + return priv->timeout_ms; +} + +/** + * g_resolver_set_timeout: + * @resolver: a #GResolver + * @timeout_ms: timeout in milliseconds, or `0` for no timeouts + * + * Set the timeout applied to all resolver lookups. See #GResolver:timeout. + * + * Since: 2.78 + */ +void +g_resolver_set_timeout (GResolver *resolver, + unsigned timeout_ms) +{ + GResolverPrivate *priv = g_resolver_get_instance_private (resolver); + + g_return_if_fail (G_IS_RESOLVER (resolver)); + + if (priv->timeout_ms == timeout_ms) + return; + + priv->timeout_ms = timeout_ms; + g_object_notify_by_pspec (G_OBJECT (resolver), props[PROP_TIMEOUT]); +} + /** * g_resolver_error_quark: * diff --git a/gio/gresolver.h b/gio/gresolver.h index 2dffeadbf..9b9a8a81a 100644 --- a/gio/gresolver.h +++ b/gio/gresolver.h @@ -277,6 +277,11 @@ GList *g_resolver_lookup_records_finish (GResolver GIO_AVAILABLE_IN_ALL void g_resolver_free_targets (GList *targets); +GIO_AVAILABLE_IN_2_78 +unsigned g_resolver_get_timeout (GResolver *resolver); +GIO_AVAILABLE_IN_2_78 +void g_resolver_set_timeout (GResolver *resolver, + unsigned timeout_ms); /** * G_RESOLVER_ERROR: -- cgit v1.2.1 From 6e499764e4a4bab98fcd799f0395917367b114b1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 30 Mar 2023 12:02:40 +0100 Subject: gthreadedresolver: Port to G_DECLARE_FINAL_TYPE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The class and its header are not public, so this should not be an API or ABI break. This just simplifies the code a little and allows for easy extension of the object’s private data in future commits. Signed-off-by: Philip Withnall --- gio/gthreadedresolver.c | 4 ++++ gio/gthreadedresolver.h | 25 ++++++++++--------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c index 021409d3c..2fa683fe8 100644 --- a/gio/gthreadedresolver.c +++ b/gio/gthreadedresolver.c @@ -38,6 +38,10 @@ #include "gsocketaddress.h" #include "gsrvtarget.h" +struct _GThreadedResolver +{ + GResolver parent_instance; +}; G_DEFINE_TYPE (GThreadedResolver, g_threaded_resolver, G_TYPE_RESOLVER) diff --git a/gio/gthreadedresolver.h b/gio/gthreadedresolver.h index ea76e4bfd..099df5b84 100644 --- a/gio/gthreadedresolver.h +++ b/gio/gthreadedresolver.h @@ -21,28 +21,23 @@ #ifndef __G_THREADED_RESOLVER_H__ #define __G_THREADED_RESOLVER_H__ +#include #include G_BEGIN_DECLS +/** + * GThreadedResolver: + * + * #GThreadedResolver is an implementation of #GResolver which calls the libc + * lookup functions in threads to allow them to run asynchronously. + * + * Since: 2.20 + */ #define G_TYPE_THREADED_RESOLVER (g_threaded_resolver_get_type ()) -#define G_THREADED_RESOLVER(o) (G_TYPE_CHECK_INSTANCE_CAST ((o), G_TYPE_THREADED_RESOLVER, GThreadedResolver)) -#define G_THREADED_RESOLVER_CLASS(k) (G_TYPE_CHECK_CLASS_CAST((k), G_TYPE_THREADED_RESOLVER, GThreadedResolverClass)) -#define G_IS_THREADED_RESOLVER(o) (G_TYPE_CHECK_INSTANCE_TYPE ((o), G_TYPE_THREADED_RESOLVER)) -#define G_IS_THREADED_RESOLVER_CLASS(k) (G_TYPE_CHECK_CLASS_TYPE ((k), G_TYPE_THREADED_RESOLVER)) -#define G_THREADED_RESOLVER_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), G_TYPE_THREADED_RESOLVER, GThreadedResolverClass)) - -typedef struct { - GResolver parent_instance; -} GThreadedResolver; - -typedef struct { - GResolverClass parent_class; - -} GThreadedResolverClass; GIO_AVAILABLE_IN_ALL -GType g_threaded_resolver_get_type (void) G_GNUC_CONST; +G_DECLARE_FINAL_TYPE (GThreadedResolver, g_threaded_resolver, G, THREADED_RESOLVER, GResolver) /* Used for a private test API */ #ifdef G_OS_UNIX -- cgit v1.2.1 From 694394207c32a398f90dbcc175407081204bee60 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 25 Apr 2023 15:56:47 +0100 Subject: gthreadedresolver: Combine closure structs for different lookup types This introduces no functional changes, but will make a reorganisation of the code simpler in the next commit. Rather than dealing with three different closure types, this changes the code to deal with one which is a tagged union of the three. Signed-off-by: Philip Withnall --- gio/gthreadedresolver.c | 144 +++++++++++++++++++++++++++++++----------------- 1 file changed, 93 insertions(+), 51 deletions(-) diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c index 2fa683fe8..96ea3cd44 100644 --- a/gio/gthreadedresolver.c +++ b/gio/gthreadedresolver.c @@ -71,24 +71,75 @@ g_resolver_error_from_addrinfo_error (gint err) } typedef struct { - char *hostname; - int address_family; + enum { + LOOKUP_BY_NAME, + LOOKUP_BY_ADDRESS, + LOOKUP_RECORDS, + } lookup_type; + + union { + struct { + char *hostname; + int address_family; + } lookup_by_name; + struct { + GInetAddress *address; /* (owned) */ + } lookup_by_address; + struct { + char *rrname; + GResolverRecordType record_type; + } lookup_records; + }; } LookupData; static LookupData * -lookup_data_new (const char *hostname, - int address_family) +lookup_data_new_by_name (const char *hostname, + int address_family) { - LookupData *data = g_new (LookupData, 1); - data->hostname = g_strdup (hostname); - data->address_family = address_family; - return data; + LookupData *data = g_new0 (LookupData, 1); + data->lookup_type = LOOKUP_BY_NAME; + data->lookup_by_name.hostname = g_strdup (hostname); + data->lookup_by_name.address_family = address_family; + return g_steal_pointer (&data); +} + +static LookupData * +lookup_data_new_by_address (GInetAddress *address) +{ + LookupData *data = g_new0 (LookupData, 1); + data->lookup_type = LOOKUP_BY_ADDRESS; + data->lookup_by_address.address = g_object_ref (address); + return g_steal_pointer (&data); +} + +static LookupData * +lookup_data_new_records (const gchar *rrname, + GResolverRecordType record_type) +{ + LookupData *data = g_new0 (LookupData, 1); + data->lookup_type = LOOKUP_RECORDS; + data->lookup_records.rrname = g_strdup (rrname); + data->lookup_records.record_type = record_type; + return g_steal_pointer (&data); } static void lookup_data_free (LookupData *data) { - g_free (data->hostname); + switch (data->lookup_type) { + case LOOKUP_BY_NAME: + g_free (data->lookup_by_name.hostname); + break; + case LOOKUP_BY_ADDRESS: + g_clear_object (&data->lookup_by_address.address); + break; + case LOOKUP_RECORDS: + g_free (data->lookup_records.rrname); + break; + default: + g_assert_not_reached (); + } + g_free (data); } @@ -99,7 +150,7 @@ do_lookup_by_name (GTask *task, GCancellable *cancellable) { LookupData *lookup_data = task_data; - const char *hostname = lookup_data->hostname; + const char *hostname = lookup_data->lookup_by_name.hostname; struct addrinfo *res = NULL; GList *addresses; gint retval; @@ -115,7 +166,7 @@ do_lookup_by_name (GTask *task, addrinfo_hints.ai_socktype = SOCK_STREAM; addrinfo_hints.ai_protocol = IPPROTO_TCP; - addrinfo_hints.ai_family = lookup_data->address_family; + addrinfo_hints.ai_family = lookup_data->lookup_by_name.address_family; retval = getaddrinfo (hostname, NULL, &addrinfo_hints, &res); if (retval == 0) @@ -190,11 +241,11 @@ lookup_by_name (GResolver *resolver, GList *addresses; LookupData *data; - data = lookup_data_new (hostname, AF_UNSPEC); + data = lookup_data_new_by_name (hostname, AF_UNSPEC); task = g_task_new (resolver, cancellable, NULL, NULL); g_task_set_source_tag (task, lookup_by_name); g_task_set_name (task, "[gio] resolver lookup"); - g_task_set_task_data (task, data, (GDestroyNotify)lookup_data_free); + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); g_task_run_in_thread_sync (task, do_lookup_by_name); addresses = g_task_propagate_pointer (task, error); @@ -232,11 +283,11 @@ lookup_by_name_with_flags (GResolver *resolver, GList *addresses; LookupData *data; - data = lookup_data_new (hostname, flags_to_family (flags)); + data = lookup_data_new_by_name (hostname, flags_to_family (flags)); task = g_task_new (resolver, cancellable, NULL, NULL); g_task_set_source_tag (task, lookup_by_name_with_flags); g_task_set_name (task, "[gio] resolver lookup"); - g_task_set_task_data (task, data, (GDestroyNotify)lookup_data_free); + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); g_task_run_in_thread_sync (task, do_lookup_by_name); addresses = g_task_propagate_pointer (task, error); @@ -256,7 +307,7 @@ lookup_by_name_with_flags_async (GResolver *resolver, GTask *task; LookupData *data; - data = lookup_data_new (hostname, flags_to_family (flags)); + data = lookup_data_new_by_name (hostname, flags_to_family (flags)); task = g_task_new (resolver, cancellable, callback, user_data); g_debug ("%s: starting new lookup for %s with GTask %p, LookupData %p", @@ -264,7 +315,7 @@ lookup_by_name_with_flags_async (GResolver *resolver, g_task_set_source_tag (task, lookup_by_name_with_flags_async); g_task_set_name (task, "[gio] resolver lookup"); - g_task_set_task_data (task, data, (GDestroyNotify)lookup_data_free); + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); g_task_run_in_thread (task, do_lookup_by_name); g_object_unref (task); @@ -311,7 +362,8 @@ do_lookup_by_address (GTask *task, gpointer task_data, GCancellable *cancellable) { - GInetAddress *address = task_data; + LookupData *data = task_data; + GInetAddress *address = data->lookup_by_address.address; struct sockaddr_storage sockaddr_address; gsize sockaddr_address_size; GSocketAddress *gsockaddr; @@ -358,13 +410,15 @@ lookup_by_address (GResolver *resolver, GCancellable *cancellable, GError **error) { + LookupData *data = NULL; GTask *task; gchar *name; + data = lookup_data_new_by_address (address); task = g_task_new (resolver, cancellable, NULL, NULL); g_task_set_source_tag (task, lookup_by_address); g_task_set_name (task, "[gio] resolver lookup"); - g_task_set_task_data (task, g_object_ref (address), g_object_unref); + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); g_task_run_in_thread_sync (task, do_lookup_by_address); name = g_task_propagate_pointer (task, error); @@ -380,12 +434,14 @@ lookup_by_address_async (GResolver *resolver, GAsyncReadyCallback callback, gpointer user_data) { + LookupData *data = NULL; GTask *task; + data = lookup_data_new_by_address (address); task = g_task_new (resolver, cancellable, callback, user_data); g_task_set_source_tag (task, lookup_by_address_async); g_task_set_name (task, "[gio] resolver lookup"); - g_task_set_task_data (task, g_object_ref (address), g_object_unref); + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); g_task_run_in_thread (task, do_lookup_by_address); g_object_unref (task); @@ -1074,18 +1130,6 @@ g_resolver_records_from_DnsQuery (const gchar *rrname, #endif -typedef struct { - char *rrname; - GResolverRecordType record_type; -} LookupRecordsData; - -static void -free_lookup_records_data (LookupRecordsData *lrd) -{ - g_free (lrd->rrname); - g_slice_free (LookupRecordsData, lrd); -} - static void free_records (GList *records) { @@ -1107,7 +1151,9 @@ do_lookup_records (GTask *task, gpointer task_data, GCancellable *cancellable) { - LookupRecordsData *lrd = task_data; + LookupData *data = task_data; + const gchar *rrname = data->lookup_records.rrname; + GResolverRecordType record_type = data->lookup_records.record_type; GList *records; GError *error = NULL; @@ -1134,20 +1180,20 @@ do_lookup_records (GTask *task, if (res_ninit (&res) != 0) { g_task_return_new_error (task, G_RESOLVER_ERROR, G_RESOLVER_ERROR_INTERNAL, - _("Error resolving “%s”"), lrd->rrname); + _("Error resolving “%s”"), rrname); return; } #endif - rrtype = g_resolver_record_type_to_rrtype (lrd->record_type); + rrtype = g_resolver_record_type_to_rrtype (record_type); answer = g_byte_array_new (); for (;;) { g_byte_array_set_size (answer, len * 2); #if defined(HAVE_RES_NQUERY) - len = res_nquery (&res, lrd->rrname, C_IN, rrtype, answer->data, answer->len); + len = res_nquery (&res, rrname, C_IN, rrtype, answer->data, answer->len); #else - len = res_query (lrd->rrname, C_IN, rrtype, answer->data, answer->len); + len = res_query (rrname, C_IN, rrtype, answer->data, answer->len); #endif /* If answer fit in the buffer then we're done */ @@ -1161,7 +1207,7 @@ do_lookup_records (GTask *task, } herr = h_errno; - records = g_resolver_records_from_res_query (lrd->rrname, rrtype, answer->data, len, herr, &error); + records = g_resolver_records_from_res_query (rrname, rrtype, answer->data, len, herr, &error); g_byte_array_free (answer, TRUE); #ifdef HAVE_RES_NQUERY @@ -1182,9 +1228,9 @@ do_lookup_records (GTask *task, DNS_RECORD *results = NULL; WORD dnstype; - dnstype = g_resolver_record_type_to_dnstype (lrd->record_type); - status = DnsQuery_A (lrd->rrname, dnstype, DNS_QUERY_STANDARD, NULL, &results, NULL); - records = g_resolver_records_from_DnsQuery (lrd->rrname, dnstype, status, results, &error); + dnstype = g_resolver_record_type_to_dnstype (record_type); + status = DnsQuery_A (rrname, dnstype, DNS_QUERY_STANDARD, NULL, &results, NULL); + records = g_resolver_records_from_DnsQuery (rrname, dnstype, status, results, &error); if (results != NULL) DnsRecordListFree (results, DnsFreeRecordList); @@ -1205,16 +1251,14 @@ lookup_records (GResolver *resolver, { GTask *task; GList *records; - LookupRecordsData *lrd; + LookupData *data = NULL; task = g_task_new (resolver, cancellable, NULL, NULL); g_task_set_source_tag (task, lookup_records); g_task_set_name (task, "[gio] resolver lookup records"); - lrd = g_slice_new (LookupRecordsData); - lrd->rrname = g_strdup (rrname); - lrd->record_type = record_type; - g_task_set_task_data (task, lrd, (GDestroyNotify) free_lookup_records_data); + data = lookup_data_new_records (rrname, record_type); + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); g_task_run_in_thread_sync (task, do_lookup_records); @@ -1233,16 +1277,14 @@ lookup_records_async (GResolver *resolver, gpointer user_data) { GTask *task; - LookupRecordsData *lrd; + LookupData *data = NULL; task = g_task_new (resolver, cancellable, callback, user_data); g_task_set_source_tag (task, lookup_records_async); g_task_set_name (task, "[gio] resolver lookup records"); - lrd = g_slice_new (LookupRecordsData); - lrd->rrname = g_strdup (rrname); - lrd->record_type = record_type; - g_task_set_task_data (task, lrd, (GDestroyNotify) free_lookup_records_data); + data = lookup_data_new_records (rrname, record_type); + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); g_task_run_in_thread (task, do_lookup_records); -- cgit v1.2.1 From 84074ce757d090f52f6701f43b99a41517131433 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 30 Mar 2023 13:42:19 +0100 Subject: gthreadedresolver: Centralise GTask return handling in worker threads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will make it simpler to handle timeouts and cancellation in future, as all the logic for working out whether to return will all be in one place, and all the lookup-specific code is now implemented in simple sync functions which don’t need to care about `GTask`s. This commit introduces no functional changes, it’s just setting up for the following commit. Signed-off-by: Philip Withnall --- gio/gthreadedresolver.c | 176 +++++++++++++++++++++++++++++++----------------- 1 file changed, 113 insertions(+), 63 deletions(-) diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c index 96ea3cd44..4e5b20751 100644 --- a/gio/gthreadedresolver.c +++ b/gio/gthreadedresolver.c @@ -45,6 +45,11 @@ struct _GThreadedResolver G_DEFINE_TYPE (GThreadedResolver, g_threaded_resolver, G_TYPE_RESOLVER) +static void threaded_resolver_worker_cb (GTask *task, + gpointer source_object, + gpointer task_data, + GCancellable *cancellable); + static void g_threaded_resolver_init (GThreadedResolver *gtr) { @@ -143,14 +148,12 @@ lookup_data_free (LookupData *data) g_free (data); } -static void -do_lookup_by_name (GTask *task, - gpointer source_object, - gpointer task_data, - GCancellable *cancellable) +static GList * +do_lookup_by_name (const gchar *hostname, + int address_family, + GCancellable *cancellable, + GError **error) { - LookupData *lookup_data = task_data; - const char *hostname = lookup_data->lookup_by_name.hostname; struct addrinfo *res = NULL; GList *addresses; gint retval; @@ -166,7 +169,7 @@ do_lookup_by_name (GTask *task, addrinfo_hints.ai_socktype = SOCK_STREAM; addrinfo_hints.ai_protocol = IPPROTO_TCP; - addrinfo_hints.ai_family = lookup_data->lookup_by_name.address_family; + addrinfo_hints.ai_family = address_family; retval = getaddrinfo (hostname, NULL, &addrinfo_hints, &res); if (retval == 0) @@ -192,21 +195,23 @@ do_lookup_by_name (GTask *task, g_object_unref (sockaddr); } + g_clear_pointer (&res, freeaddrinfo); + if (addresses != NULL) { addresses = g_list_reverse (addresses); - g_task_return_pointer (task, addresses, - (GDestroyNotify)g_resolver_free_addresses); + return g_steal_pointer (&addresses); } else { /* All addresses failed to be converted to GSocketAddresses. */ - g_task_return_new_error (task, - G_RESOLVER_ERROR, - G_RESOLVER_ERROR_NOT_FOUND, - _("Error resolving “%s”: %s"), - hostname, - _("No valid addresses were found")); + g_set_error (error, + G_RESOLVER_ERROR, + G_RESOLVER_ERROR_NOT_FOUND, + _("Error resolving “%s”: %s"), + hostname, + _("No valid addresses were found")); + return NULL; } } else @@ -219,16 +224,17 @@ do_lookup_by_name (GTask *task, error_message = g_strdup ("[Invalid UTF-8]"); #endif - g_task_return_new_error (task, - G_RESOLVER_ERROR, - g_resolver_error_from_addrinfo_error (retval), - _("Error resolving “%s”: %s"), - hostname, error_message); + g_clear_pointer (&res, freeaddrinfo); + + g_set_error (error, + G_RESOLVER_ERROR, + g_resolver_error_from_addrinfo_error (retval), + _("Error resolving “%s”: %s"), + hostname, error_message); g_free (error_message); - } - if (res) - freeaddrinfo (res); + return NULL; + } } static GList * @@ -247,7 +253,7 @@ lookup_by_name (GResolver *resolver, g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, do_lookup_by_name); + g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); addresses = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -289,7 +295,7 @@ lookup_by_name_with_flags (GResolver *resolver, g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, do_lookup_by_name); + g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); addresses = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -317,7 +323,7 @@ lookup_by_name_with_flags_async (GResolver *resolver, g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread (task, do_lookup_by_name); + g_task_run_in_thread (task, threaded_resolver_worker_cb); g_object_unref (task); } @@ -356,14 +362,11 @@ lookup_by_name_with_flags_finish (GResolver *resolver, return g_task_propagate_pointer (G_TASK (result), error); } -static void -do_lookup_by_address (GTask *task, - gpointer source_object, - gpointer task_data, - GCancellable *cancellable) +static gchar * +do_lookup_by_address (GInetAddress *address, + GCancellable *cancellable, + GError **error) { - LookupData *data = task_data; - GInetAddress *address = data->lookup_by_address.address; struct sockaddr_storage sockaddr_address; gsize sockaddr_address_size; GSocketAddress *gsockaddr; @@ -379,7 +382,7 @@ do_lookup_by_address (GTask *task, retval = getnameinfo ((struct sockaddr *) &sockaddr_address, sockaddr_address_size, name, sizeof (name), NULL, 0, NI_NAMEREQD); if (retval == 0) - g_task_return_pointer (task, g_strdup (name), g_free); + return g_strdup (name); else { gchar *phys; @@ -393,14 +396,16 @@ do_lookup_by_address (GTask *task, #endif phys = g_inet_address_to_string (address); - g_task_return_new_error (task, - G_RESOLVER_ERROR, - g_resolver_error_from_addrinfo_error (retval), - _("Error reverse-resolving “%s”: %s"), - phys ? phys : "(unknown)", - error_message); + g_set_error (error, + G_RESOLVER_ERROR, + g_resolver_error_from_addrinfo_error (retval), + _("Error reverse-resolving “%s”: %s"), + phys ? phys : "(unknown)", + error_message); g_free (phys); g_free (error_message); + + return NULL; } } @@ -420,7 +425,7 @@ lookup_by_address (GResolver *resolver, g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, do_lookup_by_address); + g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); name = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -443,7 +448,7 @@ lookup_by_address_async (GResolver *resolver, g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread (task, do_lookup_by_address); + g_task_run_in_thread (task, threaded_resolver_worker_cb); g_object_unref (task); } @@ -1145,17 +1150,13 @@ int res_query(const char *, int, int, u_char *, int); #endif #endif -static void -do_lookup_records (GTask *task, - gpointer source_object, - gpointer task_data, - GCancellable *cancellable) +static GList * +do_lookup_records (const gchar *rrname, + GResolverRecordType record_type, + GCancellable *cancellable, + GError **error) { - LookupData *data = task_data; - const gchar *rrname = data->lookup_records.rrname; - GResolverRecordType record_type = data->lookup_records.record_type; GList *records; - GError *error = NULL; #if defined(G_OS_UNIX) gint len = 512; @@ -1179,9 +1180,9 @@ do_lookup_records (GTask *task, struct __res_state res = { 0, }; if (res_ninit (&res) != 0) { - g_task_return_new_error (task, G_RESOLVER_ERROR, G_RESOLVER_ERROR_INTERNAL, - _("Error resolving “%s”"), rrname); - return; + g_set_error (error, G_RESOLVER_ERROR, G_RESOLVER_ERROR_INTERNAL, + _("Error resolving “%s”"), rrname); + return NULL; } #endif @@ -1207,7 +1208,7 @@ do_lookup_records (GTask *task, } herr = h_errno; - records = g_resolver_records_from_res_query (rrname, rrtype, answer->data, len, herr, &error); + records = g_resolver_records_from_res_query (rrname, rrtype, answer->data, len, herr, error); g_byte_array_free (answer, TRUE); #ifdef HAVE_RES_NQUERY @@ -1230,16 +1231,13 @@ do_lookup_records (GTask *task, dnstype = g_resolver_record_type_to_dnstype (record_type); status = DnsQuery_A (rrname, dnstype, DNS_QUERY_STANDARD, NULL, &results, NULL); - records = g_resolver_records_from_DnsQuery (rrname, dnstype, status, results, &error); + records = g_resolver_records_from_DnsQuery (rrname, dnstype, status, results, error); if (results != NULL) DnsRecordListFree (results, DnsFreeRecordList); #endif - if (records) - g_task_return_pointer (task, records, (GDestroyNotify) free_records); - else - g_task_return_error (task, error); + return g_steal_pointer (&records); } static GList * @@ -1261,7 +1259,7 @@ lookup_records (GResolver *resolver, g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, do_lookup_records); + g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); records = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -1287,7 +1285,7 @@ lookup_records_async (GResolver *resolver, g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread (task, do_lookup_records); + g_task_run_in_thread (task, threaded_resolver_worker_cb); g_object_unref (task); } @@ -1301,6 +1299,58 @@ lookup_records_finish (GResolver *resolver, return g_task_propagate_pointer (G_TASK (result), error); } +static void +threaded_resolver_worker_cb (GTask *task, + gpointer source_object, + gpointer task_data, + GCancellable *cancellable) +{ + LookupData *data = task_data; + GError *local_error = NULL; + + switch (data->lookup_type) { + case LOOKUP_BY_NAME: + { + GList *addresses = do_lookup_by_name (data->lookup_by_name.hostname, + data->lookup_by_name.address_family, + cancellable, + &local_error); + + if (addresses != NULL) + g_task_return_pointer (task, g_steal_pointer (&addresses), (GDestroyNotify) g_resolver_free_addresses); + else + g_task_return_error (task, g_steal_pointer (&local_error)); + } + break; + case LOOKUP_BY_ADDRESS: + { + gchar *name = do_lookup_by_address (data->lookup_by_address.address, + cancellable, + &local_error); + + if (name != NULL) + g_task_return_pointer (task, g_steal_pointer (&name), g_free); + else + g_task_return_error (task, g_steal_pointer (&local_error)); + } + break; + case LOOKUP_RECORDS: + { + GList *records = do_lookup_records (data->lookup_records.rrname, + data->lookup_records.record_type, + cancellable, + &local_error); + + if (records != NULL) + g_task_return_pointer (task, g_steal_pointer (&records), (GDestroyNotify) free_records); + else + g_task_return_error (task, g_steal_pointer (&local_error)); + } + break; + default: + g_assert_not_reached (); + } +} static void g_threaded_resolver_class_init (GThreadedResolverClass *threaded_class) -- cgit v1.2.1 From 7b18e6205a03058541391e02c013c893b144e1ed Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 25 Apr 2023 15:58:46 +0100 Subject: gthreadedresolver: Switch to using a separate thread pool Rather than running lookups in the global shared thread pool belonging to `GTask`, run them in a private thread pool. This is needed because the global shared thread pool is constrained to only 14 threads. If there are 14 ongoing calls to `g_task_run_in_thread()` from any library/code in the process, and then one of them asks to do a DNS lookup, the lookup will block forever. Under certain circumstances, particularly where there are a couple of deep chains of dependent tasks running with `g_task_run_in_thread()`, this can livelock the program. Since `GResolver` is likely to be called as a frequent leaf call in certain workloads, and in particular there are likely to be several lookups requested at the same time, it makes sense to move resolver lookups to a private thread pool. Signed-off-by: Philip Withnall --- gio/gthreadedresolver.c | 198 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 162 insertions(+), 36 deletions(-) diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c index 4e5b20751..1277907e7 100644 --- a/gio/gthreadedresolver.c +++ b/gio/gthreadedresolver.c @@ -41,18 +41,39 @@ struct _GThreadedResolver { GResolver parent_instance; + + GThreadPool *thread_pool; /* (owned) */ }; G_DEFINE_TYPE (GThreadedResolver, g_threaded_resolver, G_TYPE_RESOLVER) -static void threaded_resolver_worker_cb (GTask *task, - gpointer source_object, - gpointer task_data, - GCancellable *cancellable); +static void run_task_in_thread_pool_async (GThreadedResolver *self, + GTask *task); +static void run_task_in_thread_pool_sync (GThreadedResolver *self, + GTask *task); +static void threaded_resolver_worker_cb (gpointer task_data, + gpointer user_data); static void -g_threaded_resolver_init (GThreadedResolver *gtr) +g_threaded_resolver_init (GThreadedResolver *self) { + self->thread_pool = g_thread_pool_new_full (threaded_resolver_worker_cb, + self, + (GDestroyNotify) g_object_unref, + 20, + FALSE, + NULL); +} + +static void +g_threaded_resolver_finalize (GObject *object) +{ + GThreadedResolver *self = G_THREADED_RESOLVER (object); + + g_thread_pool_free (self->thread_pool, TRUE, FALSE); + self->thread_pool = NULL; + + G_OBJECT_CLASS (g_threaded_resolver_parent_class)->finalize (object); } static GResolverError @@ -95,6 +116,23 @@ typedef struct { GResolverRecordType record_type; } lookup_records; }; + + GCond cond; /* used for signalling completion of the task when running it sync */ + GMutex lock; + + /* This enum indicates that a particular code path has claimed the + * task and is shortly about to call g_task_return_*() on it. + * This must be accessed with GThreadedResolver.lock held. */ + enum + { + NOT_YET, + COMPLETED, /* libc lookup call has completed successfully or errored */ + } will_return; + + /* Whether the thread pool thread executing this lookup has finished executing + * it and g_task_return_*() has been called on it already. + * This must be accessed with GThreadedResolver.lock held. */ + gboolean has_returned; } LookupData; static LookupData * @@ -103,6 +141,8 @@ lookup_data_new_by_name (const char *hostname, { LookupData *data = g_new0 (LookupData, 1); data->lookup_type = LOOKUP_BY_NAME; + g_cond_init (&data->cond); + g_mutex_init (&data->lock); data->lookup_by_name.hostname = g_strdup (hostname); data->lookup_by_name.address_family = address_family; return g_steal_pointer (&data); @@ -113,6 +153,8 @@ lookup_data_new_by_address (GInetAddress *address) { LookupData *data = g_new0 (LookupData, 1); data->lookup_type = LOOKUP_BY_ADDRESS; + g_cond_init (&data->cond); + g_mutex_init (&data->lock); data->lookup_by_address.address = g_object_ref (address); return g_steal_pointer (&data); } @@ -123,6 +165,8 @@ lookup_data_new_records (const gchar *rrname, { LookupData *data = g_new0 (LookupData, 1); data->lookup_type = LOOKUP_RECORDS; + g_cond_init (&data->cond); + g_mutex_init (&data->lock); data->lookup_records.rrname = g_strdup (rrname); data->lookup_records.record_type = record_type; return g_steal_pointer (&data); @@ -145,6 +189,9 @@ lookup_data_free (LookupData *data) g_assert_not_reached (); } + g_mutex_clear (&data->lock); + g_cond_clear (&data->cond); + g_free (data); } @@ -243,6 +290,7 @@ lookup_by_name (GResolver *resolver, GCancellable *cancellable, GError **error) { + GThreadedResolver *self = G_THREADED_RESOLVER (resolver); GTask *task; GList *addresses; LookupData *data; @@ -252,8 +300,9 @@ lookup_by_name (GResolver *resolver, g_task_set_source_tag (task, lookup_by_name); g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); - g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); + + run_task_in_thread_pool_sync (self, task); + addresses = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -285,6 +334,7 @@ lookup_by_name_with_flags (GResolver *resolver, GCancellable *cancellable, GError **error) { + GThreadedResolver *self = G_THREADED_RESOLVER (resolver); GTask *task; GList *addresses; LookupData *data; @@ -294,8 +344,9 @@ lookup_by_name_with_flags (GResolver *resolver, g_task_set_source_tag (task, lookup_by_name_with_flags); g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); - g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); + + run_task_in_thread_pool_sync (self, task); + addresses = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -310,6 +361,7 @@ lookup_by_name_with_flags_async (GResolver *resolver, GAsyncReadyCallback callback, gpointer user_data) { + GThreadedResolver *self = G_THREADED_RESOLVER (resolver); GTask *task; LookupData *data; @@ -322,8 +374,9 @@ lookup_by_name_with_flags_async (GResolver *resolver, g_task_set_source_tag (task, lookup_by_name_with_flags_async); g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); - g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread (task, threaded_resolver_worker_cb); + + run_task_in_thread_pool_async (self, task); + g_object_unref (task); } @@ -415,6 +468,7 @@ lookup_by_address (GResolver *resolver, GCancellable *cancellable, GError **error) { + GThreadedResolver *self = G_THREADED_RESOLVER (resolver); LookupData *data = NULL; GTask *task; gchar *name; @@ -424,8 +478,9 @@ lookup_by_address (GResolver *resolver, g_task_set_source_tag (task, lookup_by_address); g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); - g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); + + run_task_in_thread_pool_sync (self, task); + name = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -439,6 +494,7 @@ lookup_by_address_async (GResolver *resolver, GAsyncReadyCallback callback, gpointer user_data) { + GThreadedResolver *self = G_THREADED_RESOLVER (resolver); LookupData *data = NULL; GTask *task; @@ -447,8 +503,9 @@ lookup_by_address_async (GResolver *resolver, g_task_set_source_tag (task, lookup_by_address_async); g_task_set_name (task, "[gio] resolver lookup"); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); - g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread (task, threaded_resolver_worker_cb); + + run_task_in_thread_pool_async (self, task); + g_object_unref (task); } @@ -1247,6 +1304,7 @@ lookup_records (GResolver *resolver, GCancellable *cancellable, GError **error) { + GThreadedResolver *self = G_THREADED_RESOLVER (resolver); GTask *task; GList *records; LookupData *data = NULL; @@ -1258,8 +1316,8 @@ lookup_records (GResolver *resolver, data = lookup_data_new_records (rrname, record_type); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); - g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread_sync (task, threaded_resolver_worker_cb); + run_task_in_thread_pool_sync (self, task); + records = g_task_propagate_pointer (task, error); g_object_unref (task); @@ -1274,6 +1332,7 @@ lookup_records_async (GResolver *resolver, GAsyncReadyCallback callback, gpointer user_data) { + GThreadedResolver *self = G_THREADED_RESOLVER (resolver); GTask *task; LookupData *data = NULL; @@ -1284,8 +1343,8 @@ lookup_records_async (GResolver *resolver, data = lookup_data_new_records (rrname, record_type); g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) lookup_data_free); - g_task_set_return_on_cancel (task, TRUE); - g_task_run_in_thread (task, threaded_resolver_worker_cb); + run_task_in_thread_pool_async (self, task); + g_object_unref (task); } @@ -1300,13 +1359,41 @@ lookup_records_finish (GResolver *resolver, } static void -threaded_resolver_worker_cb (GTask *task, - gpointer source_object, - gpointer task_data, - GCancellable *cancellable) +run_task_in_thread_pool_async (GThreadedResolver *self, + GTask *task) +{ + LookupData *data = g_task_get_task_data (task); + + g_mutex_lock (&data->lock); + + g_thread_pool_push (self->thread_pool, g_object_ref (task), NULL); + + g_mutex_unlock (&data->lock); +} + +static void +run_task_in_thread_pool_sync (GThreadedResolver *self, + GTask *task) +{ + LookupData *data = g_task_get_task_data (task); + + run_task_in_thread_pool_async (self, task); + + g_mutex_lock (&data->lock); + while (!data->has_returned) + g_cond_wait (&data->cond, &data->lock); + g_mutex_unlock (&data->lock); +} + +static void +threaded_resolver_worker_cb (gpointer task_data, + gpointer user_data) { - LookupData *data = task_data; + GTask *task = G_TASK (g_steal_pointer (&task_data)); + LookupData *data = g_task_get_task_data (task); + GCancellable *cancellable = g_task_get_cancellable (task); GError *local_error = NULL; + gboolean should_return; switch (data->lookup_type) { case LOOKUP_BY_NAME: @@ -1316,10 +1403,19 @@ threaded_resolver_worker_cb (GTask *task, cancellable, &local_error); - if (addresses != NULL) - g_task_return_pointer (task, g_steal_pointer (&addresses), (GDestroyNotify) g_resolver_free_addresses); - else - g_task_return_error (task, g_steal_pointer (&local_error)); + g_mutex_lock (&data->lock); + should_return = g_atomic_int_compare_and_exchange (&data->will_return, NOT_YET, COMPLETED); + g_mutex_unlock (&data->lock); + + if (should_return) + { + if (addresses != NULL) + g_task_return_pointer (task, g_steal_pointer (&addresses), (GDestroyNotify) g_resolver_free_addresses); + else + g_task_return_error (task, g_steal_pointer (&local_error)); + } + + g_clear_pointer (&addresses, g_resolver_free_addresses); } break; case LOOKUP_BY_ADDRESS: @@ -1328,10 +1424,19 @@ threaded_resolver_worker_cb (GTask *task, cancellable, &local_error); - if (name != NULL) - g_task_return_pointer (task, g_steal_pointer (&name), g_free); - else - g_task_return_error (task, g_steal_pointer (&local_error)); + g_mutex_lock (&data->lock); + should_return = g_atomic_int_compare_and_exchange (&data->will_return, NOT_YET, COMPLETED); + g_mutex_unlock (&data->lock); + + if (should_return) + { + if (name != NULL) + g_task_return_pointer (task, g_steal_pointer (&name), g_free); + else + g_task_return_error (task, g_steal_pointer (&local_error)); + } + + g_clear_pointer (&name, g_free); } break; case LOOKUP_RECORDS: @@ -1341,22 +1446,43 @@ threaded_resolver_worker_cb (GTask *task, cancellable, &local_error); - if (records != NULL) - g_task_return_pointer (task, g_steal_pointer (&records), (GDestroyNotify) free_records); - else - g_task_return_error (task, g_steal_pointer (&local_error)); + g_mutex_lock (&data->lock); + should_return = g_atomic_int_compare_and_exchange (&data->will_return, NOT_YET, COMPLETED); + g_mutex_unlock (&data->lock); + + if (should_return) + { + if (records != NULL) + g_task_return_pointer (task, g_steal_pointer (&records), (GDestroyNotify) free_records); + else + g_task_return_error (task, g_steal_pointer (&local_error)); + } + + g_clear_pointer (&records, free_records); } break; default: g_assert_not_reached (); } + + /* Signal completion of a task. */ + g_mutex_lock (&data->lock); + g_assert (!data->has_returned); + data->has_returned = TRUE; + g_cond_broadcast (&data->cond); + g_mutex_unlock (&data->lock); + + g_object_unref (task); } static void g_threaded_resolver_class_init (GThreadedResolverClass *threaded_class) { + GObjectClass *object_class = G_OBJECT_CLASS (threaded_class); GResolverClass *resolver_class = G_RESOLVER_CLASS (threaded_class); + object_class->finalize = g_threaded_resolver_finalize; + resolver_class->lookup_by_name = lookup_by_name; resolver_class->lookup_by_name_async = lookup_by_name_async; resolver_class->lookup_by_name_finish = lookup_by_name_finish; -- cgit v1.2.1 From c3209f1d84a3d755bf1610a5d1c95f55ac914a4e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 25 Apr 2023 17:26:28 +0100 Subject: gthreadedresolver: Re-add cancellation and add timeout support Signed-off-by: Philip Withnall --- gio/gthreadedresolver.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c index 1277907e7..baa5a4d50 100644 --- a/gio/gthreadedresolver.c +++ b/gio/gthreadedresolver.c @@ -28,6 +28,7 @@ #include #include +#include "glib/glib-private.h" #include "gthreadedresolver.h" #include "gnetworkingprivate.h" @@ -120,6 +121,9 @@ typedef struct { GCond cond; /* used for signalling completion of the task when running it sync */ GMutex lock; + GSource *timeout_source; /* (nullable) (owned) */ + GSource *cancellable_source; /* (nullable) (owned) */ + /* This enum indicates that a particular code path has claimed the * task and is shortly about to call g_task_return_*() on it. * This must be accessed with GThreadedResolver.lock held. */ @@ -127,6 +131,8 @@ typedef struct { { NOT_YET, COMPLETED, /* libc lookup call has completed successfully or errored */ + TIMED_OUT, + CANCELLED, } will_return; /* Whether the thread pool thread executing this lookup has finished executing @@ -189,6 +195,18 @@ lookup_data_free (LookupData *data) g_assert_not_reached (); } + if (data->timeout_source != NULL) + { + g_source_destroy (data->timeout_source); + g_clear_pointer (&data->timeout_source, g_source_unref); + } + + if (data->cancellable_source != NULL) + { + g_source_destroy (data->cancellable_source); + g_clear_pointer (&data->cancellable_source, g_source_unref); + } + g_mutex_clear (&data->lock); g_cond_clear (&data->cond); @@ -1358,16 +1376,95 @@ lookup_records_finish (GResolver *resolver, return g_task_propagate_pointer (G_TASK (result), error); } +/* Will be called in the GLib worker thread, so must lock all accesses to shared + * data. */ +static gboolean +timeout_cb (gpointer user_data) +{ + GTask *task = G_TASK (user_data); + LookupData *data = g_task_get_task_data (task); + gboolean should_return; + + g_mutex_lock (&data->lock); + + should_return = g_atomic_int_compare_and_exchange (&data->will_return, NOT_YET, TIMED_OUT); + g_clear_pointer (&data->timeout_source, g_source_unref); + + g_mutex_unlock (&data->lock); + + if (should_return) + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT, + _("Socket I/O timed out")); + + /* Signal completion of the task. */ + g_mutex_lock (&data->lock); + g_assert (!data->has_returned); + data->has_returned = TRUE; + g_cond_broadcast (&data->cond); + g_mutex_unlock (&data->lock); + + return G_SOURCE_REMOVE; +} + +/* Will be called in the GLib worker thread, so must lock all accesses to shared + * data. */ +static gboolean +cancelled_cb (GCancellable *cancellable, + gpointer user_data) +{ + GTask *task = G_TASK (user_data); + LookupData *data = g_task_get_task_data (task); + gboolean should_return; + + g_mutex_lock (&data->lock); + + g_assert (g_cancellable_is_cancelled (cancellable)); + should_return = g_atomic_int_compare_and_exchange (&data->will_return, NOT_YET, CANCELLED); + g_clear_pointer (&data->cancellable_source, g_source_unref); + + g_mutex_unlock (&data->lock); + + if (should_return) + g_task_return_error_if_cancelled (task); + + /* Signal completion of the task. */ + g_mutex_lock (&data->lock); + g_assert (!data->has_returned); + data->has_returned = TRUE; + g_cond_broadcast (&data->cond); + g_mutex_unlock (&data->lock); + + return G_SOURCE_REMOVE; +} + static void run_task_in_thread_pool_async (GThreadedResolver *self, GTask *task) { LookupData *data = g_task_get_task_data (task); + guint timeout_ms = g_resolver_get_timeout (G_RESOLVER (self)); + GCancellable *cancellable = g_task_get_cancellable (task); g_mutex_lock (&data->lock); g_thread_pool_push (self->thread_pool, g_object_ref (task), NULL); + if (timeout_ms != 0) + { + data->timeout_source = g_timeout_source_new (timeout_ms); + g_source_set_static_name (data->timeout_source, "[gio] threaded resolver timeout"); + g_source_set_callback (data->timeout_source, G_SOURCE_FUNC (timeout_cb), task, NULL); + g_source_attach (data->timeout_source, GLIB_PRIVATE_CALL (g_get_worker_context) ()); + } + + if (cancellable != NULL) + { + data->cancellable_source = g_cancellable_source_new (cancellable); + g_source_set_static_name (data->cancellable_source, "[gio] threaded resolver cancellable"); + g_source_set_callback (data->cancellable_source, G_SOURCE_FUNC (cancelled_cb), task, NULL); + g_source_attach (data->cancellable_source, GLIB_PRIVATE_CALL (g_get_worker_context) ()); + } + g_mutex_unlock (&data->lock); } -- cgit v1.2.1 From ef08e8dd81e865a50afe25dd0af028eaabcd7246 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 25 Apr 2023 17:27:22 +0100 Subject: gthreadedresolver: Document design of GThreadedResolver Signed-off-by: Philip Withnall --- gio/gthreadedresolver.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/gio/gthreadedresolver.c b/gio/gthreadedresolver.c index baa5a4d50..b452d1e1b 100644 --- a/gio/gthreadedresolver.c +++ b/gio/gthreadedresolver.c @@ -39,6 +39,47 @@ #include "gsocketaddress.h" #include "gsrvtarget.h" +/* + * GThreadedResolver is a threaded wrapper around the system libc’s + * `getaddrinfo()`. + * + * It has to be threaded, as `getaddrinfo()` is synchronous. libc does provide + * `getaddrinfo_a()` as an asynchronous version of `getaddrinfo()`, but it does + * not integrate with a poll loop. It requires use of sigevent to notify of + * completion of an asynchronous operation. That either emits a signal, or calls + * a callback function in a newly spawned thread. + * + * A signal (`SIGEV_SIGNAL`) can’t be used for completion as (aside from being + * another expensive round trip into the kernel) GLib cannot pick a `SIG*` + * number which is guaranteed to not be in use elsewhere in the process. Various + * other things could be interfering with signal dispositions, such as gdb or + * other libraries in the process. Using a `signalfd()` + * [cannot improve this situation](https://ldpreload.com/blog/signalfd-is-useless). + * + * A callback function in a newly spawned thread (`SIGEV_THREAD`) could be used, + * but that is very expensive. Internally, glibc currently also just implements + * `getaddrinfo_a()` + * [using its own thread pool](https://github.com/bminor/glibc/blob/master/resolv/gai_misc.c), + * and then + * [spawns an additional thread for each completion callback](https://github.com/bminor/glibc/blob/master/resolv/gai_notify.c). + * That is very expensive. + * + * No other appropriate sigevent callback types + * [currently exist](https://sourceware.org/bugzilla/show_bug.cgi?id=30287), and + * [others agree that sigevent is not great](http://davmac.org/davpage/linux/async-io.html#posixaio). + * + * Hence, #GThreadedResolver calls the normal synchronous `getaddrinfo()` in its + * own thread pool. Previously, #GThreadedResolver used the thread pool which is + * internal to #GTask by calling g_task_run_in_thread(). That lead to exhaustion + * of the #GTask thread pool in some situations, though, as DNS lookups are + * quite frequent leaf operations in some use cases. Now, #GThreadedResolver + * uses its own private thread pool. + * + * This is similar to what + * [libasyncns](http://git.0pointer.net/libasyncns.git/tree/libasyncns/asyncns.h) + * and other multi-threaded users of `getaddrinfo()` do. + */ + struct _GThreadedResolver { GResolver parent_instance; -- cgit v1.2.1 From 2ac66413a4669228d101d0a1f77664d7fe5e7d9a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 25 Apr 2023 18:38:21 +0100 Subject: tests: Support --timeout argument in resolver manual test For testing timeouts. Signed-off-by: Philip Withnall --- gio/tests/resolver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gio/tests/resolver.c b/gio/tests/resolver.c index ec9b9e9de..68b3d3cfb 100644 --- a/gio/tests/resolver.c +++ b/gio/tests/resolver.c @@ -41,6 +41,7 @@ static int nlookups = 0; static gboolean synchronous = FALSE; static guint connectable_count = 0; static GResolverRecordType record_type = 0; +static gint timeout_ms = 0; static G_NORETURN void usage (void) @@ -722,6 +723,7 @@ static const GOptionEntry option_entries[] = { { "synchronous", 's', 0, G_OPTION_ARG_NONE, &synchronous, "Synchronous connections", NULL }, { "connectable", 'c', 0, G_OPTION_ARG_INT, &connectable_count, "Connectable count", "C" }, { "special-type", 't', 0, G_OPTION_ARG_CALLBACK, record_type_arg, "Record type like MX, TXT, NS or SOA", "RR" }, + { "timeout", 0, 0, G_OPTION_ARG_INT, &timeout_ms, "Timeout (ms)", "ms" }, G_OPTION_ENTRY_NULL, }; @@ -749,6 +751,9 @@ main (int argc, char **argv) resolver = g_resolver_get_default (); + if (timeout_ms != 0) + g_resolver_set_timeout (resolver, timeout_ms); + cancellable = g_cancellable_new (); #ifdef G_OS_UNIX -- cgit v1.2.1 From 1c4384aec58cae002817cc55651fcf32b556143e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 25 Apr 2023 18:38:40 +0100 Subject: tests: Fix cancellation source handling in resolver manual test If `async_cancel()` was invoked, it would remove the IO watch source, which would cause the `g_source_remove()` call at the end of `main()` to warn about an unknown source ID. Fix that by handling the source as a pointer instead of a handle. Signed-off-by: Philip Withnall --- gio/tests/resolver.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/gio/tests/resolver.c b/gio/tests/resolver.c index 68b3d3cfb..b8adb4a36 100644 --- a/gio/tests/resolver.c +++ b/gio/tests/resolver.c @@ -689,7 +689,7 @@ static gboolean async_cancel (GIOChannel *source, GIOCondition cond, gpointer cancel) { g_cancellable_cancel (cancel); - return FALSE; + return G_SOURCE_REMOVE; } #endif @@ -734,7 +734,7 @@ main (int argc, char **argv) GError *error = NULL; #ifdef G_OS_UNIX GIOChannel *chan; - guint watch; + GSource *watch_source = NULL; #endif context = g_option_context_new ("lookups ..."); @@ -768,7 +768,9 @@ main (int argc, char **argv) exit (1); } chan = g_io_channel_unix_new (cancel_fds[0]); - watch = g_io_add_watch (chan, G_IO_IN, async_cancel, cancellable); + watch_source = g_io_create_watch (chan, G_IO_IN); + g_source_set_callback (watch_source, (GSourceFunc) async_cancel, cancellable, NULL); + g_source_attach (watch_source, NULL); g_io_channel_unref (chan); #endif @@ -792,7 +794,8 @@ main (int argc, char **argv) g_main_loop_unref (loop); #ifdef G_OS_UNIX - g_source_remove (watch); + g_source_destroy (watch_source); + g_clear_pointer (&watch_source, g_source_unref); #endif g_object_unref (cancellable); g_option_context_free (context); -- cgit v1.2.1 From e73c5ed5e1754205122a7f17838525052fc83e84 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 26 Apr 2023 12:53:37 +0100 Subject: gresolver: Set timeout on default resolver to 30s The default for the class is still to have no timeout, but it seems more practical for most use cases to set a non-infinite timeout on the default resolver. If applications have a more specific use case, they can change the timeout or replace the default resolver. See https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3397#note_1731387 Signed-off-by: Philip Withnall --- gio/gresolver.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/gio/gresolver.c b/gio/gresolver.c index ac6fa4f7a..676f1e271 100644 --- a/gio/gresolver.c +++ b/gio/gresolver.c @@ -55,6 +55,10 @@ * #GNetworkAddress and #GNetworkService provide wrappers around * #GResolver functionality that also implement #GSocketConnectable, * making it easy to connect to a remote host/service. + * + * The default resolver (see g_resolver_get_default()) has a timeout of 30s set + * on it since GLib 2.78. Earlier versions of GLib did not support resolver + * timeouts. */ typedef enum { @@ -299,7 +303,9 @@ g_resolver_get_default (void) G_LOCK (default_resolver); if (!default_resolver) - default_resolver = g_object_new (G_TYPE_THREADED_RESOLVER, NULL); + default_resolver = g_object_new (G_TYPE_THREADED_RESOLVER, + "timeout", 30000, + NULL); ret = g_object_ref (default_resolver); G_UNLOCK (default_resolver); -- cgit v1.2.1