[gnome-software: 1/2] appstream: Ensure XbSilo monitors are attached to right context

[Philip Withnall <pwithnall src gnome org>]

commit 6fb78f853b8183501fba791b8539210e42af7677
Author: Philip Withnall <pwithnall endlessos org>
Date:   Thu Sep 16 17:59:03 2021 +0100

    appstream: Ensure XbSilo monitors are attached to right context
    
    `xb_builder_ensure()`, `xb_builder_compile()` and `xb_silo_watch_file()`
    all need to be called with a thread-default main context which is
    suitable for attaching the silo’s file monitors to. The silo’s file
    monitors watch for changes in `/usr/share/applications`,
    `/usr/share/metainfo`, etc., and invalidate the silo if an application
    is installed or uninstalled.
    
    Currently, `xb_builder_ensure()` is called once at startup in the main
    thread, and it creates the initial silo. The file monitors for this silo
    are attached to the global default main context and work fine.
    
    When the appstream is next refreshed (due to a refresh timer firing, or
    an application being installed or removed), that refresh is done in a
    worker thread by the `GsPluginLoader`. Since commit 5d0a641c3daa2b, that
    worker thread uses a new thread-default main context, which only exists
    for the lifetime of that task. The `XbSilo` which is constructed is
    passed back to the main thread, and used once the refresh is complete to
    provide app data.
    
    Unfortunately that means that the file monitors for the newly-created
    `XbSilo` cease to function after the worker thread completes the task.
    Events queue up in the `event_queue` of the `GLocalFileMonitor`, but are
    never drained from the queue because the `GFileMonitorSource` for that
    monitor is no longer attached to a `GMainContext`.
    
    I cannot think of a good and straightforward fix for this.
    
    The workaround in this commit temporarily pops the worker thread’s main
    context while calling any libxmlb functions which create a
    `GFileMonitor`, and then re-push the main context immediately
    afterwards. This means that the file monitors will use the global
    default main context to invoke their callbacks. The callbacks call
    `xb_silo_invalidate()` only, the implementation of which happens to be
    thread-safe. By inspection, no other code is called while the main
    context is popped which might get messed up by using the global main
    context instead of the worker thread’s main context.
    
    A proper fix here would probably involve splitting appstream handling
    out to a dedicated appstream worker thread, which has a long-lived main
    context. That is weeks of work, though.
    
    I will follow this up with some documentation improvements to libxmlb to
    try and make this situation easier to avoid for others in future.
    
    Signed-off-by: Philip Withnall <pwithnall endlessos org>
    
    Fixes: #1422

 gs-install-appstream/gs-install-appstream.c |  2 ++
 plugins/core/gs-plugin-appstream.c          | 27 ++++++++++++++++++++++++---
 plugins/flatpak/gs-flatpak.c                | 28 ++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 3 deletions(-)
---
diff --git a/gs-install-appstream/gs-install-appstream.c b/gs-install-appstream/gs-install-appstream.c
index c28e1e921..4108d4971 100644
--- a/gs-install-appstream/gs-install-appstream.c
+++ b/gs-install-appstream/gs-install-appstream.c
@@ -94,6 +94,8 @@ gs_install_appstream_check_content_type (GFile *file, GError **error)
                return FALSE;
        }
        xb_builder_import_source (builder, source);
+       /* No need to change the thread-default main context because the silo
+        * doesn’t live beyond this function. */
        silo = xb_builder_compile (builder,
                                   XB_BUILDER_COMPILE_FLAG_NONE,
                                   NULL, &error_local);
diff --git a/plugins/core/gs-plugin-appstream.c b/plugins/core/gs-plugin-appstream.c
index f7b3ae428..3b7728efa 100644
--- a/plugins/core/gs-plugin-appstream.c
+++ b/plugins/core/gs-plugin-appstream.c
@@ -513,6 +513,7 @@ gs_plugin_appstream_check_silo (GsPlugin *plugin,
        g_autoptr(GPtrArray) parent_appdata = g_ptr_array_new_with_free_func (g_free);
        g_autoptr(GPtrArray) parent_appstream = g_ptr_array_new_with_free_func (g_free);
        const gchar *const *locales = g_get_language_names ();
+       g_autoptr(GMainContext) old_thread_default = NULL;
 
        reader_locker = g_rw_lock_reader_locker_new (&priv->silo_lock);
        /* everything is okay */
@@ -638,27 +639,47 @@ gs_plugin_appstream_check_silo (GsPlugin *plugin,
                return FALSE;
        file = g_file_new_for_path (blobfn);
        g_debug ("ensuring %s", blobfn);
+
+       /* 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);
+
        priv->silo = xb_builder_ensure (builder, file,
                                        XB_BUILDER_COMPILE_FLAG_IGNORE_INVALID |
                                        XB_BUILDER_COMPILE_FLAG_SINGLE_LANG,
                                        NULL, error);
-       if (priv->silo == NULL)
+       if (priv->silo == NULL) {
+               if (old_thread_default != NULL)
+                       g_main_context_push_thread_default (old_thread_default);
                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 (priv->silo, file_tmp, cancellable, error))
+               if (!xb_silo_watch_file (priv->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 (priv->silo, file_tmp, cancellable, error))
+               if (!xb_silo_watch_file (priv->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);
+
        /* test we found something */
        n = xb_silo_query_first (priv->silo, "components/component", NULL);
        if (n == NULL) {
diff --git a/plugins/flatpak/gs-flatpak.c b/plugins/flatpak/gs-flatpak.c
index f3e77f041..55a91af2a 100644
--- a/plugins/flatpak/gs-flatpak.c
+++ b/plugins/flatpak/gs-flatpak.c
@@ -960,6 +960,7 @@ gs_flatpak_rescan_appstream_store (GsFlatpak *self,
        g_autoptr(GRWLockReaderLocker) reader_locker = NULL;
        g_autoptr(GRWLockWriterLocker) writer_locker = NULL;
        g_autoptr(XbBuilder) builder = xb_builder_new ();
+       g_autoptr(GMainContext) old_thread_default = NULL;
 
        reader_locker = g_rw_lock_reader_locker_new (&self->silo_lock);
        /* everything is okay */
@@ -1016,10 +1017,22 @@ gs_flatpak_rescan_appstream_store (GsFlatpak *self,
                return FALSE;
        file = g_file_new_for_path (blobfn);
        g_debug ("ensuring %s", blobfn);
+
+       /* 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);
+
        self->silo = xb_builder_ensure (builder, file,
                                        XB_BUILDER_COMPILE_FLAG_IGNORE_INVALID |
                                        XB_BUILDER_COMPILE_FLAG_SINGLE_LANG,
                                        NULL, error);
+
+       if (old_thread_default != NULL)
+               g_main_context_push_thread_default (old_thread_default);
+
        if (self->silo == NULL)
                return FALSE;
 
@@ -2688,6 +2701,7 @@ gs_flatpak_refine_appstream_from_bytes (GsFlatpak *self,
        g_autoptr(GInputStream) stream_data = NULL;
        g_autoptr(GInputStream) stream_gz = NULL;
        g_autoptr(GZlibDecompressor) decompressor = NULL;
+       g_autoptr(GMainContext) old_thread_default = NULL;
 
        /* add current locales */
        for (guint i = 0; locales[i] != NULL; i++)
@@ -2746,10 +2760,22 @@ gs_flatpak_refine_appstream_from_bytes (GsFlatpak *self,
        }
 
        xb_builder_import_source (builder, source);
+
+       /* 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);
+
        silo = xb_builder_compile (builder,
                                   XB_BUILDER_COMPILE_FLAG_SINGLE_LANG,
                                   cancellable,
                                   error);
+
+       if (old_thread_default != NULL)
+               g_main_context_push_thread_default (old_thread_default);
+
        if (silo == NULL)
                return FALSE;
        if (g_getenv ("GS_XMLB_VERBOSE") != NULL) {
@@ -3522,6 +3548,8 @@ gs_flatpak_file_to_app_ref (GsFlatpak *self,
                return NULL;
 
        /* build silo */
+       /* No need to change the thread-default main context because the silo
+        * doesn’t live beyond this function */
        silo = xb_builder_compile (builder,
                                   XB_BUILDER_COMPILE_FLAG_SINGLE_LANG,
                                   cancellable,