From 349e79f91557f852ca60ed7ebcc11fb33f8f3226 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 13:10:22 +0000 Subject: [PATCH 01/24] gs-odrs-provider: Remove unreachable return statement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `json_generator_to_data()` can’t fail. Signed-off-by: Philip Withnall --- lib/gs-odrs-provider.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index e3bbd53ce..00f3f9737 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -776,8 +776,7 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, json_generator_set_pretty (json_generator, TRUE); json_generator_set_root (json_generator, json_root); data = json_generator_to_data (json_generator, NULL); - if (data == NULL) - return NULL; + uri = g_strdup_printf ("%s/fetch", self->review_server); g_debug ("Updating ODRS cache for %s from %s to %s; request %s", gs_app_get_id (app), uri, cachefn, data); -- GitLab From 7b545a5de02687d2cc895ab2591f26de98084a0a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 13:17:35 +0000 Subject: [PATCH 02/24] gs-odrs-provider: Inline a helper function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s only called in one place, and the following commit will make this code asynchronous. If this were to stay as a helper function, it would need its own `GTask`, which would complicate things for no benefit. This introduces no functional changes. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 69 ++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index 00f3f9737..147fd62d4 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -819,43 +819,6 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, return g_steal_pointer (&reviews); } -static gboolean -gs_odrs_provider_refine_reviews (GsOdrsProvider *self, - GsApp *app, - GCancellable *cancellable, - GError **error) -{ - AsReview *review; - g_autoptr(GPtrArray) reviews = NULL; - - /* get from server */ - reviews = gs_odrs_provider_fetch_for_app (self, app, cancellable, error); - if (reviews == NULL) - return FALSE; - for (guint i = 0; i < reviews->len; i++) { - review = g_ptr_array_index (reviews, i); - - /* save this on the application object so we can use it for - * submitting a new review */ - if (i == 0) { - gs_app_set_metadata (app, "ODRS::user_skey", - as_review_get_metadata_item (review, "user_skey")); - } - - /* ignore invalid reviews */ - if (as_review_get_rating (review) == 0) - continue; - - /* the user_hash matches, so mark this as our own review */ - if (g_strcmp0 (as_review_get_reviewer_id (review), - self->user_hash) == 0) { - as_review_set_flags (review, AS_REVIEW_FLAG_SELF); - } - gs_app_add_review (app, review); - } - return TRUE; -} - static gboolean refine_app (GsOdrsProvider *self, GsApp *app, @@ -871,11 +834,37 @@ refine_app (GsOdrsProvider *self, /* add reviews if possible */ if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) { - if (gs_app_get_reviews(app)->len > 0) + AsReview *review; + g_autoptr(GPtrArray) reviews = NULL; + + if (gs_app_get_reviews (app)->len > 0) return TRUE; - if (!gs_odrs_provider_refine_reviews (self, app, - cancellable, error)) + + /* get from server */ + reviews = gs_odrs_provider_fetch_for_app (self, app, cancellable, error); + if (reviews == NULL) return FALSE; + for (guint i = 0; i < reviews->len; i++) { + review = g_ptr_array_index (reviews, i); + + /* save this on the application object so we can use it for + * submitting a new review */ + if (i == 0) { + gs_app_set_metadata (app, "ODRS::user_skey", + as_review_get_metadata_item (review, "user_skey")); + } + + /* ignore invalid reviews */ + if (as_review_get_rating (review) == 0) + continue; + + /* the user_hash matches, so mark this as our own review */ + if (g_strcmp0 (as_review_get_reviewer_id (review), + self->user_hash) == 0) { + as_review_set_flags (review, AS_REVIEW_FLAG_SELF); + } + gs_app_add_review (app, review); + } } /* add ratings if possible */ -- GitLab From 0f477e2d4f89ada81e431691a3fe09e088cac973 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 14:01:09 +0000 Subject: [PATCH 03/24] gs-odrs-provider: Add new type for refine flags Rather than reusing the not-really-appropriate `GsPluginRefineFlags`, which is not appropriate because it carries a lot of unrelated flags, and has a duplicate flag for ratings. This further decouples the `GsOdrsProvider` API from `GsPlugin`. Signed-off-by: Philip Withnall --- lib/gs-odrs-provider.c | 33 +++++++++++++++------------------ lib/gs-odrs-provider.h | 17 +++++++++++++++-- lib/gs-plugin-job-refine.c | 11 +++++++++-- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index 147fd62d4..11fb86db1 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -820,11 +820,11 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, } static gboolean -refine_app (GsOdrsProvider *self, - GsApp *app, - GsPluginRefineFlags flags, - GCancellable *cancellable, - GError **error) +refine_app (GsOdrsProvider *self, + GsApp *app, + GsOdrsProviderRefineFlags flags, + GCancellable *cancellable, + GError **error) { /* not valid */ if (gs_app_get_kind (app) == AS_COMPONENT_KIND_ADDON) @@ -833,7 +833,7 @@ refine_app (GsOdrsProvider *self, return TRUE; /* add reviews if possible */ - if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) { + if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS) { AsReview *review; g_autoptr(GPtrArray) reviews = NULL; @@ -868,8 +868,7 @@ refine_app (GsOdrsProvider *self, } /* add ratings if possible */ - if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS || - flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_RATING) { + if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS) { if (gs_app_get_review_ratings (app) != NULL) return TRUE; if (!gs_odrs_provider_refine_ratings (self, app, cancellable, error)) @@ -1386,19 +1385,17 @@ gs_odrs_provider_refresh_ratings_finish (GsOdrsProvider *self, * specified in @flags. * * Returns: %TRUE on success, %FALSE otherwise - * Since: 41 + * Since: 42 */ gboolean -gs_odrs_provider_refine (GsOdrsProvider *self, - GsAppList *list, - GsPluginRefineFlags flags, - GCancellable *cancellable, - GError **error) +gs_odrs_provider_refine (GsOdrsProvider *self, + GsAppList *list, + GsOdrsProviderRefineFlags flags, + GCancellable *cancellable, + GError **error) { - /* nothing to do here */ - if ((flags & (GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS | - GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS | - GS_PLUGIN_REFINE_FLAGS_REQUIRE_RATING)) == 0) + if ((flags & (GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS | + GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS)) == 0) return TRUE; for (guint i = 0; i < gs_app_list_length (list); i++) { diff --git a/lib/gs-odrs-provider.h b/lib/gs-odrs-provider.h index 2f045d2ed..6959cca4a 100644 --- a/lib/gs-odrs-provider.h +++ b/lib/gs-odrs-provider.h @@ -16,7 +16,6 @@ #include "gs-app-list.h" #include "gs-download-utils.h" -#include "gs-plugin-types.h" G_BEGIN_DECLS @@ -41,6 +40,20 @@ typedef enum { #define GS_ODRS_PROVIDER_ERROR gs_odrs_provider_error_quark () GQuark gs_odrs_provider_error_quark (void); +/** + * GsOdrsProviderRefineFlags: + * @GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS: Get the numerical ratings for the app. + * @GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS: Get the written reviews for the app. + * + * The flags for refining apps to get their reviews or ratings. + * + * Since: 42 + */ +typedef enum { + GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS = (1 << 0), + GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS = (1 << 1), +} GsOdrsProviderRefineFlags; + #define GS_TYPE_ODRS_PROVIDER (gs_odrs_provider_get_type ()) G_DECLARE_FINAL_TYPE (GsOdrsProvider, gs_odrs_provider, GS, ODRS_PROVIDER, GObject) @@ -65,7 +78,7 @@ gboolean gs_odrs_provider_refresh_ratings_finish(GsOdrsProvider *self, gboolean gs_odrs_provider_refine (GsOdrsProvider *self, GsAppList *list, - GsPluginRefineFlags flags, + GsOdrsProviderRefineFlags flags, GCancellable *cancellable, GError **error); diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index b7d097adb..4eba1f793 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -182,6 +182,7 @@ run_refine_filter (GsPluginJobRefine *self, GError **error) { GsOdrsProvider *odrs_provider; + GsOdrsProviderRefineFlags odrs_refine_flags = 0; GPtrArray *plugins; /* (element-type GsPlugin) */ /* run each plugin */ @@ -215,9 +216,15 @@ run_refine_filter (GsPluginJobRefine *self, /* Add ODRS data if needed */ odrs_provider = gs_plugin_loader_get_odrs_provider (plugin_loader); - if (odrs_provider != NULL) { + if (refine_flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) + odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS; + if (refine_flags & (GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS | + GS_PLUGIN_REFINE_FLAGS_REQUIRE_RATING)) + odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS; + + if (odrs_provider != NULL && odrs_refine_flags != 0) { if (!gs_odrs_provider_refine (odrs_provider, - list, refine_flags, cancellable, error)) + list, odrs_refine_flags, cancellable, error)) return FALSE; } -- GitLab From 9b619be5bcc5992791dbb2bede8837b78ae15528 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 14:21:28 +0000 Subject: [PATCH 04/24] gs-odrs-provider: Move the refine_app() helper function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This moves it closer to where it’s used. The code isn’t changed at all. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 124 +++++++++++++++++++++-------------------- 1 file changed, 65 insertions(+), 59 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index 11fb86db1..a90aeed87 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -819,65 +819,6 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, return g_steal_pointer (&reviews); } -static gboolean -refine_app (GsOdrsProvider *self, - GsApp *app, - GsOdrsProviderRefineFlags flags, - GCancellable *cancellable, - GError **error) -{ - /* not valid */ - if (gs_app_get_kind (app) == AS_COMPONENT_KIND_ADDON) - return TRUE; - if (gs_app_get_id (app) == NULL) - return TRUE; - - /* add reviews if possible */ - if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS) { - AsReview *review; - g_autoptr(GPtrArray) reviews = NULL; - - if (gs_app_get_reviews (app)->len > 0) - return TRUE; - - /* get from server */ - reviews = gs_odrs_provider_fetch_for_app (self, app, cancellable, error); - if (reviews == NULL) - return FALSE; - for (guint i = 0; i < reviews->len; i++) { - review = g_ptr_array_index (reviews, i); - - /* save this on the application object so we can use it for - * submitting a new review */ - if (i == 0) { - gs_app_set_metadata (app, "ODRS::user_skey", - as_review_get_metadata_item (review, "user_skey")); - } - - /* ignore invalid reviews */ - if (as_review_get_rating (review) == 0) - continue; - - /* the user_hash matches, so mark this as our own review */ - if (g_strcmp0 (as_review_get_reviewer_id (review), - self->user_hash) == 0) { - as_review_set_flags (review, AS_REVIEW_FLAG_SELF); - } - gs_app_add_review (app, review); - } - } - - /* add ratings if possible */ - if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS) { - if (gs_app_get_review_ratings (app) != NULL) - return TRUE; - if (!gs_odrs_provider_refine_ratings (self, app, cancellable, error)) - return FALSE; - } - - return TRUE; -} - static gchar * gs_odrs_provider_trim_version (const gchar *version) { @@ -1373,6 +1314,12 @@ gs_odrs_provider_refresh_ratings_finish (GsOdrsProvider *self, return g_task_propagate_boolean (G_TASK (result), error); } +static gboolean refine_app (GsOdrsProvider *self, + GsApp *app, + GsOdrsProviderRefineFlags flags, + GCancellable *cancellable, + GError **error); + /** * gs_odrs_provider_refine: * @self: a #GsOdrsProvider @@ -1416,6 +1363,65 @@ gs_odrs_provider_refine (GsOdrsProvider *self, return TRUE; } +static gboolean +refine_app (GsOdrsProvider *self, + GsApp *app, + GsOdrsProviderRefineFlags flags, + GCancellable *cancellable, + GError **error) +{ + /* not valid */ + if (gs_app_get_kind (app) == AS_COMPONENT_KIND_ADDON) + return TRUE; + if (gs_app_get_id (app) == NULL) + return TRUE; + + /* add reviews if possible */ + if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS) { + AsReview *review; + g_autoptr(GPtrArray) reviews = NULL; + + if (gs_app_get_reviews (app)->len > 0) + return TRUE; + + /* get from server */ + reviews = gs_odrs_provider_fetch_for_app (self, app, cancellable, error); + if (reviews == NULL) + return FALSE; + for (guint i = 0; i < reviews->len; i++) { + review = g_ptr_array_index (reviews, i); + + /* save this on the application object so we can use it for + * submitting a new review */ + if (i == 0) { + gs_app_set_metadata (app, "ODRS::user_skey", + as_review_get_metadata_item (review, "user_skey")); + } + + /* ignore invalid reviews */ + if (as_review_get_rating (review) == 0) + continue; + + /* the user_hash matches, so mark this as our own review */ + if (g_strcmp0 (as_review_get_reviewer_id (review), + self->user_hash) == 0) { + as_review_set_flags (review, AS_REVIEW_FLAG_SELF); + } + gs_app_add_review (app, review); + } + } + + /* add ratings if possible */ + if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS) { + if (gs_app_get_review_ratings (app) != NULL) + return TRUE; + if (!gs_odrs_provider_refine_ratings (self, app, cancellable, error)) + return FALSE; + } + + return TRUE; +} + /** * gs_odrs_provider_submit_review: * @self: a #GsOdrsProvider -- GitLab From 3ad1cf5106f21a85bbeda6c96fc627de9ccdb4aa Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 14:36:09 +0000 Subject: [PATCH 05/24] gs-odrs-provider: Fix incorrect early returns when refining apps If the app already had review data, but no ratings data, the code to fetch the reviews would incorrectly return before fetching the ratings. Signed-off-by: Philip Withnall --- lib/gs-odrs-provider.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index a90aeed87..18ab6886b 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -1377,13 +1377,11 @@ refine_app (GsOdrsProvider *self, return TRUE; /* add reviews if possible */ - if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS) { + if ((flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS) && + gs_app_get_reviews (app)->len == 0) { AsReview *review; g_autoptr(GPtrArray) reviews = NULL; - if (gs_app_get_reviews (app)->len > 0) - return TRUE; - /* get from server */ reviews = gs_odrs_provider_fetch_for_app (self, app, cancellable, error); if (reviews == NULL) @@ -1412,9 +1410,8 @@ refine_app (GsOdrsProvider *self, } /* add ratings if possible */ - if (flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS) { - if (gs_app_get_review_ratings (app) != NULL) - return TRUE; + if ((flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS) && + gs_app_get_review_ratings (app) == NULL) { if (!gs_odrs_provider_refine_ratings (self, app, cancellable, error)) return FALSE; } -- GitLab From 0ae0904b63dd4a799e1e76be1733a9a2409da97c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 14:38:23 +0000 Subject: [PATCH 06/24] gs-odrs-provider: Move an early-return check This will simplify the code in an upcoming refactor commit to make `refine_app()` asynchronous. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index 18ab6886b..cb9e73da0 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -1348,6 +1348,13 @@ gs_odrs_provider_refine (GsOdrsProvider *self, for (guint i = 0; i < gs_app_list_length (list); i++) { GsApp *app = gs_app_list_index (list, i); g_autoptr(GError) local_error = NULL; + + /* not valid */ + if (gs_app_get_kind (app) == AS_COMPONENT_KIND_ADDON) + continue; + if (gs_app_get_id (app) == NULL) + continue; + if (!refine_app (self, app, flags, cancellable, &local_error)) { if (g_error_matches (local_error, GS_ODRS_PROVIDER_ERROR, GS_ODRS_PROVIDER_ERROR_NO_NETWORK)) { g_debug ("failed to refine app %s: %s", @@ -1370,12 +1377,6 @@ refine_app (GsOdrsProvider *self, GCancellable *cancellable, GError **error) { - /* not valid */ - if (gs_app_get_kind (app) == AS_COMPONENT_KIND_ADDON) - return TRUE; - if (gs_app_get_id (app) == NULL) - return TRUE; - /* add reviews if possible */ if ((flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS) && gs_app_get_reviews (app)->len == 0) { -- GitLab From 8a7b767bee5d9d081889e62432a77df447edd907 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 16:09:29 +0000 Subject: [PATCH 07/24] gs-odrs-provider: Make refine asynchronous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is needed to make `GsPluginJobRefine` fully asynchronous. This commit makes progress towards that, but doesn’t completely solve the issue. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 353 +++++++++++++++++++++++++++---------- lib/gs-odrs-provider.h | 6 +- lib/gs-plugin-job-refine.c | 13 +- 3 files changed, 275 insertions(+), 97 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index cb9e73da0..0c98b5089 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -693,11 +693,16 @@ gs_odrs_provider_get_compat_ids (GsApp *app) return g_steal_pointer (&json_node); } -static GPtrArray * -gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, - GsApp *app, - GCancellable *cancellable, - GError **error) +static void set_reviews_on_app (GsOdrsProvider *self, + GsApp *app, + GPtrArray *reviews); + +static void +gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, + GsApp *app, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { JsonNode *json_compat_ids; const gchar *version; @@ -717,6 +722,11 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, #if SOUP_CHECK_VERSION(3, 0, 0) g_autoptr(GBytes) bytes = NULL; #endif + g_autoptr(GTask) task = NULL; + g_autoptr(GError) local_error = NULL; + + task = g_task_new (self, cancellable, callback, user_data); + g_task_set_source_tag (task, gs_odrs_provider_fetch_reviews_for_app_async); /* look in the cache */ cachefn_basename = g_strdup_printf ("%s.json", gs_app_get_id (app)); @@ -724,23 +734,36 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, cachefn_basename, GS_UTILS_CACHE_FLAG_WRITEABLE | GS_UTILS_CACHE_FLAG_CREATE_DIRECTORY, - error); - if (cachefn == NULL) - return NULL; + &local_error); + if (cachefn == NULL) { + g_task_return_error (task, g_steal_pointer (&local_error)); + return; + } + cachefn_file = g_file_new_for_path (cachefn); if (gs_utils_get_file_age (cachefn_file) < self->max_cache_age_secs) { g_autoptr(GMappedFile) mapped_file = NULL; - mapped_file = g_mapped_file_new (cachefn, FALSE, error); - if (mapped_file == NULL) - return NULL; + mapped_file = g_mapped_file_new (cachefn, FALSE, &local_error); + if (mapped_file == NULL) { + g_task_return_error (task, g_steal_pointer (&local_error)); + return; + } g_debug ("got review data for %s from %s", gs_app_get_id (app), cachefn); - return gs_odrs_provider_parse_reviews (self, - g_mapped_file_get_contents (mapped_file), - g_mapped_file_get_length (mapped_file), - error); + reviews = gs_odrs_provider_parse_reviews (self, + g_mapped_file_get_contents (mapped_file), + g_mapped_file_get_length (mapped_file), + &local_error); + if (reviews == NULL) { + g_task_return_error (task, g_steal_pointer (&local_error)); + } else { + set_reviews_on_app (self, app, reviews); + g_task_return_pointer (task, g_steal_pointer (&reviews), (GDestroyNotify) g_ptr_array_unref); + } + + return; } /* not always available */ @@ -784,9 +807,11 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, #if SOUP_CHECK_VERSION(3, 0, 0) g_odrs_provider_set_message_request_body (msg, "application/json; charset=utf-8", data, strlen (data)); - bytes = soup_session_send_and_read (self->session, msg, cancellable, error); - if (bytes == NULL) - return NULL; + bytes = soup_session_send_and_read (self->session, msg, cancellable, &local_error); + if (bytes == NULL) { + g_task_return_error (task, g_steal_pointer (&local_error)); + return; + } downloaded_data = g_bytes_get_data (bytes, &downloaded_data_length); status_code = soup_message_get_status (msg); @@ -798,25 +823,71 @@ gs_odrs_provider_fetch_for_app (GsOdrsProvider *self, downloaded_data_length = msg->response_body ? msg->response_body->length : 0; #endif if (status_code != SOUP_STATUS_OK) { - if (!gs_odrs_provider_parse_success (downloaded_data, downloaded_data_length, error)) - return NULL; + if (!gs_odrs_provider_parse_success (downloaded_data, downloaded_data_length, &local_error)) { + g_task_return_error (task, g_steal_pointer (&local_error)); + return; + } + /* not sure what to do here */ - g_set_error_literal (error, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_DOWNLOADING, - "status code invalid"); - return NULL; + g_task_return_new_error (task, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_DOWNLOADING, + "status code invalid"); + return; + } + + reviews = gs_odrs_provider_parse_reviews (self, downloaded_data, downloaded_data_length, &local_error); + if (reviews == NULL) { + g_task_return_error (task, g_steal_pointer (&local_error)); + return; } - reviews = gs_odrs_provider_parse_reviews (self, downloaded_data, downloaded_data_length, error); - if (reviews == NULL) - return NULL; /* save to the cache */ - if (!g_file_set_contents (cachefn, downloaded_data, downloaded_data_length, error)) - return NULL; + if (!g_file_set_contents (cachefn, downloaded_data, downloaded_data_length, &local_error)) { + g_task_return_error (task, g_steal_pointer (&local_error)); + return; + } + + set_reviews_on_app (self, app, reviews); /* success */ - return g_steal_pointer (&reviews); + g_task_return_pointer (task, g_steal_pointer (&reviews), (GDestroyNotify) g_ptr_array_unref); +} + +static void +set_reviews_on_app (GsOdrsProvider *self, + GsApp *app, + GPtrArray *reviews) +{ + for (guint i = 0; i < reviews->len; i++) { + AsReview *review = g_ptr_array_index (reviews, i); + + /* save this on the application object so we can use it for + * submitting a new review */ + if (i == 0) { + gs_app_set_metadata (app, "ODRS::user_skey", + as_review_get_metadata_item (review, "user_skey")); + } + + /* ignore invalid reviews */ + if (as_review_get_rating (review) == 0) + continue; + + /* the user_hash matches, so mark this as our own review */ + if (g_strcmp0 (as_review_get_reviewer_id (review), + self->user_hash) == 0) { + as_review_set_flags (review, AS_REVIEW_FLAG_SELF); + } + gs_app_add_review (app, review); + } +} + +static GPtrArray * +gs_odrs_provider_fetch_reviews_for_app_finish (GsOdrsProvider *self, + GAsyncResult *result, + GError **error) +{ + return g_task_propagate_pointer (G_TASK (result), error); } static gchar * @@ -1314,40 +1385,87 @@ gs_odrs_provider_refresh_ratings_finish (GsOdrsProvider *self, return g_task_propagate_boolean (G_TASK (result), error); } -static gboolean refine_app (GsOdrsProvider *self, - GsApp *app, - GsOdrsProviderRefineFlags flags, - GCancellable *cancellable, - GError **error); +static void refine_app_op (GsOdrsProvider *self, + GTask *task, + GsApp *app, + GsOdrsProviderRefineFlags flags, + GCancellable *cancellable); +static void refine_reviews_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data); +static void finish_refine_op (GTask *task, + GError *error); + +typedef struct { + /* Input data. */ + GsAppList *list; /* (owned) (not nullable) */ + GsOdrsProviderRefineFlags flags; + + /* In-progress data. */ + guint n_pending_ops; + GError *error; /* (nullable) (owned) */ +} RefineData; + +static void +refine_data_free (RefineData *data) +{ + g_assert (data->n_pending_ops == 0); + + g_clear_object (&data->list); + g_clear_error (&data->error); + + g_free (data); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC (RefineData, refine_data_free) /** - * gs_odrs_provider_refine: + * gs_odrs_provider_refine_async: * @self: a #GsOdrsProvider * @list: list of apps to refine * @flags: refine flags * @cancellable: (nullable): a #GCancellable, or %NULL - * @error: return location for a #GError + * @callback: callback for asynchronous completion + * @user_data: data to pass to @callback * - * Refine the given @list of apps to add ratings and review data to them, as - * specified in @flags. + * Asynchronously refine the given @list of apps to add ratings and review data + * to them, as specified in @flags. * - * Returns: %TRUE on success, %FALSE otherwise * Since: 42 */ -gboolean -gs_odrs_provider_refine (GsOdrsProvider *self, - GsAppList *list, - GsOdrsProviderRefineFlags flags, - GCancellable *cancellable, - GError **error) +void +gs_odrs_provider_refine_async (GsOdrsProvider *self, + GsAppList *list, + GsOdrsProviderRefineFlags flags, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { + g_autoptr(GTask) task = NULL; + g_autoptr(RefineData) data = NULL; + RefineData *data_unowned = NULL; + + task = g_task_new (self, cancellable, callback, user_data); + g_task_set_source_tag (task, gs_odrs_provider_refine_async); + + data_unowned = data = g_new0 (RefineData, 1); + data->list = g_object_ref (list); + data->flags = flags; + g_task_set_task_data (task, g_steal_pointer (&data), (GDestroyNotify) refine_data_free); + if ((flags & (GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS | - GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS)) == 0) - return TRUE; + GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS)) == 0) { + g_task_return_boolean (task, TRUE); + return; + } + + /* Mark one operation as pending while all the operations are started, + * so the overall operation can’t complete while things are still being + * started. */ + data_unowned->n_pending_ops++; for (guint i = 0; i < gs_app_list_length (list); i++) { GsApp *app = gs_app_list_index (list, i); - g_autoptr(GError) local_error = NULL; /* not valid */ if (gs_app_get_kind (app) == AS_COMPONENT_KIND_ADDON) @@ -1355,69 +1473,116 @@ gs_odrs_provider_refine (GsOdrsProvider *self, if (gs_app_get_id (app) == NULL) continue; - if (!refine_app (self, app, flags, cancellable, &local_error)) { + data_unowned->n_pending_ops++; + refine_app_op (self, task, app, flags, cancellable); + } + + finish_refine_op (task, NULL); +} + +static void +refine_app_op (GsOdrsProvider *self, + GTask *task, + GsApp *app, + GsOdrsProviderRefineFlags flags, + GCancellable *cancellable) +{ + g_autoptr(GError) local_error = NULL; + + /* add ratings if possible */ + if ((flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS) && + gs_app_get_review_ratings (app) == NULL) { + if (!gs_odrs_provider_refine_ratings (self, app, cancellable, &local_error)) { if (g_error_matches (local_error, GS_ODRS_PROVIDER_ERROR, GS_ODRS_PROVIDER_ERROR_NO_NETWORK)) { g_debug ("failed to refine app %s: %s", gs_app_get_unique_id (app), local_error->message); } else { g_prefix_error (&local_error, "failed to refine app: "); - g_propagate_error (error, g_steal_pointer (&local_error)); - return FALSE; + finish_refine_op (task, g_steal_pointer (&local_error)); + return; } } } - return TRUE; -} - -static gboolean -refine_app (GsOdrsProvider *self, - GsApp *app, - GsOdrsProviderRefineFlags flags, - GCancellable *cancellable, - GError **error) -{ /* add reviews if possible */ if ((flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS) && gs_app_get_reviews (app)->len == 0) { - AsReview *review; - g_autoptr(GPtrArray) reviews = NULL; - - /* get from server */ - reviews = gs_odrs_provider_fetch_for_app (self, app, cancellable, error); - if (reviews == NULL) - return FALSE; - for (guint i = 0; i < reviews->len; i++) { - review = g_ptr_array_index (reviews, i); - - /* save this on the application object so we can use it for - * submitting a new review */ - if (i == 0) { - gs_app_set_metadata (app, "ODRS::user_skey", - as_review_get_metadata_item (review, "user_skey")); - } + /* get from server asynchronously */ + gs_odrs_provider_fetch_reviews_for_app_async (self, app, cancellable, refine_reviews_cb, g_object_ref (task)); + } else { + finish_refine_op (task, NULL); + } +} - /* ignore invalid reviews */ - if (as_review_get_rating (review) == 0) - continue; +static void +refine_reviews_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + GsOdrsProvider *self = GS_ODRS_PROVIDER (source_object); + g_autoptr(GTask) task = g_steal_pointer (&user_data); + g_autoptr(GError) local_error = NULL; - /* the user_hash matches, so mark this as our own review */ - if (g_strcmp0 (as_review_get_reviewer_id (review), - self->user_hash) == 0) { - as_review_set_flags (review, AS_REVIEW_FLAG_SELF); - } - gs_app_add_review (app, review); + if (!gs_odrs_provider_fetch_reviews_for_app_finish (self, result, &local_error)) { + if (g_error_matches (local_error, GS_ODRS_PROVIDER_ERROR, GS_ODRS_PROVIDER_ERROR_NO_NETWORK)) { + g_debug ("failed to refine app: %s", local_error->message); + } else { + g_prefix_error (&local_error, "failed to refine app: "); + finish_refine_op (task, g_steal_pointer (&local_error)); + return; } } - /* add ratings if possible */ - if ((flags & GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS) && - gs_app_get_review_ratings (app) == NULL) { - if (!gs_odrs_provider_refine_ratings (self, app, cancellable, error)) - return FALSE; + finish_refine_op (task, NULL); +} + +/* @error is (transfer full) if non-NULL. */ +static void +finish_refine_op (GTask *task, + GError *error) +{ + RefineData *data = g_task_get_task_data (task); + g_autoptr(GError) error_owned = g_steal_pointer (&error); + + if (data->error == NULL && error_owned != NULL) + data->error = g_steal_pointer (&error_owned); + else if (error_owned != NULL) + g_debug ("Additional error while refining ODRS data: %s", error_owned->message); + + g_assert (data->n_pending_ops > 0); + data->n_pending_ops--; + + if (data->n_pending_ops == 0) { + if (data->error != NULL) + g_task_return_error (task, g_steal_pointer (&data->error)); + else + g_task_return_boolean (task, TRUE); } +} - return TRUE; +/** + * gs_odrs_provider_refine_finish: + * @self: a #GsOdrsProvider + * @result: result of the asynchronous operation + * @error: return location for a #GError, or %NULL + * + * Finish an asynchronous refine operation started with + * gs_odrs_provider_refine_finish(). + * + * Returns: %TRUE on success, %FALSE otherwise + * Since: 42 + */ +gboolean +gs_odrs_provider_refine_finish (GsOdrsProvider *self, + GAsyncResult *result, + GError **error) +{ + g_return_val_if_fail (GS_IS_ODRS_PROVIDER (self), FALSE); + g_return_val_if_fail (g_task_is_valid (result, self), FALSE); + g_return_val_if_fail (g_async_result_is_tagged (result, gs_odrs_provider_refine_async), FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + + return g_task_propagate_boolean (G_TASK (result), error); } /** diff --git a/lib/gs-odrs-provider.h b/lib/gs-odrs-provider.h index 6959cca4a..f02ffde64 100644 --- a/lib/gs-odrs-provider.h +++ b/lib/gs-odrs-provider.h @@ -76,10 +76,14 @@ gboolean gs_odrs_provider_refresh_ratings_finish(GsOdrsProvider *self, GAsyncResult *result, GError **error); -gboolean gs_odrs_provider_refine (GsOdrsProvider *self, +void gs_odrs_provider_refine_async (GsOdrsProvider *self, GsAppList *list, GsOdrsProviderRefineFlags flags, GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data); +gboolean gs_odrs_provider_refine_finish (GsOdrsProvider *self, + GAsyncResult *result, GError **error); gboolean gs_odrs_provider_submit_review (GsOdrsProvider *self, diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index 4eba1f793..f47abf8df 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -223,8 +223,17 @@ run_refine_filter (GsPluginJobRefine *self, odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS; if (odrs_provider != NULL && odrs_refine_flags != 0) { - if (!gs_odrs_provider_refine (odrs_provider, - list, odrs_refine_flags, cancellable, error)) + g_autoptr(GAsyncResult) odrs_refine_result = NULL; + + gs_odrs_provider_refine_async (odrs_provider, list, odrs_refine_flags, + cancellable, plugin_refine_cb, &odrs_refine_result); + + /* FIXME: Make this sync until the calling function is rearranged + * to be async. */ + while (odrs_refine_result == NULL) + g_main_context_iteration (g_main_context_get_thread_default (), TRUE); + + if (!gs_odrs_provider_refine_finish (odrs_provider, odrs_refine_result, error)) return FALSE; } -- GitLab From 1d5a1e2d4d5d4f9c60da99e255ede2e5cf20b622 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 16:19:15 +0000 Subject: [PATCH 08/24] gs-plugin-job-refine: Inline a helper function `run_refine()` needs to be made asynchronous to fix #1658, and having this helper function separate will only complicate that. This commit introduces no functional changes. Signed-off-by: Philip Withnall Hels: #1658 --- lib/gs-plugin-job-refine.c | 66 +++++++++++++++----------------------- 1 file changed, 25 insertions(+), 41 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index f47abf8df..4e4f3e97e 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -173,18 +173,33 @@ static void plugin_refine_cb (GObject *source_object, GAsyncResult *result, gpointer user_data); +static void +plugin_refine_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + GAsyncResult **result_out = user_data; + + g_assert (*result_out == NULL); + *result_out = g_object_ref (result); + g_main_context_wakeup (g_main_context_get_thread_default ()); +} + static gboolean -run_refine_filter (GsPluginJobRefine *self, - GsPluginLoader *plugin_loader, - GsAppList *list, - GsPluginRefineFlags refine_flags, - GCancellable *cancellable, - GError **error) +run_refine_internal (GsPluginJobRefine *self, + GsPluginLoader *plugin_loader, + GsAppList *list, + GsPluginRefineFlags flags, + GCancellable *cancellable, + GError **error) { GsOdrsProvider *odrs_provider; GsOdrsProviderRefineFlags odrs_refine_flags = 0; GPtrArray *plugins; /* (element-type GsPlugin) */ + /* try to adopt each application with a plugin */ + gs_plugin_loader_run_adopt (plugin_loader, list); + /* run each plugin */ plugins = gs_plugin_loader_get_plugins (plugin_loader); @@ -199,7 +214,7 @@ run_refine_filter (GsPluginJobRefine *self, continue; /* run the batched plugin symbol */ - plugin_class->refine_async (plugin, list, refine_flags, + plugin_class->refine_async (plugin, list, flags, cancellable, plugin_refine_cb, &refine_result); /* FIXME: Make this sync until the calling function is rearranged @@ -216,10 +231,10 @@ run_refine_filter (GsPluginJobRefine *self, /* Add ODRS data if needed */ odrs_provider = gs_plugin_loader_get_odrs_provider (plugin_loader); - if (refine_flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) + if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS; - if (refine_flags & (GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS | - GS_PLUGIN_REFINE_FLAGS_REQUIRE_RATING)) + if (flags & (GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS | + GS_PLUGIN_REFINE_FLAGS_REQUIRE_RATING)) odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS; if (odrs_provider != NULL && odrs_refine_flags != 0) { @@ -239,37 +254,6 @@ run_refine_filter (GsPluginJobRefine *self, /* filter any wildcard apps left in the list */ gs_app_list_filter (list, app_is_non_wildcard, NULL); - return TRUE; -} - -static void -plugin_refine_cb (GObject *source_object, - GAsyncResult *result, - gpointer user_data) -{ - GAsyncResult **result_out = user_data; - - g_assert (*result_out == NULL); - *result_out = g_object_ref (result); - g_main_context_wakeup (g_main_context_get_thread_default ()); -} - -static gboolean -run_refine_internal (GsPluginJobRefine *self, - GsPluginLoader *plugin_loader, - GsAppList *list, - GsPluginRefineFlags flags, - GCancellable *cancellable, - GError **error) -{ - /* try to adopt each application with a plugin */ - gs_plugin_loader_run_adopt (plugin_loader, list); - - /* run each plugin */ - if (!run_refine_filter (self, plugin_loader, list, flags, - cancellable, error)) { - return FALSE; - } /* ensure these are sorted by score */ if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) { -- GitLab From bead4d462f8ae023ba517e57d0d15ea68c6b5e21 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 16:28:27 +0000 Subject: [PATCH 09/24] gs-plugin-job-refine: Set task source tag Rather than setting the task name, set its source tag. GLib provides a macro implementation of `g_task_set_source_tag()` which sets the task name at the same time. Signed-off-by: Philip Withnall --- lib/gs-plugin-job-refine.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index 4e4f3e97e..2d16db9e3 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -434,7 +434,7 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, /* check required args */ task = g_task_new (job, cancellable, callback, user_data); - g_task_set_name (task, G_STRFUNC); + g_task_set_source_tag (task, gs_plugin_job_refine_run_async); /* Operate on a copy of the input list so we don’t modify it when * resolving wildcards. */ -- GitLab From 9dd1ccd4c6714efc5599153afdb9a2ccc864a6c1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 16:31:50 +0000 Subject: [PATCH 10/24] gs-plugin-job-refine: Move pre-refine checks around MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A following commit will make `run_refine()` asynchronous. It’ll be easier to handle the branch for `self->flags != 0` if it’s inside `run_refine()` rather than providing an alternate code path which isn’t asynchronous. This introduces no functional changes. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-plugin-job-refine.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index 2d16db9e3..f99ffee31 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -366,6 +366,10 @@ run_refine (GsPluginJobRefine *self, g_autoptr(GsAppList) freeze_list = NULL; /* nothing to do */ + if (self->flags == 0) { + g_debug ("no refine flags set for transaction"); + return TRUE; + } if (gs_app_list_length (list) == 0) return TRUE; @@ -441,14 +445,10 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, result_list = gs_app_list_copy (self->app_list); /* run refine() on each one if required */ - if (self->flags != 0) { - if (!run_refine (self, plugin_loader, result_list, cancellable, &local_error)) { - gs_utils_error_convert_gio (&local_error); - g_task_return_error (task, g_steal_pointer (&local_error)); - return; - } - } else { - g_debug ("no refine flags set for transaction"); + if (!run_refine (self, plugin_loader, result_list, cancellable, &local_error)) { + gs_utils_error_convert_gio (&local_error); + g_task_return_error (task, g_steal_pointer (&local_error)); + return; } /* Internal calls to #GsPluginJobRefine may want to do their own -- GitLab From 1f8e677a7af1924978baeeead800a8d645991d32 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 16:33:22 +0000 Subject: [PATCH 11/24] gs-plugin-job-refine: Rearrange private struct members This makes their separate lifetimes a little clearer. This commit introduces no functional changes. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-plugin-job-refine.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index f99ffee31..36aee5c08 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -55,9 +55,12 @@ struct _GsPluginJobRefine { GsPluginJob parent; + /* Input data. */ GsAppList *app_list; /* (owned) */ - GsAppList *result_list; /* (owned) (nullable) */ GsPluginRefineFlags flags; + + /* Output data. */ + GsAppList *result_list; /* (owned) (nullable) */ }; G_DEFINE_TYPE (GsPluginJobRefine, gs_plugin_job_refine, GS_TYPE_PLUGIN_JOB) -- GitLab From 8946d8d71d3583ff9e0e711627b0c7abf69296ce Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 23 Feb 2022 17:00:24 +0000 Subject: [PATCH 12/24] gs-plugin-job-refine: Inline run_refine() This will make refactoring the refine code to be fully asynchronous a bit easier. This introduces no functional changes. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-plugin-job-refine.c | 117 +++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 65 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index 36aee5c08..0ea3b6ab9 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -358,102 +358,89 @@ app_thaw_notify_idle (gpointer data) return G_SOURCE_REMOVE; } -static gboolean -run_refine (GsPluginJobRefine *self, - GsPluginLoader *plugin_loader, - GsAppList *list, - GCancellable *cancellable, - GError **error) +static void +gs_plugin_job_refine_run_async (GsPluginJob *job, + GsPluginLoader *plugin_loader, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { - gboolean ret; + GsPluginJobRefine *self = GS_PLUGIN_JOB_REFINE (job); + g_autoptr(GTask) task = NULL; + g_autoptr(GError) local_error = NULL; + g_autofree gchar *job_debug = NULL; + g_autoptr(GsAppList) result_list = NULL; g_autoptr(GsAppList) freeze_list = NULL; + /* check required args */ + task = g_task_new (job, cancellable, callback, user_data); + g_task_set_source_tag (task, gs_plugin_job_refine_run_async); + + /* Operate on a copy of the input list so we don’t modify it when + * resolving wildcards. */ + result_list = gs_app_list_copy (self->app_list); + /* nothing to do */ if (self->flags == 0) { g_debug ("no refine flags set for transaction"); - return TRUE; + goto results; } - if (gs_app_list_length (list) == 0) - return TRUE; + if (gs_app_list_length (result_list) == 0) + goto results; /* freeze all apps */ - freeze_list = gs_app_list_copy (list); + freeze_list = gs_app_list_copy (result_list); for (guint i = 0; i < gs_app_list_length (freeze_list); i++) { GsApp *app = gs_app_list_index (freeze_list, i); g_object_freeze_notify (G_OBJECT (app)); } /* first pass */ - ret = run_refine_internal (self, plugin_loader, list, self->flags, cancellable, error); - if (!ret) - goto out; - - /* remove any addons that have the same source as the parent app */ - for (guint i = 0; i < gs_app_list_length (list); i++) { - g_autoptr(GPtrArray) to_remove = g_ptr_array_new (); - GsApp *app = gs_app_list_index (list, i); - GsAppList *addons = gs_app_get_addons (app); - - /* find any apps with the same source */ - const gchar *pkgname_parent = gs_app_get_source_default (app); - if (pkgname_parent == NULL) - continue; - for (guint j = 0; j < gs_app_list_length (addons); j++) { - GsApp *addon = gs_app_list_index (addons, j); - if (g_strcmp0 (gs_app_get_source_default (addon), - pkgname_parent) == 0) { - g_debug ("%s has the same pkgname of %s as %s", - gs_app_get_unique_id (app), - pkgname_parent, - gs_app_get_unique_id (addon)); - g_ptr_array_add (to_remove, addon); + if (run_refine_internal (self, plugin_loader, result_list, self->flags, cancellable, &local_error)) { + /* remove any addons that have the same source as the parent app */ + for (guint i = 0; i < gs_app_list_length (result_list); i++) { + g_autoptr(GPtrArray) to_remove = g_ptr_array_new (); + GsApp *app = gs_app_list_index (result_list, i); + GsAppList *addons = gs_app_get_addons (app); + + /* find any apps with the same source */ + const gchar *pkgname_parent = gs_app_get_source_default (app); + if (pkgname_parent == NULL) + continue; + for (guint j = 0; j < gs_app_list_length (addons); j++) { + GsApp *addon = gs_app_list_index (addons, j); + if (g_strcmp0 (gs_app_get_source_default (addon), + pkgname_parent) == 0) { + g_debug ("%s has the same pkgname of %s as %s", + gs_app_get_unique_id (app), + pkgname_parent, + gs_app_get_unique_id (addon)); + g_ptr_array_add (to_remove, addon); + } } - } - /* remove any addons with the same source */ - for (guint j = 0; j < to_remove->len; j++) { - GsApp *addon = g_ptr_array_index (to_remove, j); - gs_app_remove_addon (app, addon); + /* remove any addons with the same source */ + for (guint j = 0; j < to_remove->len; j++) { + GsApp *addon = g_ptr_array_index (to_remove, j); + gs_app_remove_addon (app, addon); + } } } -out: /* now emit all the changed signals */ for (guint i = 0; i < gs_app_list_length (freeze_list); i++) { GsApp *app = gs_app_list_index (freeze_list, i); g_idle_add (app_thaw_notify_idle, g_object_ref (app)); } - return ret; -} - -static void -gs_plugin_job_refine_run_async (GsPluginJob *job, - GsPluginLoader *plugin_loader, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data) -{ - GsPluginJobRefine *self = GS_PLUGIN_JOB_REFINE (job); - g_autoptr(GTask) task = NULL; - g_autoptr(GError) local_error = NULL; - g_autofree gchar *job_debug = NULL; - g_autoptr(GsAppList) result_list = NULL; - - /* check required args */ - task = g_task_new (job, cancellable, callback, user_data); - g_task_set_source_tag (task, gs_plugin_job_refine_run_async); - - /* Operate on a copy of the input list so we don’t modify it when - * resolving wildcards. */ - result_list = gs_app_list_copy (self->app_list); - /* run refine() on each one if required */ - if (!run_refine (self, plugin_loader, result_list, cancellable, &local_error)) { + /* Delayed error handling. */ + if (local_error != NULL) { gs_utils_error_convert_gio (&local_error); g_task_return_error (task, g_steal_pointer (&local_error)); return; } +results: /* Internal calls to #GsPluginJobRefine may want to do their own * filtering, typically if the refine is being done as part of another * plugin job. If so, only filter to remove wildcards. Wildcards should -- GitLab From 55872b0e718efc0e48eecc902a2762ece13f28fd Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 24 Feb 2022 15:38:39 +0000 Subject: [PATCH 13/24] gs-plugin-job-refine: Remove unnecessary app list copy The `self->app_list` is never modified after construction of the `GsPluginJobRefine`, so is safe to use as the freeze list and the thaw list. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-plugin-job-refine.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index 0ea3b6ab9..f5cc9604c 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -370,7 +370,6 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, g_autoptr(GError) local_error = NULL; g_autofree gchar *job_debug = NULL; g_autoptr(GsAppList) result_list = NULL; - g_autoptr(GsAppList) freeze_list = NULL; /* check required args */ task = g_task_new (job, cancellable, callback, user_data); @@ -389,9 +388,8 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, goto results; /* freeze all apps */ - freeze_list = gs_app_list_copy (result_list); - for (guint i = 0; i < gs_app_list_length (freeze_list); i++) { - GsApp *app = gs_app_list_index (freeze_list, i); + for (guint i = 0; i < gs_app_list_length (self->app_list); i++) { + GsApp *app = gs_app_list_index (self->app_list, i); g_object_freeze_notify (G_OBJECT (app)); } @@ -428,8 +426,8 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, } /* now emit all the changed signals */ - for (guint i = 0; i < gs_app_list_length (freeze_list); i++) { - GsApp *app = gs_app_list_index (freeze_list, i); + for (guint i = 0; i < gs_app_list_length (self->app_list); i++) { + GsApp *app = gs_app_list_index (self->app_list, i); g_idle_add (app_thaw_notify_idle, g_object_ref (app)); } -- GitLab From 6d16116101789d27ccefd249b3cd34655f674488 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 12:57:53 +0000 Subject: [PATCH 14/24] gs-plugin-job-refine: Tidy up some internal refine code Rename variables to have more descriptive names, add some whitespace to separate logical blocks of code, and tidy up variable declarations a bit. This introduces no functional changes. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-plugin-job-refine.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index f5cc9604c..1bdc211d6 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -270,14 +270,13 @@ run_refine_internal (GsPluginJobRefine *self, /* refine addons one layer deep */ if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_ADDONS) { - g_autoptr(GsAppList) addons_list = NULL; + g_autoptr(GsAppList) addons_list = gs_app_list_new (); GsPluginRefineFlags addons_flags = flags; addons_flags &= ~(GS_PLUGIN_REFINE_FLAGS_REQUIRE_ADDONS | GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS | GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS); - addons_list = gs_app_list_new (); for (guint i = 0; i < gs_app_list_length (list); i++) { GsApp *app = gs_app_list_index (list, i); GsAppList *addons = gs_app_get_addons (app); @@ -300,18 +299,20 @@ run_refine_internal (GsPluginJobRefine *self, /* also do runtime */ if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_RUNTIME) { - g_autoptr(GsAppList) list2 = gs_app_list_new (); + g_autoptr(GsAppList) runtimes_list = gs_app_list_new (); + GsPluginRefineFlags runtimes_flags = flags; + for (guint i = 0; i < gs_app_list_length (list); i++) { - GsApp *runtime; GsApp *app = gs_app_list_index (list, i); - runtime = gs_app_get_runtime (app); + GsApp *runtime = gs_app_get_runtime (app); + if (runtime != NULL) - gs_app_list_add (list2, runtime); + gs_app_list_add (runtimes_list, runtime); } - if (gs_app_list_length (list2) > 0) { + if (gs_app_list_length (runtimes_list) > 0) { if (!run_refine_internal (self, plugin_loader, - list2, flags, cancellable, - error)) { + runtimes_list, runtimes_flags, + cancellable, error)) { return FALSE; } } @@ -319,12 +320,11 @@ run_refine_internal (GsPluginJobRefine *self, /* also do related packages one layer deep */ if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_RELATED) { - g_autoptr(GsAppList) related_list = NULL; + g_autoptr(GsAppList) related_list = gs_app_list_new (); GsPluginRefineFlags related_flags = flags; related_flags &= ~GS_PLUGIN_REFINE_FLAGS_REQUIRE_RELATED; - related_list = gs_app_list_new (); for (guint i = 0; i < gs_app_list_length (list); i++) { GsApp *app = gs_app_list_index (list, i); GsAppList *related = gs_app_get_related (app); -- GitLab From d9db49c56baf3cfef36e5627a9eccd272bf8716a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 12:59:10 +0000 Subject: [PATCH 15/24] =?UTF-8?q?gs-plugin-job-refine:=20Don=E2=80=99t=20r?= =?UTF-8?q?ecursive=20if=20refine=20flags=20are=20empty?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refine job recurses into related components of the components it’s been asked to refine, also refining them. For example, addons and runtimes of the apps it’s refining. To avoid infinite recursion, these recursive refines are done with various flags disabled. If those are the only flags in the set of `GsPluginRefineFlags`, though, the recursive refine operations will be no-ops. Might as well not do the recursion in those cases. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-plugin-job-refine.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index 1bdc211d6..9e5bf7ca6 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -288,7 +288,8 @@ run_refine_internal (GsPluginJobRefine *self, gs_app_list_add (addons_list, addon); } } - if (gs_app_list_length (addons_list) > 0) { + + if (gs_app_list_length (addons_list) > 0 && addons_flags != 0) { if (!run_refine_internal (self, plugin_loader, addons_list, addons_flags, cancellable, error)) { @@ -302,6 +303,8 @@ run_refine_internal (GsPluginJobRefine *self, g_autoptr(GsAppList) runtimes_list = gs_app_list_new (); GsPluginRefineFlags runtimes_flags = flags; + runtimes_flags &= ~GS_PLUGIN_REFINE_FLAGS_REQUIRE_RUNTIME; + for (guint i = 0; i < gs_app_list_length (list); i++) { GsApp *app = gs_app_list_index (list, i); GsApp *runtime = gs_app_get_runtime (app); @@ -309,7 +312,8 @@ run_refine_internal (GsPluginJobRefine *self, if (runtime != NULL) gs_app_list_add (runtimes_list, runtime); } - if (gs_app_list_length (runtimes_list) > 0) { + + if (gs_app_list_length (runtimes_list) > 0 && runtimes_flags != 0) { if (!run_refine_internal (self, plugin_loader, runtimes_list, runtimes_flags, cancellable, error)) { @@ -336,7 +340,8 @@ run_refine_internal (GsPluginJobRefine *self, gs_app_list_add (related_list, app2); } } - if (gs_app_list_length (related_list) > 0) { + + if (gs_app_list_length (related_list) > 0 && related_flags != 0) { if (!run_refine_internal (self, plugin_loader, related_list, related_flags, cancellable, error)) { -- GitLab From 08f808e06c6727aa6e16a7ccf351efcc4481a2c9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 14:31:21 +0000 Subject: [PATCH 16/24] gs-odrs-provider: Refactor parsing function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor the parsing function so it can take a `JsonParser` object, rather than always constructing one internally. Different callers get access to their data in different ways. This will allow us to use `json_parser_load*()` methods which are more appropriate to each caller. In particular, this will allow JSON to be parsed chunk-wise as it’s downloaded from the network, rather than downloading into a huge allocated buffer and then parsing that buffer. This introduces no functional changes. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 116 +++++++++++++++++++++++++++++------------ 1 file changed, 83 insertions(+), 33 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index 0c98b5089..de8290443 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -254,44 +254,20 @@ gs_odrs_provider_parse_review_object (JsonObject *item) return rev; } +/* json_parser_load*() must have been called on @json_parser before calling + * this function. */ static GPtrArray * gs_odrs_provider_parse_reviews (GsOdrsProvider *self, - const gchar *data, - gssize data_len, + JsonParser *json_parser, GError **error) { JsonArray *json_reviews; JsonNode *json_root; guint i; - g_autoptr(JsonParser) json_parser = NULL; g_autoptr(GHashTable) reviewer_ids = NULL; g_autoptr(GPtrArray) reviews = NULL; g_autoptr(GError) local_error = NULL; - /* nothing */ - if (data == NULL) { - if (!g_network_monitor_get_network_available (g_network_monitor_get_default ())) - g_set_error_literal (error, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_NO_NETWORK, - "server couldn't be reached"); - else - g_set_error_literal (error, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_PARSING_DATA, - "server returned no data"); - return NULL; - } - - /* parse the data and find the array or ratings */ - json_parser = json_parser_new_immutable (); - if (!json_parser_load_from_data (json_parser, data, data_len, &local_error)) { - g_set_error (error, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_PARSING_DATA, - "Error parsing ODRS data: %s", local_error->message); - return NULL; - } json_root = json_parser_get_root (json_parser); if (json_root == NULL) { g_set_error_literal (error, @@ -716,6 +692,7 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, g_autoptr(GFile) cachefn_file = NULL; g_autoptr(GPtrArray) reviews = NULL; g_autoptr(JsonBuilder) builder = NULL; + g_autoptr(JsonParser) json_parser = NULL; g_autoptr(JsonGenerator) json_generator = NULL; g_autoptr(JsonNode) json_root = NULL; g_autoptr(SoupMessage) msg = NULL; @@ -752,10 +729,30 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, g_debug ("got review data for %s from %s", gs_app_get_id (app), cachefn); - reviews = gs_odrs_provider_parse_reviews (self, - g_mapped_file_get_contents (mapped_file), - g_mapped_file_get_length (mapped_file), - &local_error); + + /* nothing */ + if (g_mapped_file_get_contents (mapped_file) == NULL) { + g_task_return_new_error (task, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_PARSING_DATA, + "server returned no data"); + return; + } + + /* parse the data and find the array of ratings */ + json_parser = json_parser_new_immutable (); + if (!json_parser_load_from_data (json_parser, + g_mapped_file_get_contents (mapped_file), + g_mapped_file_get_length (mapped_file), + &local_error)) { + g_task_return_new_error (task, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_PARSING_DATA, + "Error parsing ODRS data: %s", local_error->message); + return; + } + + reviews = gs_odrs_provider_parse_reviews (self, json_parser, &local_error); if (reviews == NULL) { g_task_return_error (task, g_steal_pointer (&local_error)); } else { @@ -836,7 +833,32 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, return; } - reviews = gs_odrs_provider_parse_reviews (self, downloaded_data, downloaded_data_length, &local_error); + /* nothing */ + if (downloaded_data == NULL) { + if (!g_network_monitor_get_network_available (g_network_monitor_get_default ())) + g_task_return_new_error (task, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_NO_NETWORK, + "server couldn't be reached"); + else + g_task_return_new_error (task, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_PARSING_DATA, + "server returned no data"); + return; + } + + /* parse the data and find the array of ratings */ + json_parser = json_parser_new_immutable (); + if (!json_parser_load_from_data (json_parser, downloaded_data, downloaded_data_length, &local_error)) { + g_task_return_new_error (task, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_PARSING_DATA, + "Error parsing ODRS data: %s", local_error->message); + return; + } + + reviews = gs_odrs_provider_parse_reviews (self, json_parser, &local_error); if (reviews == NULL) { g_task_return_error (task, g_steal_pointer (&local_error)); return; @@ -1825,11 +1847,13 @@ gs_odrs_provider_add_unvoted_reviews (GsOdrsProvider *self, gsize downloaded_data_length; g_autofree gchar *uri = NULL; g_autoptr(GHashTable) hash = NULL; + g_autoptr(JsonParser) json_parser = NULL; g_autoptr(GPtrArray) reviews = NULL; g_autoptr(SoupMessage) msg = NULL; #if SOUP_CHECK_VERSION(3, 0, 0) g_autoptr(GBytes) bytes = NULL; #endif + g_autoptr(GError) local_error = NULL; /* create the GET data *with* the machine hash so we can later * review the application ourselves */ @@ -1861,7 +1885,33 @@ gs_odrs_provider_add_unvoted_reviews (GsOdrsProvider *self, return FALSE; } g_debug ("odrs returned: %.*s", (gint) downloaded_data_length, (const gchar *) downloaded_data); - reviews = gs_odrs_provider_parse_reviews (self, downloaded_data, downloaded_data_length, error); + + /* nothing */ + if (downloaded_data == NULL) { + if (!g_network_monitor_get_network_available (g_network_monitor_get_default ())) + g_set_error_literal (error, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_NO_NETWORK, + "server couldn't be reached"); + else + g_set_error_literal (error, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_PARSING_DATA, + "server returned no data"); + return FALSE; + } + + /* parse the data and find the array of ratings */ + json_parser = json_parser_new_immutable (); + if (!json_parser_load_from_data (json_parser, downloaded_data, downloaded_data_length, &local_error)) { + g_set_error (error, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_PARSING_DATA, + "Error parsing ODRS data: %s", local_error->message); + return FALSE; + } + + reviews = gs_odrs_provider_parse_reviews (self, json_parser, error); if (reviews == NULL) return FALSE; -- GitLab From 109a200fee255829bda04286cbc7e7b1697edbc0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 14:44:54 +0000 Subject: [PATCH 17/24] build: Bump json-glib dependency to 1.6 This means we can use the memory mapped file support unconditionally. Debian Stable and Fedora 35 both have json-glib 1.6. Signed-off-by: Philip Withnall --- contrib/gnome-software.spec.in | 2 +- lib/gs-odrs-provider.c | 4 ---- meson.build | 2 +- .../gs-plugin-fedora-pkgdb-collections.c | 17 ----------------- 4 files changed, 2 insertions(+), 23 deletions(-) diff --git a/contrib/gnome-software.spec.in b/contrib/gnome-software.spec.in index 497f7b08f..584d081ea 100644 --- a/contrib/gnome-software.spec.in +++ b/contrib/gnome-software.spec.in @@ -1,6 +1,6 @@ %global glib2_version 2.61.1 %global gtk3_version 3.22.4 -%global json_glib_version 1.2.0 +%global json_glib_version 1.6.0 %global packagekit_version 1.1.1 %global appstream_version 0.14.0 %global libsoup_version 2.52.0 diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index de8290443..e7e6df792 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -126,11 +126,7 @@ gs_odrs_provider_load_ratings (GsOdrsProvider *self, /* parse the data and find the success */ json_parser = json_parser_new_immutable (); -#if JSON_CHECK_VERSION(1, 6, 0) if (!json_parser_load_from_mapped_file (json_parser, filename, &local_error)) { -#else - if (!json_parser_load_from_file (json_parser, filename, &local_error)) { -#endif g_set_error (error, GS_ODRS_PROVIDER_ERROR, GS_ODRS_PROVIDER_ERROR_PARSING_DATA, diff --git a/meson.build b/meson.build index 6b2163d07..37d3efdf2 100644 --- a/meson.build +++ b/meson.build @@ -136,7 +136,7 @@ gtk = dependency('gtk4', ] ) glib = dependency('glib-2.0', version : '>= 2.66.0') -json_glib = dependency('json-glib-1.0', version : '>= 1.2.0') +json_glib = dependency('json-glib-1.0', version : '>= 1.6.0') libm = cc.find_library('m', required: false) if get_option('soup2') libsoup = dependency('libsoup-2.4', version : '>= 2.52.0') diff --git a/plugins/fedora-pkgdb-collections/gs-plugin-fedora-pkgdb-collections.c b/plugins/fedora-pkgdb-collections/gs-plugin-fedora-pkgdb-collections.c index 6d28b2cd5..c380a9803 100644 --- a/plugins/fedora-pkgdb-collections/gs-plugin-fedora-pkgdb-collections.c +++ b/plugins/fedora-pkgdb-collections/gs-plugin-fedora-pkgdb-collections.c @@ -408,28 +408,11 @@ load_json (GsPluginFedoraPkgdbCollections *self, { JsonArray *collections; JsonObject *root; -#if !JSON_CHECK_VERSION(1, 6, 0) - gsize len; - g_autofree gchar *data = NULL; -#endif /* json-glib < 1.6.0 */ g_autoptr(JsonParser) parser = NULL; -#if JSON_CHECK_VERSION(1, 6, 0) parser = json_parser_new_immutable (); if (!json_parser_load_from_mapped_file (parser, self->cachefn, error)) return FALSE; -#else /* if json-glib < 1.6.0 */ - /* get cached file */ - if (!g_file_get_contents (self->cachefn, &data, &len, error)) { - gs_utils_error_convert_gio (error); - return FALSE; - } - - /* parse data */ - parser = json_parser_new (); - if (!json_parser_load_from_data (parser, data, len, error)) - return FALSE; -#endif /* json-glib < 1.6.0 */ root = json_node_get_object (json_parser_get_root (parser)); if (root == NULL) { -- GitLab From 2e7179a1096c16e6043fb39910258e7dba441ce6 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 14:45:46 +0000 Subject: [PATCH 18/24] gs-odrs-provider: Simplify memory mapped JSON loading Simplify loading a memory mapped local JSON file in `fetch_reviews_for_app_async()`. This introduces no functional changes. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index e7e6df792..23f2b529f 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -715,32 +715,12 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, cachefn_file = g_file_new_for_path (cachefn); if (gs_utils_get_file_age (cachefn_file) < self->max_cache_age_secs) { - g_autoptr(GMappedFile) mapped_file = NULL; - - mapped_file = g_mapped_file_new (cachefn, FALSE, &local_error); - if (mapped_file == NULL) { - g_task_return_error (task, g_steal_pointer (&local_error)); - return; - } - g_debug ("got review data for %s from %s", gs_app_get_id (app), cachefn); - /* nothing */ - if (g_mapped_file_get_contents (mapped_file) == NULL) { - g_task_return_new_error (task, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_PARSING_DATA, - "server returned no data"); - return; - } - /* parse the data and find the array of ratings */ json_parser = json_parser_new_immutable (); - if (!json_parser_load_from_data (json_parser, - g_mapped_file_get_contents (mapped_file), - g_mapped_file_get_length (mapped_file), - &local_error)) { + if (!json_parser_load_from_mapped_file (json_parser, cachefn, &local_error)) { g_task_return_new_error (task, GS_ODRS_PROVIDER_ERROR, GS_ODRS_PROVIDER_ERROR_PARSING_DATA, -- GitLab From 86c2b5975bdfbee07595aae06e43de392bb8c69a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 15:23:49 +0000 Subject: [PATCH 19/24] packagekit: Make a critical section smaller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The critical sections where `prepared_updates_mutex` was locked were too long, and were causing noticeable delays when the main thread was waiting on acquiring the lock while it was held by another thread. Rather than updating the `prepared_updates` hash table in place, replace it with a new one each time, then each thread can be potentially operating (with a strong reference) on independent hash tables from successive updates to the cache. Eventually the locking on `prepared_updates` can go away if all the operations in the PackageKit plugin can move into the main thread (if that makes sense). Currently, though, it can be accessed from multiple worker threads from the vfuncs which haven’t been ported to the new threading model yet. Signed-off-by: Philip Withnall Helps: #1658 --- plugins/packagekit/gs-plugin-packagekit.c | 35 +++++++++++++++-------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/plugins/packagekit/gs-plugin-packagekit.c b/plugins/packagekit/gs-plugin-packagekit.c index 71b4d6553..5b05e44c7 100644 --- a/plugins/packagekit/gs-plugin-packagekit.c +++ b/plugins/packagekit/gs-plugin-packagekit.c @@ -1218,13 +1218,13 @@ gs_plugin_systemd_update_cache (GsPluginPackagekit *self, { g_autoptr(GError) error_local = NULL; g_auto(GStrv) package_ids = NULL; - g_autoptr(GMutexLocker) locker = g_mutex_locker_new (&self->prepared_updates_mutex); - - /* invalidate */ - g_hash_table_remove_all (self->prepared_updates); + g_autoptr(GHashTable) new_prepared_updates = NULL; + g_autoptr(GMutexLocker) locker = NULL; /* get new list of package-ids. This loads a local file, so should be * just about fast enough to be sync. */ + new_prepared_updates = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, NULL); package_ids = pk_offline_get_prepared_ids (&error_local); if (package_ids == NULL) { if (g_error_matches (error_local, @@ -1241,13 +1241,18 @@ gs_plugin_systemd_update_cache (GsPluginPackagekit *self, return FALSE; } + /* Build the new table, stealing all the elements from @package_ids. */ for (guint i = 0; package_ids[i] != NULL; i++) { - g_hash_table_add (self->prepared_updates, g_steal_pointer (&package_ids[i])); + g_hash_table_add (new_prepared_updates, g_steal_pointer (&package_ids[i])); } - /* Already stolen all the elements */ g_clear_pointer (&package_ids, g_free); + /* Update the shared state. */ + locker = g_mutex_locker_new (&self->prepared_updates_mutex); + g_clear_pointer (&self->prepared_updates, g_hash_table_unref); + self->prepared_updates = g_steal_pointer (&new_prepared_updates); + return TRUE; } @@ -1936,7 +1941,7 @@ get_details_cb (GObject *source_object, g_autoptr(GPtrArray) array = NULL; g_autoptr(PkResults) results = NULL; g_autoptr(GHashTable) details_collection = NULL; - g_autoptr(GMutexLocker) locker = NULL; + g_autoptr(GHashTable) prepared_updates = NULL; g_autoptr(GError) local_error = NULL; results = pk_client_generic_finish (client, result, &local_error); @@ -1961,10 +1966,13 @@ get_details_cb (GObject *source_object, details_collection = gs_plugin_packagekit_details_array_to_hash (array); /* set the update details for the update */ - locker = g_mutex_locker_new (&self->prepared_updates_mutex); + g_mutex_lock (&self->prepared_updates_mutex); + prepared_updates = g_hash_table_ref (self->prepared_updates); + g_mutex_unlock (&self->prepared_updates_mutex); + for (guint i = 0; i < gs_app_list_length (data->details_list); i++) { GsApp *app = gs_app_list_index (data->details_list, i); - gs_plugin_packagekit_refine_details_app (GS_PLUGIN (self), details_collection, self->prepared_updates, app); + gs_plugin_packagekit_refine_details_app (GS_PLUGIN (self), details_collection, prepared_updates, app); } refine_task_complete_operation (refine_task); @@ -2932,16 +2940,19 @@ gs_plugin_url_to_app (GsPlugin *plugin, if (packages->len >= 1) { g_autoptr(GHashTable) details_collection = NULL; - g_autoptr(GMutexLocker) locker = NULL; + g_autoptr(GHashTable) prepared_updates = NULL; if (gs_app_get_local_file (app) != NULL) return TRUE; details_collection = gs_plugin_packagekit_details_array_to_hash (details); + g_mutex_lock (&self->prepared_updates_mutex); + prepared_updates = g_hash_table_ref (self->prepared_updates); + g_mutex_unlock (&self->prepared_updates_mutex); + gs_plugin_packagekit_resolve_packages_app (GS_PLUGIN (self), packages, app); - locker = g_mutex_locker_new (&self->prepared_updates_mutex); - gs_plugin_packagekit_refine_details_app (plugin, details_collection, self->prepared_updates, app); + gs_plugin_packagekit_refine_details_app (plugin, details_collection, prepared_updates, app); gs_app_list_add (list, app); } else { -- GitLab From 58cf3bd51c991746f79d4f94369b4a3a19f473f0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 15:30:00 +0000 Subject: [PATCH 20/24] gs-odrs-provider: Change parse_success() to take an input stream This is a prerequisite to changing other parts of the download/parsing code to parse an input stream. This will improve performance by avoiding having to allocate a heap buffer for the entire JSON data to be parsed (which could be megabytes in size). It introduces no significant changes in itself. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index 23f2b529f..8cc87375b 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -327,9 +327,8 @@ gs_odrs_provider_parse_reviews (GsOdrsProvider *self, } static gboolean -gs_odrs_provider_parse_success (const gchar *data, - gssize data_len, - GError **error) +gs_odrs_provider_parse_success (GInputStream *input_stream, + GError **error) { JsonNode *json_root; JsonObject *json_item; @@ -337,24 +336,10 @@ gs_odrs_provider_parse_success (const gchar *data, g_autoptr(JsonParser) json_parser = NULL; g_autoptr(GError) local_error = NULL; - /* nothing */ - if (data == NULL) { - if (!g_network_monitor_get_network_available (g_network_monitor_get_default ())) - g_set_error_literal (error, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_NO_NETWORK, - "server couldn't be reached"); - else - g_set_error_literal (error, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_PARSING_DATA, - "server returned no data"); - return FALSE; - } - - /* parse the data and find the success */ + /* parse the data and find the success + * FIXME: This should probably eventually be refactored and made async */ json_parser = json_parser_new_immutable (); - if (!json_parser_load_from_data (json_parser, data, data_len, &local_error)) { + if (!json_parser_load_from_stream (json_parser, input_stream, NULL, &local_error)) { g_set_error (error, GS_ODRS_PROVIDER_ERROR, GS_ODRS_PROVIDER_ERROR_PARSING_DATA, @@ -484,6 +469,7 @@ gs_odrs_provider_json_post (SoupSession *session, g_autoptr(SoupMessage) msg = NULL; gconstpointer downloaded_data; gsize downloaded_data_length; + g_autoptr(GInputStream) input_stream = NULL; #if SOUP_CHECK_VERSION(3, 0, 0) g_autoptr(GBytes) bytes = NULL; #endif @@ -520,7 +506,8 @@ gs_odrs_provider_json_post (SoupSession *session, } /* process returned JSON */ - return gs_odrs_provider_parse_success (downloaded_data, downloaded_data_length, error); + input_stream = g_memory_input_stream_new_from_data (downloaded_data, downloaded_data_length, NULL); + return gs_odrs_provider_parse_success (input_stream, error); } static GPtrArray * @@ -796,7 +783,8 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, downloaded_data_length = msg->response_body ? msg->response_body->length : 0; #endif if (status_code != SOUP_STATUS_OK) { - if (!gs_odrs_provider_parse_success (downloaded_data, downloaded_data_length, &local_error)) { + g_autoptr(GInputStream) input_stream = g_memory_input_stream_new_from_data (downloaded_data, downloaded_data_length, NULL); + if (!gs_odrs_provider_parse_success (input_stream, &local_error)) { g_task_return_error (task, g_steal_pointer (&local_error)); return; } @@ -1851,7 +1839,8 @@ gs_odrs_provider_add_unvoted_reviews (GsOdrsProvider *self, downloaded_data_length = msg->response_body ? msg->response_body->length : 0; #endif if (status_code != SOUP_STATUS_OK) { - if (!gs_odrs_provider_parse_success (downloaded_data, downloaded_data_length, error)) + g_autoptr(GInputStream) input_stream = g_memory_input_stream_new_from_data (downloaded_data, downloaded_data_length, NULL); + if (!gs_odrs_provider_parse_success (input_stream, error)) return FALSE; /* not sure what to do here */ g_set_error_literal (error, -- GitLab From 3d93b683d8a192a1a3ef8cd068dbf2405050543d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 15:31:56 +0000 Subject: [PATCH 21/24] gs-odrs-provider: Make HTTP code in fetch_reviews_for_app_async() async MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It previously wasn’t, which would cause blocking. A nice side effect of this is that now the parsing is stream-based, which avoids having to allocate large heap buffers for the downloaded data before it’s parsed. Signed-off-by: Philip Withnall Helps: #1658 --- lib/gs-odrs-provider.c | 139 ++++++++++++++++++++++++++++++----------- 1 file changed, 102 insertions(+), 37 deletions(-) diff --git a/lib/gs-odrs-provider.c b/lib/gs-odrs-provider.c index 8cc87375b..3c6ca084f 100644 --- a/lib/gs-odrs-provider.c +++ b/lib/gs-odrs-provider.c @@ -652,10 +652,34 @@ gs_odrs_provider_get_compat_ids (GsApp *app) return g_steal_pointer (&json_node); } +static void open_input_stream_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data); +static void parse_reviews_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data); static void set_reviews_on_app (GsOdrsProvider *self, GsApp *app, GPtrArray *reviews); +typedef struct { + GsApp *app; /* (not nullable) (owned) */ + gchar *cache_filename; /* (not nullable) (owned) */ + SoupMessage *message; /* (nullable) (owned) */ +} FetchReviewsForAppData; + +static void +fetch_reviews_for_app_data_free (FetchReviewsForAppData *data) +{ + g_clear_object (&data->app); + g_free (data->cache_filename); + g_clear_object (&data->message); + + g_free (data); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC (FetchReviewsForAppData, fetch_reviews_for_app_data_free) + static void gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, GsApp *app, @@ -665,12 +689,9 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, { JsonNode *json_compat_ids; const gchar *version; - guint status_code; - gconstpointer downloaded_data; - gsize downloaded_data_length; g_autofree gchar *cachefn_basename = NULL; g_autofree gchar *cachefn = NULL; - g_autofree gchar *data = NULL; + g_autofree gchar *request_body = NULL; g_autofree gchar *uri = NULL; g_autoptr(GFile) cachefn_file = NULL; g_autoptr(GPtrArray) reviews = NULL; @@ -683,11 +704,17 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, g_autoptr(GBytes) bytes = NULL; #endif g_autoptr(GTask) task = NULL; + FetchReviewsForAppData *data; + g_autoptr(FetchReviewsForAppData) data_owned = NULL; g_autoptr(GError) local_error = NULL; task = g_task_new (self, cancellable, callback, user_data); g_task_set_source_tag (task, gs_odrs_provider_fetch_reviews_for_app_async); + data = data_owned = g_new0 (FetchReviewsForAppData, 1); + data->app = g_object_ref (app); + g_task_set_task_data (task, g_steal_pointer (&data_owned), (GDestroyNotify) fetch_reviews_for_app_data_free); + /* look in the cache */ cachefn_basename = g_strdup_printf ("%s.json", gs_app_get_id (app)); cachefn = gs_utils_get_cache_filename ("odrs", @@ -700,6 +727,7 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, return; } + data->cache_filename = g_strdup (cachefn); cachefn_file = g_file_new_for_path (cachefn); if (gs_utils_get_file_age (cachefn_file) < self->max_cache_age_secs) { g_debug ("got review data for %s from %s", @@ -758,47 +786,50 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, json_generator = json_generator_new (); json_generator_set_pretty (json_generator, TRUE); json_generator_set_root (json_generator, json_root); - data = json_generator_to_data (json_generator, NULL); + request_body = json_generator_to_data (json_generator, NULL); uri = g_strdup_printf ("%s/fetch", self->review_server); g_debug ("Updating ODRS cache for %s from %s to %s; request %s", gs_app_get_id (app), - uri, cachefn, data); + uri, cachefn, request_body); msg = soup_message_new (SOUP_METHOD_POST, uri); + data->message = g_object_ref (msg); + #if SOUP_CHECK_VERSION(3, 0, 0) g_odrs_provider_set_message_request_body (msg, "application/json; charset=utf-8", - data, strlen (data)); - bytes = soup_session_send_and_read (self->session, msg, cancellable, &local_error); - if (bytes == NULL) { - g_task_return_error (task, g_steal_pointer (&local_error)); - return; - } - - downloaded_data = g_bytes_get_data (bytes, &downloaded_data_length); - status_code = soup_message_get_status (msg); + request_body, strlen (request_body)); + soup_session_send_async (self->session, msg, G_PRIORITY_DEFAULT, + cancellable, open_input_stream_cb, g_steal_pointer (&task)); #else soup_message_set_request (msg, "application/json; charset=utf-8", - SOUP_MEMORY_COPY, data, strlen (data)); - status_code = soup_session_send_message (self->session, msg); - downloaded_data = msg->response_body ? msg->response_body->data : NULL; - downloaded_data_length = msg->response_body ? msg->response_body->length : 0; + SOUP_MEMORY_COPY, request_body, strlen (request_body)); + soup_session_send_async (self->session, msg, cancellable, + open_input_stream_cb, g_steal_pointer (&task)); #endif - if (status_code != SOUP_STATUS_OK) { - g_autoptr(GInputStream) input_stream = g_memory_input_stream_new_from_data (downloaded_data, downloaded_data_length, NULL); - if (!gs_odrs_provider_parse_success (input_stream, &local_error)) { - g_task_return_error (task, g_steal_pointer (&local_error)); - return; - } +} - /* not sure what to do here */ - g_task_return_new_error (task, - GS_ODRS_PROVIDER_ERROR, - GS_ODRS_PROVIDER_ERROR_DOWNLOADING, - "status code invalid"); - return; - } +static void +open_input_stream_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + SoupSession *soup_session = SOUP_SESSION (source_object); + g_autoptr(GTask) task = g_steal_pointer (&user_data); + FetchReviewsForAppData *data = g_task_get_task_data (task); + GCancellable *cancellable = g_task_get_cancellable (task); + g_autoptr(GInputStream) input_stream = NULL; + guint status_code; + g_autoptr(JsonParser) json_parser = NULL; + g_autoptr(GError) local_error = NULL; - /* nothing */ - if (downloaded_data == NULL) { +#if SOUP_CHECK_VERSION(3, 0, 0) + input_stream = soup_session_send_finish (soup_session, result, &local_error); + status_code = soup_message_get_status (data->message); +#else + input_stream = soup_session_send_finish (soup_session, result, &local_error); + status_code = data->message->status_code; +#endif + + if (input_stream == NULL) { if (!g_network_monitor_get_network_available (g_network_monitor_get_default ())) g_task_return_new_error (task, GS_ODRS_PROVIDER_ERROR, @@ -812,9 +843,39 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, return; } + if (status_code != SOUP_STATUS_OK) { + if (!gs_odrs_provider_parse_success (input_stream, &local_error)) { + g_task_return_error (task, g_steal_pointer (&local_error)); + return; + } + + /* not sure what to do here */ + g_task_return_new_error (task, + GS_ODRS_PROVIDER_ERROR, + GS_ODRS_PROVIDER_ERROR_DOWNLOADING, + "status code invalid"); + return; + } + /* parse the data and find the array of ratings */ json_parser = json_parser_new_immutable (); - if (!json_parser_load_from_data (json_parser, downloaded_data, downloaded_data_length, &local_error)) { + json_parser_load_from_stream_async (json_parser, input_stream, cancellable, parse_reviews_cb, g_steal_pointer (&task)); +} + +static void +parse_reviews_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + JsonParser *json_parser = JSON_PARSER (source_object); + g_autoptr(GTask) task = g_steal_pointer (&user_data); + GsOdrsProvider *self = g_task_get_source_object (task); + FetchReviewsForAppData *data = g_task_get_task_data (task); + g_autoptr(GPtrArray) reviews = NULL; + g_autoptr(JsonGenerator) cache_generator = NULL; + g_autoptr(GError) local_error = NULL; + + if (!json_parser_load_from_stream_finish (json_parser, result, &local_error)) { g_task_return_new_error (task, GS_ODRS_PROVIDER_ERROR, GS_ODRS_PROVIDER_ERROR_PARSING_DATA, @@ -829,12 +890,16 @@ gs_odrs_provider_fetch_reviews_for_app_async (GsOdrsProvider *self, } /* save to the cache */ - if (!g_file_set_contents (cachefn, downloaded_data, downloaded_data_length, &local_error)) { + cache_generator = json_generator_new (); + json_generator_set_pretty (cache_generator, FALSE); + json_generator_set_root (cache_generator, json_parser_get_root (json_parser)); + + if (!json_generator_to_file (cache_generator, data->cache_filename, &local_error)) { g_task_return_error (task, g_steal_pointer (&local_error)); return; } - set_reviews_on_app (self, app, reviews); + set_reviews_on_app (self, data->app, reviews); /* success */ g_task_return_pointer (task, g_steal_pointer (&reviews), (GDestroyNotify) g_ptr_array_unref); -- GitLab From 1d2054d19631825e8119e709b396f409db122375 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 25 Feb 2022 15:33:09 +0000 Subject: [PATCH 22/24] gs-plugin-job-refine: Make async internally Previously this was actually synchronous internally, as the changes to make it async were too big to do in the first round of porting. Signed-off-by: Philip Withnall Fixes: #1658 --- lib/gs-plugin-job-refine.c | 338 ++++++++++++++++++++++++++++++------- 1 file changed, 275 insertions(+), 63 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index 9e5bf7ca6..e8ac5d4df 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -32,6 +32,38 @@ * gs_plugin_job_refine_get_result_list(). The #GsAppList which was passed * into the job will not be modified. * + * Internally, the #GsPluginClass.refine_async() functions are called on all + * the plugins in parallel, and in parallel with a call to + * gs_odrs_provider_refine_async(). Once all of those calls are finished, + * zero or more recursive calls to run_refine_internal_async() are made in + * parallel to do a similar refine process on the addons, runtime and related + * components for all the components in the input #GsAppList. The refine job is + * complete once all these recursive calls complete. + * + * ``` + * run_async() + * | + * v + * /-----------------------+-------------+----------------\ + * | | | | + * plugin->refine_async() plugin->refine_async() … gs_odrs_provider_refine_async() + * | | | | + * v v v v + * \-----------------------+-------------+----------------/ + * | + * finish_refine_internal_op() + * | + * v + * /----------------------------+-----------------\ + * | | | + * run_refine_internal_async() run_refine_internal_async() … + * | | | + * v v v + * \----------------------------+-----------------/ + * | + * finish_refine_internal_recursion() + * ``` + * * See also: #GsPluginClass.refine_async * Since: 42 */ @@ -175,41 +207,88 @@ app_is_non_wildcard (GsApp *app, gpointer user_data) static void plugin_refine_cb (GObject *source_object, GAsyncResult *result, gpointer user_data); +static void odrs_provider_refine_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data); +static void finish_refine_internal_op (GTask *task, + GError *error); +static void recursive_internal_refine_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data); +static void finish_refine_internal_recursion (GTask *task, + GError *error); +static gboolean run_refine_internal_finish (GsPluginJobRefine *self, + GAsyncResult *result, + GError **error); + +typedef struct { + /* Input data. */ + GsPluginLoader *plugin_loader; /* (not nullable) (owned) */ + GsAppList *list; /* (not nullable) (owned) */ + GsPluginRefineFlags flags; + + /* In-progress data. */ + guint n_pending_ops; + guint n_pending_recursions; + + /* Output data. */ + GError *error; /* (nullable) (owned) */ +} RefineInternalData; static void -plugin_refine_cb (GObject *source_object, - GAsyncResult *result, - gpointer user_data) +refine_internal_data_free (RefineInternalData *data) { - GAsyncResult **result_out = user_data; + g_clear_object (&data->plugin_loader); + g_clear_object (&data->list); + + g_assert (data->n_pending_ops == 0); + g_assert (data->n_pending_recursions == 0); + + /* If an error occurred, it should have been stolen to pass to + * g_task_return_error() by now. */ + g_assert (data->error == NULL); - g_assert (*result_out == NULL); - *result_out = g_object_ref (result); - g_main_context_wakeup (g_main_context_get_thread_default ()); + g_free (data); } -static gboolean -run_refine_internal (GsPluginJobRefine *self, - GsPluginLoader *plugin_loader, - GsAppList *list, - GsPluginRefineFlags flags, - GCancellable *cancellable, - GError **error) +G_DEFINE_AUTOPTR_CLEANUP_FUNC (RefineInternalData, refine_internal_data_free) + +static void +run_refine_internal_async (GsPluginJobRefine *self, + GsPluginLoader *plugin_loader, + GsAppList *list, + GsPluginRefineFlags flags, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { GsOdrsProvider *odrs_provider; GsOdrsProviderRefineFlags odrs_refine_flags = 0; GPtrArray *plugins; /* (element-type GsPlugin) */ + g_autoptr(GTask) task = NULL; + RefineInternalData *data; + g_autoptr(RefineInternalData) data_owned = NULL; + + task = g_task_new (self, cancellable, callback, user_data); + g_task_set_source_tag (task, run_refine_internal_async); + + data = data_owned = g_new0 (RefineInternalData, 1); + data->plugin_loader = g_object_ref (plugin_loader); + data->list = g_object_ref (list); + data->flags = flags; + g_task_set_task_data (task, g_steal_pointer (&data_owned), (GDestroyNotify) refine_internal_data_free); /* try to adopt each application with a plugin */ gs_plugin_loader_run_adopt (plugin_loader, list); + data->n_pending_ops = 1; + /* run each plugin */ plugins = gs_plugin_loader_get_plugins (plugin_loader); for (guint i = 0; i < plugins->len; i++) { GsPlugin *plugin = g_ptr_array_index (plugins, i); GsPluginClass *plugin_class = GS_PLUGIN_GET_CLASS (plugin); - g_autoptr(GAsyncResult) refine_result = NULL; if (!gs_plugin_get_enabled (plugin)) continue; @@ -217,18 +296,9 @@ run_refine_internal (GsPluginJobRefine *self, continue; /* run the batched plugin symbol */ + data->n_pending_ops++; plugin_class->refine_async (plugin, list, flags, - cancellable, plugin_refine_cb, &refine_result); - - /* FIXME: Make this sync until the calling function is rearranged - * to be async. */ - while (refine_result == NULL) - g_main_context_iteration (g_main_context_get_thread_default (), TRUE); - - if (!plugin_class->refine_finish (plugin, refine_result, error)) - return FALSE; - - gs_plugin_status_update (plugin, NULL, GS_PLUGIN_STATUS_FINISHED); + cancellable, plugin_refine_cb, g_object_ref (task)); } /* Add ODRS data if needed */ @@ -241,18 +311,79 @@ run_refine_internal (GsPluginJobRefine *self, odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS; if (odrs_provider != NULL && odrs_refine_flags != 0) { - g_autoptr(GAsyncResult) odrs_refine_result = NULL; - + data->n_pending_ops++; gs_odrs_provider_refine_async (odrs_provider, list, odrs_refine_flags, - cancellable, plugin_refine_cb, &odrs_refine_result); + cancellable, odrs_provider_refine_cb, g_object_ref (task)); + } + + finish_refine_internal_op (task, NULL); +} + +static void +plugin_refine_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + GsPlugin *plugin = GS_PLUGIN (source_object); + g_autoptr(GTask) task = g_steal_pointer (&user_data); + GsPluginClass *plugin_class = GS_PLUGIN_GET_CLASS (plugin); + g_autoptr(GError) local_error = NULL; + + if (!plugin_class->refine_finish (plugin, result, &local_error)) { + finish_refine_internal_op (task, g_steal_pointer (&local_error)); + return; + } + + gs_plugin_status_update (plugin, NULL, GS_PLUGIN_STATUS_FINISHED); + + finish_refine_internal_op (task, NULL); +} + +static void +odrs_provider_refine_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + GsOdrsProvider *odrs_provider = GS_ODRS_PROVIDER (source_object); + g_autoptr(GTask) task = g_steal_pointer (&user_data); + g_autoptr(GError) local_error = NULL; + + gs_odrs_provider_refine_finish (odrs_provider, result, &local_error); + finish_refine_internal_op (task, g_steal_pointer (&local_error)); +} + +/* @error is (transfer full) if non-NULL */ +static void +finish_refine_internal_op (GTask *task, + GError *error) +{ + GsPluginJobRefine *self = g_task_get_source_object (task); + GCancellable *cancellable = g_task_get_cancellable (task); + g_autoptr(GError) error_owned = g_steal_pointer (&error); + RefineInternalData *data = g_task_get_task_data (task); + GsPluginLoader *plugin_loader = data->plugin_loader; + GsAppList *list = data->list; + GsPluginRefineFlags flags = data->flags; + + if (data->error == NULL && error_owned != NULL) { + data->error = g_steal_pointer (&error_owned); + } else if (error_owned != NULL) { + g_debug ("Additional error while refining: %s", error_owned->message); + } + + g_assert (data->n_pending_ops > 0); + data->n_pending_ops--; - /* FIXME: Make this sync until the calling function is rearranged - * to be async. */ - while (odrs_refine_result == NULL) - g_main_context_iteration (g_main_context_get_thread_default (), TRUE); + if (data->n_pending_ops > 0) + return; - if (!gs_odrs_provider_refine_finish (odrs_provider, odrs_refine_result, error)) - return FALSE; + /* At this point, all the plugin->refine() calls are complete and the + * gs_odrs_provider_refine_async() call is also complete. If an error + * occurred during those calls, return with it now rather than + * proceeding to the recursive calls below. */ + if (data->error != NULL) { + g_task_return_error (task, g_steal_pointer (&data->error)); + return; } /* filter any wildcard apps left in the list */ @@ -268,6 +399,10 @@ run_refine_internal (GsPluginJobRefine *self, } } + /* Now run several recursive calls to run_refine_internal_async() in + * parallel, to refine related components. */ + data->n_pending_recursions = 1; + /* refine addons one layer deep */ if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_ADDONS) { g_autoptr(GsAppList) addons_list = gs_app_list_new (); @@ -290,11 +425,11 @@ run_refine_internal (GsPluginJobRefine *self, } if (gs_app_list_length (addons_list) > 0 && addons_flags != 0) { - if (!run_refine_internal (self, plugin_loader, - addons_list, addons_flags, - cancellable, error)) { - return FALSE; - } + data->n_pending_recursions++; + run_refine_internal_async (self, plugin_loader, + addons_list, addons_flags, + cancellable, recursive_internal_refine_cb, + g_object_ref (task)); } } @@ -314,11 +449,11 @@ run_refine_internal (GsPluginJobRefine *self, } if (gs_app_list_length (runtimes_list) > 0 && runtimes_flags != 0) { - if (!run_refine_internal (self, plugin_loader, - runtimes_list, runtimes_flags, - cancellable, error)) { - return FALSE; - } + data->n_pending_recursions++; + run_refine_internal_async (self, plugin_loader, + runtimes_list, runtimes_flags, + cancellable, recursive_internal_refine_cb, + g_object_ref (task)); } } @@ -342,16 +477,64 @@ run_refine_internal (GsPluginJobRefine *self, } if (gs_app_list_length (related_list) > 0 && related_flags != 0) { - if (!run_refine_internal (self, plugin_loader, - related_list, related_flags, - cancellable, error)) { - return FALSE; - } + data->n_pending_recursions++; + run_refine_internal_async (self, plugin_loader, + related_list, related_flags, + cancellable, recursive_internal_refine_cb, + g_object_ref (task)); } } - /* success */ - return TRUE; + finish_refine_internal_recursion (task, NULL); +} + +static void +recursive_internal_refine_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + GsPluginJobRefine *self = GS_PLUGIN_JOB_REFINE (source_object); + g_autoptr(GTask) task = g_steal_pointer (&user_data); + g_autoptr(GError) local_error = NULL; + + run_refine_internal_finish (self, result, &local_error); + finish_refine_internal_recursion (task, g_steal_pointer (&local_error)); +} + +/* @error is (transfer full) if non-NULL */ +static void +finish_refine_internal_recursion (GTask *task, + GError *error) +{ + g_autoptr(GError) error_owned = g_steal_pointer (&error); + RefineInternalData *data = g_task_get_task_data (task); + + if (data->error == NULL && error_owned != NULL) { + data->error = g_steal_pointer (&error_owned); + } else if (error_owned != NULL) { + g_debug ("Additional error while refining: %s", error_owned->message); + } + + g_assert (data->n_pending_recursions > 0); + data->n_pending_recursions--; + + if (data->n_pending_recursions > 0) + return; + + /* The entire refine operation (and all its sub-operations and + * recursions) is complete. */ + if (data->error != NULL) + g_task_return_error (task, g_steal_pointer (&data->error)); + else + g_task_return_boolean (task, TRUE); +} + +static gboolean +run_refine_internal_finish (GsPluginJobRefine *self, + GAsyncResult *result, + GError **error) +{ + return g_task_propagate_boolean (G_TASK (result), error); } static gboolean @@ -363,6 +546,12 @@ app_thaw_notify_idle (gpointer data) return G_SOURCE_REMOVE; } +static void run_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data); +static void finish_run (GTask *task, + GsAppList *result_list); + static void gs_plugin_job_refine_run_async (GsPluginJob *job, GsPluginLoader *plugin_loader, @@ -372,8 +561,6 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, { GsPluginJobRefine *self = GS_PLUGIN_JOB_REFINE (job); g_autoptr(GTask) task = NULL; - g_autoptr(GError) local_error = NULL; - g_autofree gchar *job_debug = NULL; g_autoptr(GsAppList) result_list = NULL; /* check required args */ @@ -383,14 +570,15 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, /* Operate on a copy of the input list so we don’t modify it when * resolving wildcards. */ result_list = gs_app_list_copy (self->app_list); + g_task_set_task_data (task, g_object_ref (result_list), (GDestroyNotify) g_object_unref); /* nothing to do */ - if (self->flags == 0) { - g_debug ("no refine flags set for transaction"); - goto results; + if (self->flags == 0 || + gs_app_list_length (result_list) == 0) { + g_debug ("no refine flags set for transaction or app list is empty"); + finish_run (task, result_list); + return; } - if (gs_app_list_length (result_list) == 0) - goto results; /* freeze all apps */ for (guint i = 0; i < gs_app_list_length (self->app_list); i++) { @@ -398,8 +586,23 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, g_object_freeze_notify (G_OBJECT (app)); } - /* first pass */ - if (run_refine_internal (self, plugin_loader, result_list, self->flags, cancellable, &local_error)) { + /* Start refining the apps. */ + run_refine_internal_async (self, plugin_loader, result_list, + self->flags, cancellable, + run_cb, g_steal_pointer (&task)); +} + +static void +run_cb (GObject *source_object, + GAsyncResult *result, + gpointer user_data) +{ + GsPluginJobRefine *self = GS_PLUGIN_JOB_REFINE (source_object); + g_autoptr(GTask) task = g_steal_pointer (&user_data); + GsAppList *result_list = g_task_get_task_data (task); + g_autoptr(GError) local_error = NULL; + + if (run_refine_internal_finish (self, result, &local_error)) { /* remove any addons that have the same source as the parent app */ for (guint i = 0; i < gs_app_list_length (result_list); i++) { g_autoptr(GPtrArray) to_remove = g_ptr_array_new (); @@ -443,7 +646,16 @@ gs_plugin_job_refine_run_async (GsPluginJob *job, return; } -results: + finish_run (task, result_list); +} + +static void +finish_run (GTask *task, + GsAppList *result_list) +{ + GsPluginJobRefine *self = g_task_get_source_object (task); + g_autofree gchar *job_debug = NULL; + /* Internal calls to #GsPluginJobRefine may want to do their own * filtering, typically if the refine is being done as part of another * plugin job. If so, only filter to remove wildcards. Wildcards should @@ -458,7 +670,7 @@ results: gs_app_list_filter (result_list, app_is_valid_filter, self); /* show elapsed time */ - job_debug = gs_plugin_job_to_string (job); + job_debug = gs_plugin_job_to_string (GS_PLUGIN_JOB (self)); g_debug ("%s", job_debug); /* success */ -- GitLab From 5dc0d66f56b982a8c32b9dc16354e28b102fc752 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 28 Feb 2022 19:44:54 +0000 Subject: [PATCH 23/24] gs-plugin-job-refine: Execute refine vfuncs in series not parallel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unfortunately we can’t run them in parallel until all of the plugins are entirely independent. Currently, the results of one plugin’s refine function can depend on the results of another’s. For example, apps from the `dummy` plugin require being refined by the `dummy` plugin and the `appstream` plugin before they work correctly. This commit can be reverted once the plugins are independent. Signed-off-by: Philip Withnall Helps: #1472 --- lib/gs-plugin-job-refine.c | 100 +++++++++++++++++++++++++++++-------- 1 file changed, 79 insertions(+), 21 deletions(-) diff --git a/lib/gs-plugin-job-refine.c b/lib/gs-plugin-job-refine.c index e8ac5d4df..dabc8de42 100644 --- a/lib/gs-plugin-job-refine.c +++ b/lib/gs-plugin-job-refine.c @@ -33,22 +33,30 @@ * into the job will not be modified. * * Internally, the #GsPluginClass.refine_async() functions are called on all - * the plugins in parallel, and in parallel with a call to + * the plugins in series, and in series with a call to * gs_odrs_provider_refine_async(). Once all of those calls are finished, * zero or more recursive calls to run_refine_internal_async() are made in * parallel to do a similar refine process on the addons, runtime and related * components for all the components in the input #GsAppList. The refine job is * complete once all these recursive calls complete. * + * FIXME: Ideally, the #GsPluginClass.refine_async() calls would happen in + * parallel, but this cannot be the case until the results of the refine_async() + * call in one plugin don’t depend on the results of refine_async() in another. + * This still happens with several pairs of plugins. + * * ``` * run_async() * | * v * /-----------------------+-------------+----------------\ * | | | | - * plugin->refine_async() plugin->refine_async() … gs_odrs_provider_refine_async() + * plugin->refine_async() | | | + * v plugin->refine_async() | | + * | v … | + * | | v gs_odrs_provider_refine_async() + * | | | v * | | | | - * v v v v * \-----------------------+-------------+----------------/ * | * finish_refine_internal_op() @@ -230,6 +238,7 @@ typedef struct { /* In-progress data. */ guint n_pending_ops; guint n_pending_recursions; + guint next_plugin_index; /* Output data. */ GError *error; /* (nullable) (owned) */ @@ -262,8 +271,6 @@ run_refine_internal_async (GsPluginJobRefine *self, GAsyncReadyCallback callback, gpointer user_data) { - GsOdrsProvider *odrs_provider; - GsOdrsProviderRefineFlags odrs_refine_flags = 0; GPtrArray *plugins; /* (element-type GsPlugin) */ g_autoptr(GTask) task = NULL; RefineInternalData *data; @@ -281,9 +288,18 @@ run_refine_internal_async (GsPluginJobRefine *self, /* try to adopt each application with a plugin */ gs_plugin_loader_run_adopt (plugin_loader, list); - data->n_pending_ops = 1; + data->n_pending_ops = 0; - /* run each plugin */ + /* run each plugin + * + * FIXME: For now, we have to run these vfuncs sequentially rather than + * all in parallel. This is because there are still dependencies between + * some of the plugins, where the code to refine an app in one plugin + * depends on the results of refining it in another plugin first. + * + * Eventually, the plugins should all be changed/removed so that they + * can operate independently. At that point, this code can be reverted + * so that the refine_async() vfuncs are called in parallel. */ plugins = gs_plugin_loader_get_plugins (plugin_loader); for (guint i = 0; i < plugins->len; i++) { @@ -295,27 +311,21 @@ run_refine_internal_async (GsPluginJobRefine *self, if (plugin_class->refine_async == NULL) continue; + /* FIXME: The next refine_async() call is made in + * finish_refine_internal_op(). */ + data->next_plugin_index = i + 1; + /* run the batched plugin symbol */ data->n_pending_ops++; plugin_class->refine_async (plugin, list, flags, cancellable, plugin_refine_cb, g_object_ref (task)); - } - - /* Add ODRS data if needed */ - odrs_provider = gs_plugin_loader_get_odrs_provider (plugin_loader); - if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) - odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS; - if (flags & (GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS | - GS_PLUGIN_REFINE_FLAGS_REQUIRE_RATING)) - odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS; - - if (odrs_provider != NULL && odrs_refine_flags != 0) { - data->n_pending_ops++; - gs_odrs_provider_refine_async (odrs_provider, list, odrs_refine_flags, - cancellable, odrs_provider_refine_cb, g_object_ref (task)); + /* FIXME: The next refine_async() call is made in + * finish_refine_internal_op(). */ + return; } + data->n_pending_ops++; finish_refine_internal_op (task, NULL); } @@ -364,6 +374,9 @@ finish_refine_internal_op (GTask *task, GsPluginLoader *plugin_loader = data->plugin_loader; GsAppList *list = data->list; GsPluginRefineFlags flags = data->flags; + GsOdrsProvider *odrs_provider; + GsOdrsProviderRefineFlags odrs_refine_flags = 0; + GPtrArray *plugins; /* (element-type GsPlugin) */ if (data->error == NULL && error_owned != NULL) { data->error = g_steal_pointer (&error_owned); @@ -374,6 +387,51 @@ finish_refine_internal_op (GTask *task, g_assert (data->n_pending_ops > 0); data->n_pending_ops--; + plugins = gs_plugin_loader_get_plugins (plugin_loader); + + for (guint i = data->next_plugin_index; i < plugins->len; i++) { + GsPlugin *plugin = g_ptr_array_index (plugins, i); + GsPluginClass *plugin_class = GS_PLUGIN_GET_CLASS (plugin); + + if (!gs_plugin_get_enabled (plugin)) + continue; + if (plugin_class->refine_async == NULL) + continue; + + /* FIXME: The next refine_async() call is made in + * finish_refine_internal_op(). */ + data->next_plugin_index = i + 1; + + /* run the batched plugin symbol */ + data->n_pending_ops++; + plugin_class->refine_async (plugin, list, flags, + cancellable, plugin_refine_cb, g_object_ref (task)); + + /* FIXME: The next refine_async() call is made in + * finish_refine_internal_op(). */ + return; + } + + if (data->next_plugin_index == plugins->len) { + /* Avoid the ODRS refine being run multiple times. */ + data->next_plugin_index++; + + /* Add ODRS data if needed */ + odrs_provider = gs_plugin_loader_get_odrs_provider (plugin_loader); + + if (flags & GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEWS) + odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_REVIEWS; + if (flags & (GS_PLUGIN_REFINE_FLAGS_REQUIRE_REVIEW_RATINGS | + GS_PLUGIN_REFINE_FLAGS_REQUIRE_RATING)) + odrs_refine_flags |= GS_ODRS_PROVIDER_REFINE_FLAGS_GET_RATINGS; + + if (odrs_provider != NULL && odrs_refine_flags != 0) { + data->n_pending_ops++; + gs_odrs_provider_refine_async (odrs_provider, list, odrs_refine_flags, + cancellable, odrs_provider_refine_cb, g_object_ref (task)); + } + } + if (data->n_pending_ops > 0) return; -- GitLab From 606d0c2a8017ee3652f17c3863ff67b4335e243e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 28 Feb 2022 19:47:39 +0000 Subject: [PATCH 24/24] appstream: Ensure state is always refined when refining an app This makes these two call sites of `gs_appstream_refine_app()` match the other call site, where `gs_plugin_appstream_set_compulsory_quirk()` and `gs_plugin_appstream_refine_state()` are also called. Signed-off-by: Philip Withnall --- plugins/core/gs-plugin-appstream.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/plugins/core/gs-plugin-appstream.c b/plugins/core/gs-plugin-appstream.c index cde6a5c2c..e67a94290 100644 --- a/plugins/core/gs-plugin-appstream.c +++ b/plugins/core/gs-plugin-appstream.c @@ -1090,6 +1090,12 @@ gs_plugin_refine_from_pkgname (GsPluginAppstream *self, gs_plugin_appstream_set_compulsory_quirk (app, component); } + /* if an installed desktop or appdata file exists set to installed */ + if (gs_app_get_state (app) == GS_APP_STATE_UNKNOWN) { + if (!gs_plugin_appstream_refine_state (self, app, error)) + return FALSE; + } + /* success */ return TRUE; } @@ -1245,6 +1251,14 @@ refine_wildcard (GsPluginAppstream *self, if (!gs_appstream_refine_app (GS_PLUGIN (self), new, self->silo, component, refine_flags, error)) return FALSE; + gs_plugin_appstream_set_compulsory_quirk (new, component); + + /* if an installed desktop or appdata file exists set to installed */ + if (gs_app_get_state (new) == GS_APP_STATE_UNKNOWN) { + if (!gs_plugin_appstream_refine_state (self, new, error)) + return FALSE; + } + gs_app_list_add (list, new); } -- GitLab