[gnome-software/1517-gs-plugin-appstream-race-on-appstream-silo-changes] gs-plugin-appstream: Race on appstream silo changes

[Milan Crha <mcrha src gnome org>]

commit 880e2a5498ddd8419db6e75599e714da5a92f7a6
Author: Milan Crha <mcrha redhat com>
Date:   Tue May 3 10:28:42 2022 +0200

    gs-plugin-appstream: Race on appstream silo changes
    
    Instead of relying on XbSilo watchers use its own GFileMonitor, which
    can detect changes also when loading the appstream data from various
    directories, thus it can also reload the data again, if needed, instead
    of using possibly stale appstream data.
    
    Closes https://gitlab.gnome.org/GNOME/gnome-software/-/issues/1517

 plugins/core/gs-plugin-appstream.c | 145 ++++++++++++++++++++++++-------------
 1 file changed, 93 insertions(+), 52 deletions(-)
---
diff --git a/plugins/core/gs-plugin-appstream.c b/plugins/core/gs-plugin-appstream.c
index f56352960..84c5072ec 100644
--- a/plugins/core/gs-plugin-appstream.c
+++ b/plugins/core/gs-plugin-appstream.c
@@ -39,6 +39,12 @@ struct _GsPluginAppstream
        XbSilo                  *silo;
        GRWLock                  silo_lock;
        GSettings               *settings;
+
+       GPtrArray               *file_monitors; /* (owned) (element-type GFileMonitor) */
+       /* The stamps help to avoid locking the silo lock in the main thread
+          and also to detect changes while loading other appstream data. */
+       gint                     file_monitor_stamp; /* the file monitor stamp, increased on every file 
monitor change */
+       gint                     file_monitor_stamp_current; /* the currently known file monitor stamp, 
checked for changes */
 };
 
 G_DEFINE_TYPE (GsPluginAppstream, gs_plugin_appstream, GS_TYPE_PLUGIN)
@@ -55,6 +61,7 @@ gs_plugin_appstream_dispose (GObject *object)
        g_clear_object (&self->settings);
        g_rw_lock_clear (&self->silo_lock);
        g_clear_object (&self->worker);
+       g_clear_pointer (&self->file_monitors, g_ptr_array_unref);
 
        G_OBJECT_CLASS (gs_plugin_appstream_parent_class)->dispose (object);
 }
@@ -79,6 +86,8 @@ gs_plugin_appstream_init (GsPluginAppstream *self)
                g_signal_connect_object (application, "repository-changed",
                        G_CALLBACK (gs_plugin_update_cache_state_for_repository), self, G_CONNECT_SWAPPED);
        }
+
+       self->file_monitors = g_ptr_array_new_with_free_func (g_object_unref);
 }
 
 static const gchar *
@@ -206,6 +215,30 @@ gs_plugin_appstream_media_baseurl_cb (XbBuilderFixup *self,
        return TRUE;
 }
 
+static void
+gs_plugin_appstream_file_monitor_changed_cb (GFileMonitor *monitor,
+                                            GFile *file,
+                                            GFile *other_file,
+                                            GFileMonitorEvent event_type,
+                                            gpointer user_data)
+{
+       GsPluginAppstream *self = user_data;
+       g_atomic_int_inc (&self->file_monitor_stamp);
+}
+
+static void
+gs_plugin_appstream_maybe_store_file_monitor (GsPluginAppstream  *self,
+                                             GFileMonitor *file_monitor) /* (nullable) (transfer none) */
+{
+       if (!file_monitor)
+               return;
+
+       g_signal_connect_object (file_monitor, "changed",
+               G_CALLBACK (gs_plugin_appstream_file_monitor_changed_cb), self, 0);
+
+       g_ptr_array_add (self->file_monitors, g_object_ref (file_monitor));
+}
+
 static gboolean
 gs_plugin_appstream_load_appdata_fn (GsPluginAppstream  *self,
                                      XbBuilder          *builder,
@@ -219,16 +252,8 @@ gs_plugin_appstream_load_appdata_fn (GsPluginAppstream  *self,
        g_autoptr(XbBuilderSource) source = xb_builder_source_new ();
 
        /* add source */
-       if (!xb_builder_source_load_file (source, file,
-#if LIBXMLB_CHECK_VERSION(0, 2, 0)
-                                         XB_BUILDER_SOURCE_FLAG_WATCH_DIRECTORY,
-#else
-                                         XB_BUILDER_SOURCE_FLAG_WATCH_FILE,
-#endif
-                                         cancellable,
-                                         error)) {
+       if (!xb_builder_source_load_file (source, file, 0, cancellable, error))
                return FALSE;
-       }
 
        /* fix up any legacy installed files */
        fixup = xb_builder_fixup_new ("AppStreamUpgrade2",
@@ -257,6 +282,8 @@ gs_plugin_appstream_load_appdata (GsPluginAppstream  *self,
        const gchar *fn;
        g_autoptr(GDir) dir = NULL;
        g_autoptr(GFile) parent = g_file_new_for_path (path);
+       g_autoptr(GFileMonitor) file_monitor = NULL;
+       g_autoptr(GError) local_error = NULL;
        if (!g_file_query_exists (parent, cancellable)) {
                g_debug ("appstream: Skipping appdata path '%s' as %s", path, g_cancellable_is_cancelled 
(cancellable) ? "cancelled" : "does not exist");
                return TRUE;
@@ -268,6 +295,11 @@ gs_plugin_appstream_load_appdata (GsPluginAppstream  *self,
        if (dir == NULL)
                return FALSE;
 
+       file_monitor = g_file_monitor (parent, G_FILE_MONITOR_NONE, cancellable, &local_error);
+       if (local_error)
+               g_debug ("appstream: Failed to create file monitor for '%s': %s", path, local_error->message);
+       gs_plugin_appstream_maybe_store_file_monitor (self, file_monitor);
+
        while ((fn = g_dir_read_name (dir)) != NULL) {
                if (g_str_has_suffix (fn, ".appdata.xml") ||
                    g_str_has_suffix (fn, ".metainfo.xml")) {
@@ -335,16 +367,8 @@ gs_plugin_appstream_load_desktop_fn (GsPluginAppstream  *self,
                                       gs_plugin_appstream_load_desktop_cb, NULL, NULL);
 
        /* add source */
-       if (!xb_builder_source_load_file (source, file,
-#if LIBXMLB_CHECK_VERSION(0, 2, 0)
-                                         XB_BUILDER_SOURCE_FLAG_WATCH_DIRECTORY,
-#else
-                                         XB_BUILDER_SOURCE_FLAG_WATCH_FILE,
-#endif
-                                         cancellable,
-                                         error)) {
+       if (!xb_builder_source_load_file (source, file, 0, cancellable, error))
                return FALSE;
-       }
 
        /* add metadata */
        info = xb_builder_node_insert (NULL, "info", NULL);
@@ -366,6 +390,8 @@ gs_plugin_appstream_load_desktop (GsPluginAppstream  *self,
        const gchar *fn;
        g_autoptr(GDir) dir = NULL;
        g_autoptr(GFile) parent = g_file_new_for_path (path);
+       g_autoptr(GFileMonitor) file_monitor = NULL;
+       g_autoptr(GError) local_error = NULL;
        if (!g_file_query_exists (parent, cancellable)) {
                g_debug ("appstream: Skipping desktop path '%s' as %s", path, g_cancellable_is_cancelled 
(cancellable) ? "cancelled" : "does not exist");
                return TRUE;
@@ -377,6 +403,11 @@ gs_plugin_appstream_load_desktop (GsPluginAppstream  *self,
        if (dir == NULL)
                return FALSE;
 
+       file_monitor = g_file_monitor (parent, G_FILE_MONITOR_NONE, cancellable, &local_error);
+       if (local_error)
+               g_debug ("appstream: Failed to create file monitor for '%s': %s", path, local_error->message);
+       gs_plugin_appstream_maybe_store_file_monitor (self, file_monitor);
+
        while ((fn = g_dir_read_name (dir)) != NULL) {
                if (g_str_has_suffix (fn, ".desktop")) {
                        g_autofree gchar *filename = g_build_filename (path, fn, NULL);
@@ -489,16 +520,8 @@ gs_plugin_appstream_load_appstream_fn (GsPluginAppstream  *self,
                                       NULL, NULL);
 
        /* add source */
-       if (!xb_builder_source_load_file (source, file,
-#if LIBXMLB_CHECK_VERSION(0, 2, 0)
-                                         XB_BUILDER_SOURCE_FLAG_WATCH_DIRECTORY,
-#else
-                                         XB_BUILDER_SOURCE_FLAG_WATCH_FILE,
-#endif
-                                         cancellable,
-                                         error)) {
+       if (!xb_builder_source_load_file (source, file, 0, cancellable, error))
                return FALSE;
-       }
 
        /* add metadata */
        info = xb_builder_node_insert (NULL, "info", NULL);
@@ -558,6 +581,8 @@ gs_plugin_appstream_load_appstream (GsPluginAppstream  *self,
        const gchar *fn;
        g_autoptr(GDir) dir = NULL;
        g_autoptr(GFile) parent = g_file_new_for_path (path);
+       g_autoptr(GFileMonitor) file_monitor = NULL;
+       g_autoptr(GError) local_error = NULL;
 
        /* parent path does not exist */
        if (!g_file_query_exists (parent, cancellable)) {
@@ -568,6 +593,12 @@ gs_plugin_appstream_load_appstream (GsPluginAppstream  *self,
        dir = g_dir_open (path, 0, error);
        if (dir == NULL)
                return FALSE;
+
+       file_monitor = g_file_monitor (parent, G_FILE_MONITOR_NONE, cancellable, &local_error);
+       if (local_error)
+               g_debug ("appstream: Failed to create file monitor for '%s': %s", path, local_error->message);
+       gs_plugin_appstream_maybe_store_file_monitor (self, file_monitor);
+
        while ((fn = g_dir_read_name (dir)) != NULL) {
 #ifdef ENABLE_EXTERNAL_APPSTREAM
                /* Ignore our own system-installed files when
@@ -662,13 +693,17 @@ gs_plugin_appstream_check_silo (GsPluginAppstream  *self,
 
        reader_locker = g_rw_lock_reader_locker_new (&self->silo_lock);
        /* everything is okay */
-       if (self->silo != NULL && xb_silo_is_valid (self->silo))
+       if (self->silo != NULL && xb_silo_is_valid (self->silo) &&
+           g_atomic_int_get (&self->file_monitor_stamp_current) == g_atomic_int_get 
(&self->file_monitor_stamp))
                return TRUE;
        g_clear_pointer (&reader_locker, g_rw_lock_reader_locker_free);
 
        /* drat! silo needs regenerating */
        writer_locker = g_rw_lock_writer_locker_new (&self->silo_lock);
+ reload:
        g_clear_object (&self->silo);
+       g_ptr_array_set_size (self->file_monitors, 0);
+       g_atomic_int_set (&self->file_monitor_stamp_current, g_atomic_int_get (&self->file_monitor_stamp));
 
        /* FIXME: https://gitlab.gnome.org/GNOME/gnome-software/-/issues/1422 */
        old_thread_default = g_main_context_ref_thread_default ();
@@ -777,30 +812,49 @@ gs_plugin_appstream_check_silo (GsPluginAppstream  *self,
                        gs_add_appstream_catalog_location (parent_appstream, "/var/lib");
                }
 
+               /* FIXME: https://gitlab.gnome.org/GNOME/gnome-software/-/issues/1422 */
+               old_thread_default = g_main_context_ref_thread_default ();
+               if (old_thread_default == g_main_context_default ())
+                       g_clear_pointer (&old_thread_default, g_main_context_unref);
+               if (old_thread_default != NULL)
+                       g_main_context_pop_thread_default (old_thread_default);
+
                /* import all files */
                for (guint i = 0; i < parent_appstream->len; i++) {
                        const gchar *fn = g_ptr_array_index (parent_appstream, i);
-                       if (!gs_plugin_appstream_load_appstream (self, builder, fn,
-                                                                cancellable, error))
+                       if (!gs_plugin_appstream_load_appstream (self, builder, fn, cancellable, error)) {
+                               if (old_thread_default != NULL)
+                                       g_main_context_push_thread_default (old_thread_default);
                                return FALSE;
+                       }
                }
                for (guint i = 0; i < parent_appdata->len; i++) {
                        const gchar *fn = g_ptr_array_index (parent_appdata, i);
-                       if (!gs_plugin_appstream_load_appdata (self, builder, fn,
-                                                              cancellable, error))
+                       if (!gs_plugin_appstream_load_appdata (self, builder, fn, cancellable, error)) {
+                               if (old_thread_default != NULL)
+                                       g_main_context_push_thread_default (old_thread_default);
                                return FALSE;
+                       }
                }
                if (!gs_plugin_appstream_load_desktop (self, builder,
                                                       DATADIR "/applications",
                                                       cancellable, error)) {
+                       if (old_thread_default != NULL)
+                               g_main_context_push_thread_default (old_thread_default);
                        return FALSE;
                }
                if (g_strcmp0 (DATADIR, "/usr/share") != 0 &&
                    !gs_plugin_appstream_load_desktop (self, builder,
                                                       "/usr/share/applications",
                                                       cancellable, error)) {
+                       if (old_thread_default != NULL)
+                               g_main_context_push_thread_default (old_thread_default);
                        return FALSE;
                }
+
+               if (old_thread_default != NULL)
+                       g_main_context_push_thread_default (old_thread_default);
+               g_clear_pointer (&old_thread_default, g_main_context_unref);
        }
 
        /* regenerate with each minor release */
@@ -833,29 +887,16 @@ gs_plugin_appstream_check_silo (GsPluginAppstream  *self,
                return FALSE;
        }
 
-       /* watch all directories too */
-       for (guint i = 0; i < parent_appstream->len; i++) {
-               const gchar *fn = g_ptr_array_index (parent_appstream, i);
-               g_autoptr(GFile) file_tmp = g_file_new_for_path (fn);
-               if (!xb_silo_watch_file (self->silo, file_tmp, cancellable, error)) {
-                       if (old_thread_default != NULL)
-                               g_main_context_push_thread_default (old_thread_default);
-                       return FALSE;
-               }
-       }
-       for (guint i = 0; i < parent_appdata->len; i++) {
-               const gchar *fn = g_ptr_array_index (parent_appdata, i);
-               g_autoptr(GFile) file_tmp = g_file_new_for_path (fn);
-               if (!xb_silo_watch_file (self->silo, file_tmp, cancellable, error)) {
-                       if (old_thread_default != NULL)
-                               g_main_context_push_thread_default (old_thread_default);
-                       return FALSE;
-               }
-       }
-
        if (old_thread_default != NULL)
                g_main_context_push_thread_default (old_thread_default);
 
+       if (g_atomic_int_get (&self->file_monitor_stamp_current) != g_atomic_int_get 
(&self->file_monitor_stamp)) {
+               g_ptr_array_set_size (parent_appdata, 0);
+               g_ptr_array_set_size (parent_appstream, 0);
+               g_debug ("appstream: File monitors reported change while loading appstream data, 
reloading...");
+               goto reload;
+       }
+
        /* test we found something */
        n = xb_silo_query_first (self->silo, "components/component", NULL);
        if (n == NULL) {