From 84fe2ffb2bcc788916beb563b77cd6fe2c836456 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 31 Aug 2015 22:37:37 -0400 Subject: [PATCH] pull: Go back to using one main context xdg-app was hanging for me with v2015.8, but worked with v2015.7. I narrowed things down to the GMainLoop/context commit, in which we started pushing a temporary main context for synchronous requests internally. That's never really going to work with libsoup - there needs to be a single main context which works on the socket. Furthermore, clients couldn't get progress messages that way. For *other* internal uses where we added APIs that talk to the remote repo, we cleanly push a temporary main context. (Note that I kind of snuck in a change here around the GError handling in pulls that isn't strictly related but came up in testing) --- src/libostree/ostree-fetcher.c | 9 +++++---- src/libostree/ostree-metalink.c | 10 ++++++---- src/libostree/ostree-repo-pull.c | 15 ++++++++++++++- src/libostree/ostree-repo.c | 8 +++++++- 4 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/libostree/ostree-fetcher.c b/src/libostree/ostree-fetcher.c index bc2b676d..37e0f99d 100644 --- a/src/libostree/ostree-fetcher.c +++ b/src/libostree/ostree-fetcher.c @@ -731,6 +731,10 @@ fetch_uri_sync_on_complete (GObject *object, data->done = TRUE; } +/* Synchronously request a URI - will iterate the thread-default main + * context for historical reasons. If you don't want that, push a + * temporary one. + */ gboolean _ostree_fetcher_request_uri_to_membuf (OstreeFetcher *fetcher, SoupURI *uri, @@ -754,8 +758,7 @@ _ostree_fetcher_request_uri_to_membuf (OstreeFetcher *fetcher, if (g_cancellable_set_error_if_cancelled (cancellable, error)) return FALSE; - mainctx = g_main_context_new (); - g_main_context_push_thread_default (mainctx); + mainctx = g_main_context_ref_thread_default (); data.done = FALSE; data.error = error; @@ -800,8 +803,6 @@ _ostree_fetcher_request_uri_to_membuf (OstreeFetcher *fetcher, ret = TRUE; *out_contents = g_memory_output_stream_steal_as_bytes (buf); out: - if (mainctx) - g_main_context_pop_thread_default (mainctx); g_clear_object (&(data.result_stream)); return ret; } diff --git a/src/libostree/ostree-metalink.c b/src/libostree/ostree-metalink.c index 5ca69e69..083ad16f 100644 --- a/src/libostree/ostree-metalink.c +++ b/src/libostree/ostree-metalink.c @@ -589,6 +589,11 @@ typedef struct GMainLoop *loop; } FetchMetalinkSyncData; +/* + * Note that for legacy reasons we iterate the caller's main context. + * If you don't want that (and you probably don't) push a temporary + * one. + */ gboolean _ostree_metalink_request_sync (OstreeMetalink *self, SoupURI **out_target_uri, @@ -607,8 +612,7 @@ _ostree_metalink_request_sync (OstreeMetalink *self, if (fetching_sync_uri != NULL) *fetching_sync_uri = _ostree_metalink_get_uri (self); - mainctx = g_main_context_new (); - g_main_context_push_thread_default (mainctx); + mainctx = g_main_context_ref_thread_default (); request.metalink = g_object_ref (self); request.urls = g_ptr_array_new_with_free_func ((GDestroyNotify) soup_uri_free); @@ -633,8 +637,6 @@ _ostree_metalink_request_sync (OstreeMetalink *self, ret = TRUE; out: - if (mainctx) - g_main_context_pop_thread_default (mainctx); g_clear_object (&request.metalink); g_clear_pointer (&request.urls, g_ptr_array_unref); g_clear_pointer (&request.parser, g_markup_parse_context_free); diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index e82340ab..28198a40 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -96,6 +96,7 @@ typedef struct { guint64 previous_bytes_sec; guint64 previous_total_downloaded; + GError *cached_async_error; GError **async_error; gboolean caught_error; @@ -1697,7 +1698,10 @@ ostree_repo_pull_with_options (OstreeRepo *self, pull_data->is_mirror = (flags & OSTREE_REPO_PULL_FLAGS_MIRROR) > 0; pull_data->is_commit_only = (flags & OSTREE_REPO_PULL_FLAGS_COMMIT_ONLY) > 0; - pull_data->async_error = error; + if (error) + pull_data->async_error = &pull_data->cached_async_error; + else + pull_data->async_error = NULL; pull_data->main_context = g_main_context_ref_thread_default (); pull_data->flags = flags; @@ -2227,6 +2231,15 @@ ostree_repo_pull_with_options (OstreeRepo *self, ret = TRUE; out: + /* This is pretty ugly - we have two error locations, because we + * have a mix of synchronous and async code. Mixing them gets messy + * as we need to avoid overwriting errors. + */ + if (pull_data->cached_async_error && error && !*error) + g_propagate_error (error, pull_data->cached_async_error); + else + g_clear_error (&pull_data->cached_async_error); + ostree_repo_abort_transaction (pull_data->repo, cancellable, NULL); g_main_context_unref (pull_data->main_context); if (update_timeout) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index c1ce864e..8d2fa93a 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1681,6 +1681,7 @@ _ostree_preload_metadata_file (OstreeRepo *self, GError **error) { gboolean ret = FALSE; + if (is_metalink) { glnx_unref_object OstreeMetalink *metalink = NULL; @@ -1740,12 +1741,16 @@ repo_remote_fetch_summary (OstreeRepo *self, GError **error) { glnx_unref_object OstreeFetcher *fetcher = NULL; + g_autoptr(GMainContext) mainctx = NULL; gboolean ret = FALSE; SoupURI *base_uri = NULL; uint i; const char *filenames[] = {"summary", "summary.sig"}; GBytes **outputs[] = {out_summary, out_signatures}; + mainctx = g_main_context_new (); + g_main_context_push_thread_default (mainctx); + fetcher = _ostree_repo_remote_new_fetcher (self, name, error); if (fetcher == NULL) goto out; @@ -1787,9 +1792,10 @@ repo_remote_fetch_summary (OstreeRepo *self, ret = TRUE; out: + if (mainctx) + g_main_context_pop_thread_default (mainctx); if (base_uri != NULL) soup_uri_free (base_uri); - return ret; }