Merge lp:~thomas-voss/location-service/fix-1418033 into lp:location-service/trunk

[Thomas Voß]

Automatically clean up session store for dead clients.

Automatically clean up session store for dead clients.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:169
http://jenkins.qa.ubuntu.com/job/location-service-ci/348/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/20/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/20/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/20/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/location-service-ci/348/rebuild

review: Needs Fixing (continuous-integration)

[Ricardo Salveti (rsalveti)]

Looks good and it seems to work as expected.

review: Approve

=== modified file 'include/location_service/com/ubuntu/location/service/skeleton.h'
--- include/location_service/com/ubuntu/location/service/skeleton.h	2014-08-11 10:21:08 +0000
+++ include/location_service/com/ubuntu/location/service/skeleton.h	2015-02-13 13:20:19 +0000
@@ -25,6 +25,7 @@
 #include <core/dbus/dbus.h>
 #include <core/dbus/object.h>
 #include <core/dbus/property.h>
+#include <core/dbus/service_watcher.h>
 #include <core/dbus/skeleton.h>
 
 #include <core/dbus/interfaces/properties.h>
@@ -124,8 +125,12 @@
     // Returns true iff the session has been added to the store.
     bool add_to_session_store_for_path(
             const core::dbus::types::ObjectPath& path,
+            std::unique_ptr<core::dbus::ServiceWatcher> watcher,
             const session::Interface::Ptr& session);
 
+    // Removes the session with the given path from the session store.
+    void remove_from_session_store_for_path(const core::dbus::types::ObjectPath& path);
+
     // Called whenever the value of the respective property changes.
     void on_does_satellite_based_positioning_changed(bool value);
     // Called whenever the value of the respective property changes.
@@ -135,6 +140,8 @@
 
     // Stores the configuration passed in at creation time.
     Configuration configuration;
+    // We observe sessions if they have died and resigned from the bus.
+    core::dbus::DBus daemon;
     // The skeleton object representing com.ubuntu.location.service.Interface on the bus.
     core::dbus::Object::Ptr object;
     // We emit property changes manually.
@@ -161,8 +168,14 @@
     } connections;
     // Guards the session store.
     std::mutex guard;
+    // We track sessions and their respective watchers.
+    struct Element
+    {
+        std::unique_ptr<core::dbus::ServiceWatcher> watcher;
+        std::shared_ptr<session::Interface> session;
+    };
     // Keeps track of running sessions, keying them by their unique object path.
-    std::map<dbus::types::ObjectPath, std::shared_ptr<session::Interface>> session_store;
+    std::map<dbus::types::ObjectPath, Element> session_store;
 };
 }
 }

=== modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
--- src/location_service/com/ubuntu/location/service/daemon.cpp	2015-01-26 19:11:46 +0000
+++ src/location_service/com/ubuntu/location/service/daemon.cpp	2015-02-13 13:20:19 +0000
@@ -238,10 +238,7 @@
         }
     };
 
-    location::service::Implementation location_service
-    {
-        configuration
-    };
+    auto location_service = std::make_shared<location::service::Implementation>(configuration);
 
     std::thread t1{[&config](){config.incoming->run();}};
     std::thread t2{[&config](){config.incoming->run();}};

=== modified file 'src/location_service/com/ubuntu/location/service/skeleton.cpp'
--- src/location_service/com/ubuntu/location/service/skeleton.cpp	2014-08-14 20:31:09 +0000
+++ src/location_service/com/ubuntu/location/service/skeleton.cpp	2015-02-13 13:20:19 +0000
@@ -68,6 +68,7 @@
 culs::Skeleton::Skeleton(const culs::Skeleton::Configuration& configuration)
     : dbus::Skeleton<culs::Interface>(configuration.incoming),
       configuration(configuration),
+      daemon(configuration.incoming),
       object(access_service()->add_object_for_path(culs::Interface::path())),
       properties_changed(object->get_signal<core::dbus::interfaces::Properties::Signals::PropertiesChanged>()),
       properties
@@ -110,6 +111,7 @@
 
     auto sender = in->sender();
     auto reply = the_empty_reply();
+    auto thiz = shared_from_this();
 
     try
     {
@@ -147,12 +149,13 @@
             }
         };
 
-        culss::Interface::Ptr session
+        auto watcher = daemon.make_service_watcher(sender);
+        watcher->owner_changed().connect([thiz, path](const std::string&, const std::string&)
         {
-            new culss::Skeleton{config}
-        };
+            thiz->remove_from_session_store_for_path(path);
+        });
 
-        if (not add_to_session_store_for_path(path, session))
+        if (not add_to_session_store_for_path(path, std::move(watcher), culss::Interface::Ptr{new culss::Skeleton{config}}))
         {
             reply = dbus::Message::make_error(
                         in,
@@ -190,14 +193,21 @@
 
 bool culs::Skeleton::add_to_session_store_for_path(
         const core::dbus::types::ObjectPath& path,
+        std::unique_ptr<core::dbus::ServiceWatcher> watcher,
         const culss::Interface::Ptr& session)
 {
     std::lock_guard<std::mutex> lg(guard);
     bool inserted = false;
-    std::tie(std::ignore, inserted) = session_store.insert(std::make_pair(path, session));
+    std::tie(std::ignore, inserted) = session_store.insert(std::make_pair(path, Element{std::move(watcher), session}));
     return inserted;
 }
 
+void culs::Skeleton::remove_from_session_store_for_path(const core::dbus::types::ObjectPath& path)
+{
+    std::lock_guard<std::mutex> lg(guard);
+    session_store.erase(path);
+}
+
 void culs::Skeleton::on_does_satellite_based_positioning_changed(bool value)
 {
     std::map<std::string, core::dbus::types::Variant> dict

=== modified file 'tests/acceptance_tests.cpp'
--- tests/acceptance_tests.cpp	2014-11-14 11:26:45 +0000
+++ tests/acceptance_tests.cpp	2015-02-13 13:20:19 +0000
@@ -248,7 +248,7 @@
                 std::make_shared<NullReporter>()
             }
         };
-        cul::service::Implementation location_service{configuration};
+        auto location_service = std::make_shared<cul::service::Implementation>(configuration);
 
         sync_start.try_signal_ready_for(std::chrono::milliseconds{500});
 
@@ -371,7 +371,7 @@
             }
         };
         configuration.engine->configuration.engine_state = cul::Engine::Status::on;
-        cul::service::Implementation location_service{configuration};
+        auto location_service = std::make_shared<cul::service::Implementation>(configuration);
 
         sync_start.try_signal_ready_for(std::chrono::milliseconds{500});
 
@@ -452,7 +452,7 @@
             }
         };
         configuration.engine->configuration.satellite_based_positioning_state.set(cul::SatelliteBasedPositioningState::on);
-        cul::service::Implementation location_service{configuration};
+        auto location_service = std::make_shared<cul::service::Implementation>(configuration);
 
         sync_start.try_signal_ready_for(std::chrono::milliseconds{500});
 
@@ -531,7 +531,7 @@
                 std::make_shared<NullReporter>()
             }
         };
-        cul::service::Implementation location_service{configuration};
+        auto location_service = std::make_shared<cul::service::Implementation>(configuration);
 
         std::thread t1{[incoming](){incoming->run();}};
         std::thread t2{[outgoing](){outgoing->run();}};
@@ -619,7 +619,7 @@
                 std::make_shared<NullReporter>()
             }
         };
-        cul::service::Implementation location_service{configuration};
+        auto location_service = std::make_shared<cul::service::Implementation>(configuration);
 
         configuration.engine->updates.visible_space_vehicles.set(visible_space_vehicles);