Merge lp:~thomas-voss/location-service/wait-for-service into lp:location-service

[Thomas Voß]

Postpone creation of dbus stubs until service becomes available.

This is the first step in making the overall setup of out-of-process providers
and clients more resilient against the service going away or starting after a provider
has been started.

Postpone creation of dbus stubs until service becomes available.

This is the first step in making the overall setup of out-of-process providers
and clients more resilient against the service going away or starting after a provider
has been started.

[Simon Fels (morphis)]

LGTM

review: Approve

=== modified file 'src/location/dbus/stub/service.cpp'
--- src/location/dbus/stub/service.cpp	2017-03-07 10:01:34 +0000
+++ src/location/dbus/stub/service.cpp	2017-03-09 09:16:13 +0000
@@ -45,52 +45,64 @@
     g_bus_get(static_cast<GBusType>(bus), nullptr, on_bus_acquired, new BusAcquisitionContext{std::move(callback)});
 }
 
-location::dbus::stub::Service::Service(const glib::SharedObject<GDBusConnection>& connection,
-                                       const glib::SharedObject<ComUbuntuLocationService>& proxy)
-    : connection{connection},
-      proxy{proxy},
-      state_{boost::lexical_cast<Service::State>(com_ubuntu_location_service_get_state(proxy.get()))},
-      does_satellite_based_positioning_{com_ubuntu_location_service_get_does_satellite_based_positioning(proxy.get())},
-      does_report_cell_and_wifi_ids_{com_ubuntu_location_service_get_does_report_cell_and_wifi_ids(proxy.get())},
-      is_online_{com_ubuntu_location_service_get_is_online(proxy.get())}
-{
-}
-
-std::shared_ptr<location::dbus::stub::Service> location::dbus::stub::Service::finalize_construction()
+location::dbus::stub::Service::Service()
+{
+}
+
+void location::dbus::stub::Service::deinit()
+{
+    for (auto id : signal_handler_ids)
+        g_signal_handler_disconnect(proxy_.get(), id);
+
+    signal_handler_ids.clear();
+}
+
+std::shared_ptr<location::dbus::stub::Service> location::dbus::stub::Service::init(
+        const glib::SharedObject<GDBusConnection>& connection, const glib::SharedObject<ComUbuntuLocationService>& service)
 {
     auto sp = shared_from_this();
     std::weak_ptr<Service> wp{sp};
 
+    deinit();
+
+    connection_ = connection;
+    proxy_ = service;
+
+    state_ = boost::lexical_cast<Service::State>(com_ubuntu_location_service_get_state(proxy_.get()));
+    does_satellite_based_positioning_ = com_ubuntu_location_service_get_does_satellite_based_positioning(proxy_.get());
+    does_report_cell_and_wifi_ids_ = com_ubuntu_location_service_get_does_report_cell_and_wifi_ids(proxy_.get());
+    is_online_ = com_ubuntu_location_service_get_is_online(proxy_.get());
+
     does_satellite_based_positioning_.changed().connect([this, wp](bool value)
     {
         if (auto sp = wp.lock())
-            com_ubuntu_location_service_set_does_satellite_based_positioning(Service::proxy.get(), value);
+            com_ubuntu_location_service_set_does_satellite_based_positioning(proxy_.get(), value);
     });
 
     does_report_cell_and_wifi_ids_.changed().connect([this, wp](bool value)
     {
         if (auto sp = wp.lock())
-            com_ubuntu_location_service_set_does_report_cell_and_wifi_ids(Service::proxy.get(), value);
+            com_ubuntu_location_service_set_does_report_cell_and_wifi_ids(proxy_.get(), value);
     });
 
     is_online_.changed().connect([this, wp](bool value)
     {
         if (auto sp = wp.lock())
-            com_ubuntu_location_service_set_is_online(Service::proxy.get(), value);
+            com_ubuntu_location_service_set_is_online(proxy_.get(), value);
     });
 
     signal_handler_ids.insert(
-                g_signal_connect_data(Service::proxy.get(), "notify::does-satellite-based-positioning",
+                g_signal_connect_data(proxy_.get(), "notify::does-satellite-based-positioning",
                                       G_CALLBACK(on_does_satellite_based_positioning_changed),
                                       new Holder{wp}, Holder::closure_notify, GConnectFlags(0)));
 
     signal_handler_ids.insert(
-                g_signal_connect_data(Service::proxy.get(), "notify::does-report-cell-and-wifi-ids",
+                g_signal_connect_data(proxy_.get(), "notify::does-report-cell-and-wifi-ids",
                                       G_CALLBACK(on_does_report_cell_and_wifi_ids_changed),
                                       new Holder{wp}, Holder::closure_notify, GConnectFlags(0)));
 
     signal_handler_ids.insert(
-                g_signal_connect_data(Service::proxy.get(), "notify::is-online",
+                g_signal_connect_data(proxy_.get(), "notify::is-online",
                                       G_CALLBACK(on_is_online_changed),
                                       new Holder{wp}, Holder::closure_notify, GConnectFlags(0)));
 
@@ -99,8 +111,7 @@
 
 location::dbus::stub::Service::~Service()
 {
-    for (auto id : signal_handler_ids)
-        g_signal_handler_disconnect(proxy.get(), id);
+    deinit();
 }
 
 void location::dbus::stub::Service::create_session_for_criteria(const Criteria& criteria, const std::function<void(const Session::Ptr&)>& cb)
@@ -109,7 +120,7 @@
     std::weak_ptr<Service> wp{sp};
 
     com_ubuntu_location_service_call_create_session_for_criteria(
-                proxy.get(), dbus::encode(criteria), nullptr, on_session_ready, new Service::SessionCreationContext{std::move(cb), wp});
+                proxy_.get(), dbus::encode(criteria), nullptr, on_session_ready, new Service::SessionCreationContext{std::move(cb), wp});
 }
 
 void location::dbus::stub::Service::add_provider(const Provider::Ptr& provider)
@@ -118,10 +129,10 @@
     std::weak_ptr<Service> wp{sp};
 
     auto path = "/providers/" + std::to_string(provider_counter++);
-    auto skeleton = location::providers::remote::Provider::Skeleton::create(connection, path, provider);
+    auto skeleton = location::providers::remote::Provider::Skeleton::create(connection_, path, provider);
 
     com_ubuntu_location_service_call_add_provider(
-                proxy.get(), path.c_str(), nullptr, Service::on_provider_added, new Service::ProviderAdditionContext{skeleton, wp});
+                proxy_.get(), path.c_str(), nullptr, Service::on_provider_added, new Service::ProviderAdditionContext{skeleton, wp});
 }
 
 const core::Property<location::Service::State>& location::dbus::stub::Service::state() const
@@ -152,7 +163,6 @@
 void location::dbus::stub::Service::on_proxy_ready(GObject* source, GAsyncResult* res, gpointer user_data)
 {
     LOCATION_DBUS_TRACE_STATIC_TRAMPOLIN;
-
     boost::ignore_unused(source);
 
     if (auto context = static_cast<Service::ProxyCreationContext*>(user_data))
@@ -166,10 +176,9 @@
         }
         else
         {
-            location::dbus::stub::Service::Ptr sp{
-                new location::dbus::stub::Service{
-                    context->connection, location::glib::make_shared_object(stub)}};
-            context->cb(make_result(sp->finalize_construction()));
+            context->cb(make_result(
+                            context->instance->init(
+                                context->connection, location::glib::make_shared_object(stub))));
         }
 
         delete context;
@@ -179,7 +188,6 @@
 void location::dbus::stub::Service::on_bus_acquired(GObject* source, GAsyncResult* res, gpointer user_data)
 {
     LOCATION_DBUS_TRACE_STATIC_TRAMPOLIN;
-
     boost::ignore_unused(source);
 
     if (auto context = static_cast<Service::BusAcquisitionContext*>(user_data))
@@ -193,19 +201,47 @@
         }
         else
         {
-            com_ubuntu_location_service_proxy_new(
-                        bus, G_DBUS_PROXY_FLAGS_NONE, location::dbus::Service::name(), location::dbus::Service::path(),
-                        nullptr, on_proxy_ready, new Service::ProxyCreationContext{location::glib::make_shared_object(bus), context->cb});
+            auto name_appeared_for_creation_context =
+                    new NameAppearedForCreationContext{0, std::move(context->cb)};
+
+            name_appeared_for_creation_context->watch_id =
+                    g_bus_watch_name_on_connection(
+                        bus, location::dbus::Service::name(), G_BUS_NAME_WATCHER_FLAGS_NONE,
+                        on_name_appeared_for_creation, nullptr,
+                        name_appeared_for_creation_context, nullptr);
         }
 
         delete context;
     }
 }
 
+void location::dbus::stub::Service::on_name_appeared_for_creation(
+        GDBusConnection* bus, const gchar* name, const gchar* name_owner, gpointer user_data)
+{
+    LOCATION_DBUS_TRACE_STATIC_TRAMPOLIN;
+    boost::ignore_unused(name, name_owner);
+
+    if (auto context = static_cast<NameAppearedForCreationContext*>(user_data))
+    {
+        com_ubuntu_location_service_proxy_new(
+                    bus, G_DBUS_PROXY_FLAGS_NONE, location::dbus::Service::name(), location::dbus::Service::path(),
+                    nullptr, on_proxy_ready, new Service::ProxyCreationContext
+                    {
+                        location::glib::make_shared_object(bus),
+                        Ptr{new Service{}},
+                        context->cb
+                    });
+
+        // Make sure that we only react to name appeared events once in this code path.
+        g_bus_unwatch_name(context->watch_id);
+        // Clean up our context.
+        delete context;
+    }
+}
+
 void location::dbus::stub::Service::on_provider_added(GObject *source, GAsyncResult *res, gpointer user_data)
 {
     LOCATION_DBUS_TRACE_STATIC_TRAMPOLIN;
-
     boost::ignore_unused(source);
 
     if (auto context = static_cast<Service::ProviderAdditionContext*>(user_data))
@@ -214,7 +250,7 @@
         {
             GError* error{nullptr};
             if (com_ubuntu_location_service_call_add_provider_finish(
-                        sp->proxy.get(), res, &error))
+                        sp->proxy_.get(), res, &error))
             {
                 sp->providers.insert(context->provider);
             }
@@ -239,12 +275,12 @@
         {
             GError* error{nullptr}; char* path{nullptr};
             com_ubuntu_location_service_call_create_session_for_criteria_finish(
-                        sp->proxy.get(), &path, res, &error);
+                        sp->proxy_.get(), &path, res, &error);
 
             if (!error)
             {
                 auto cb = context->cb;
-                stub::Session::create(sp->connection, path, [cb](const Result<stub::Session::Ptr>& result)
+                stub::Session::create(sp->connection_, path, [cb](const Result<stub::Session::Ptr>& result)
                 {
                     if (result)
                         cb(result.value());
@@ -271,7 +307,7 @@
         {
             sp->does_satellite_based_positioning() =
                     com_ubuntu_location_service_get_does_satellite_based_positioning(
-                        sp->proxy.get());
+                        sp->proxy_.get());
         }
     }
 }
@@ -287,7 +323,7 @@
         {
             sp->does_report_cell_and_wifi_ids() =
                     com_ubuntu_location_service_get_does_report_cell_and_wifi_ids(
-                        sp->proxy.get());
+                        sp->proxy_.get());
         }
     }
 }
@@ -303,7 +339,7 @@
         {
             sp->is_online() =
                     com_ubuntu_location_service_get_is_online(
-                        sp->proxy.get());
+                        sp->proxy_.get());
         }
     }
 }

=== modified file 'src/location/dbus/stub/service.h'
--- src/location/dbus/stub/service.h	2017-03-07 10:01:34 +0000
+++ src/location/dbus/stub/service.h	2017-03-09 09:16:13 +0000
@@ -59,9 +59,18 @@
     };
     static void on_bus_acquired(GObject* source, GAsyncResult* res, gpointer user_data);
 
+    struct NameAppearedForCreationContext
+    {
+        guint watch_id;
+        std::function<void(const Result<Service::Ptr>&)> cb;
+    };
+    static void on_name_appeared_for_creation(
+            GDBusConnection* connection_, const gchar* name, const gchar* name_owner, gpointer user_data);
+
     struct ProxyCreationContext
     {
         glib::SharedObject<GDBusConnection> connection;
+        Service::Ptr instance;
         std::function<void(const Result<Service::Ptr>&)> cb;
     };
     static void on_proxy_ready(GObject* source, GAsyncResult* res, gpointer user_data);
@@ -84,13 +93,14 @@
     static void on_does_report_cell_and_wifi_ids_changed(GObject* object, GParamSpec* spec, gpointer user_data);
     static void on_is_online_changed(GObject* object, GParamSpec* spec, gpointer user_data);
 
-    Service(const glib::SharedObject<GDBusConnection>& connection,
-            const glib::SharedObject<ComUbuntuLocationService>& service);
-
-    std::shared_ptr<Service> finalize_construction();
-
-    glib::SharedObject<GDBusConnection> connection;
-    glib::SharedObject<ComUbuntuLocationService> proxy;
+    Service();
+
+    void deinit();
+    std::shared_ptr<Service> init(const glib::SharedObject<GDBusConnection>& connection_,
+                                  const glib::SharedObject<ComUbuntuLocationService>& service);
+
+    glib::SharedObject<GDBusConnection> connection_;
+    glib::SharedObject<ComUbuntuLocationService> proxy_;
     std::set<ulong> signal_handler_ids;
 
     std::uint64_t provider_counter{0};

=== modified file 'src/location/providers/remote/provider.h'
--- src/location/providers/remote/provider.h	2017-03-04 21:43:23 +0000
+++ src/location/providers/remote/provider.h	2017-03-09 09:16:13 +0000
@@ -43,6 +43,8 @@
     class Stub : public location::Provider, public std::enable_shared_from_this<Stub>
     {
     public:
+        using Ptr = std::shared_ptr<Stub>;
+
         // Asynchronously tries to create a new Stub instance, setting up proxy access to the remote
         // end. Reports errors via 'cb'.
         static void create(const glib::SharedObject<GDBusConnection>& connection,
@@ -91,6 +93,8 @@
     class Skeleton : public location::Provider, public std::enable_shared_from_this<Skeleton>
     {
     public:
+        using Ptr = std::shared_ptr<Skeleton>;
+
         // Asynchronously tries to create a new Skeleton instance, setting up
         // proxy access to the remote end. Reports errors via cb.
         static Provider::Ptr create(