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

[Thomas Voß]

Make sure that devices being added/removed by NetworkManager are handled correctly.

Make sure that devices being added/removed by NetworkManager are handled correctly.

[lp:~thomas-voss/location-service/fix-nm-race]

148. By Thomas Voß on 2014-11-07

Lock the cache when accessing it from signal handlers.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:148
http://jenkins.qa.ubuntu.com/job/location-service-ci/329/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/1
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/1/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/1

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

review: Approve (continuous-integration)

[Loïc Minier (lool)]

Looks good outside of not being sure about default connection handling

[lp:~thomas-voss/location-service/fix-nm-race]

149. By Thomas Voß on 2014-11-07

Do not recalculate connection characteristics for every new device. Instead, rely on the established way of listening for changes to the active connection obect.

150. By Thomas Voß on 2014-11-07

Bail out if the cache already knows about the device.

151. By Thomas Voß on 2014-11-07

Add missing ;.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:151
http://jenkins.qa.ubuntu.com/job/location-service-ci/331/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/3/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/3

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:149
http://jenkins.qa.ubuntu.com/job/location-service-ci/332/
Executed test runs:
    None: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/4/console
    None: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/4/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/4

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

review: Needs Fixing (continuous-integration)

[Loïc Minier (lool)]

(Latest version still breaks HERE)

[lp:~thomas-voss/location-service/fix-nm-race]

152. By Thomas Voß on 2014-11-10

Disconnect from properties changed signal on destructin, as mentioned in the TODO.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:152
http://jenkins.qa.ubuntu.com/job/location-service-ci/333/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/5
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/5/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/5

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

review: Needs Fixing (continuous-integration)

[lp:~thomas-voss/location-service/fix-nm-race]

153. By Thomas Voß on 2014-11-10

Make sure that locking is done correctly.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:153
http://jenkins.qa.ubuntu.com/job/location-service-ci/334/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/6/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/6

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

review: Approve (continuous-integration)

[Loïc Minier (lool)]

Hmm the locking is a bit spaghetti now; I can see why lock needs to be passed around to unlock before dispatching the updates from the on_foo() callbacks, but it does feel a bit weird to pass the lock down a callback and unlock it from there.

I guess the only way would be to queue the added/removed wifis/cells into a queue and trigger the updates once the lock is released, but that's pretty much as weird.

Anyway, tested, and seems to help with the otherwise failing test case.

Note that: a) I wasn't able to reproduce on mako + vivid, so couldn't confirm this fixed anything; b) it did help reproducible breakage on krillin from earlier revisions, but I did manage to break HERE on one boot.

So generally, this helps; I suspect there's still a race for a case.

I did witness tech=cell switching to tech=wifi on krillin when coming out of airplane mode, which was nice!

review: Approve

[Thomas Voß (thomas-voss)]

> Hmm the locking is a bit spaghetti now; I can see why lock needs to be passed
> around to unlock before dispatching the updates from the on_foo() callbacks,
> but it does feel a bit weird to pass the lock down a callback and unlock it
> from there.
>

The one case is technically not a callback but a functor passed for evaluation. In all other cases, the lock is created within the callback and handed down to the actual function. I do prefer this API style to naming functions *_locked(...), which is tedious and error prone.

> I guess the only way would be to queue the added/removed wifis/cells into a
> queue and trigger the updates once the lock is released, but that's pretty
> much as weird.
>
> Anyway, tested, and seems to help with the otherwise failing test case.
>
> Note that: a) I wasn't able to reproduce on mako + vivid, so couldn't confirm
> this fixed anything; b) it did help reproducible breakage on krillin from
> earlier revisions, but I did manage to break HERE on one boot.
>

> So generally, this helps; I suspect there's still a race for a case.
>
> I did witness tech=cell switching to tech=wifi on krillin when coming out of
> airplane mode, which was nice!

Thanks.

=== modified file 'src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp'
--- src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp	2014-10-13 15:20:56 +0000
+++ src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp	2014-11-10 20:13:24 +0000
@@ -172,8 +172,7 @@
 
 detail::CachedRadioCell::~CachedRadioCell()
 {
-    // TODO(tvoss): Reenable this once we know why the modem cache gets flushed.
-    // modem.network_registration.signals.property_changed->disconnect(connections.network_registration_properties_changed);
+    modem.network_registration.signals.property_changed->disconnect(connections.network_registration_properties_changed);
 }
 
 const core::Property<bool>& detail::CachedRadioCell::is_roaming() const

=== modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp'
--- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp	2014-10-13 13:54:05 +0000
+++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp	2014-11-10 20:13:24 +0000
@@ -423,56 +423,8 @@
 
     network_manager->for_each_device([this](const core::dbus::types::ObjectPath& device_path)
     {
-        auto device = network_manager->device_for_path(device_path);
-
-        if (device.type() == xdg::NetworkManager::Device::Type::wifi)
-        {
-            // Make the device known to the cache.
-            std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device>::iterator it;
-            std::tie(it, std::ignore) = cached.wireless_devices.insert(std::make_pair(device_path, device));
-
-            // Iterate over all currently known wifis
-            it->second.for_each_access_point([this, device_path](const core::dbus::types::ObjectPath& path)
-            {
-                try
-                {
-                    on_access_point_added(path, device_path);
-                }
-                catch (const std::exception& e)
-                {
-                    VLOG(1) << "Error while creating ap/wifi: " << e.what();
-                }
-            });
-
-            it->second.signals.scan_done->connect([this]()
-            {
-                signals.wireless_network_scan_finished();
-            });
-
-            it->second.signals.ap_added->connect([this, device_path](const core::dbus::types::ObjectPath& path)
-            {
-                try
-                {
-                    on_access_point_added(path, device_path);
-                }
-                catch (const std::exception& e)
-                {
-                    VLOG(1) << "Error while creating ap/wifi: " << e.what();
-                }
-            });
-
-            it->second.signals.ap_removed->connect([this](const core::dbus::types::ObjectPath& path)
-            {
-                try
-                {
-                    on_access_point_removed(path);
-                }
-                catch (const std::exception& e)
-                {
-                    VLOG(1) << "Error while removing ap/wifi: " << e.what();
-                }
-            });
-        }
+        std::unique_lock<std::mutex> ul{cached.guard};
+        on_device_added(device_path, ul);
     });
 
     // Query the initial connectivity state
@@ -482,6 +434,28 @@
                 characteristics_for_connection(
                     network_manager->properties.primary_connection->get()));
 
+    network_manager->signals.device_added->connect([this](const core::dbus::types::ObjectPath& path)
+    {
+        // We dispatch determining the connection characteristics to unblock
+        // the bus here.
+        dispatcher.service.post([this, path]()
+        {
+            std::unique_lock<std::mutex> ul{cached.guard};
+            on_device_added(path, ul);
+        });
+    });
+
+    network_manager->signals.device_removed->connect([this](const core::dbus::types::ObjectPath& path)
+    {
+        // We dispatch determining the connection characteristics to unblock
+        // the bus here.
+        dispatcher.service.post([this, path]()
+        {
+            std::unique_lock<std::mutex> ul{cached.guard};
+            on_device_removed(path, ul);
+        });
+    });
+
     // And we wire up to property changes here
     network_manager->signals.properties_changed->connect([this](const std::map<std::string, core::dbus::types::Variant>& dict)
     {
@@ -546,12 +520,79 @@
     });
 }
 
+void connectivity::OfonoNmConnectivityManager::Private::on_device_added(
+        const core::dbus::types::ObjectPath& device_path,
+        std::unique_lock<std::mutex>& ul)
+{
+    if (cached.wireless_devices.count(device_path) > 0)
+        return;
+
+    auto device = network_manager->device_for_path(device_path);
+
+    if (device.type() == xdg::NetworkManager::Device::Type::wifi)
+    {
+        // Make the device known to the cache.
+        std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device>::iterator it;
+        std::tie(it, std::ignore) = cached.wireless_devices.insert(std::make_pair(device_path, device));
+
+        // Iterate over all currently known wifis
+        it->second.for_each_access_point([this, device_path, &ul](const core::dbus::types::ObjectPath& path)
+        {
+            try
+            {
+                on_access_point_added(path, device_path, ul);
+            }
+            catch (const std::exception& e)
+            {
+                VLOG(1) << "Error while creating ap/wifi: " << e.what();
+            }
+        });
+
+        it->second.signals.scan_done->connect([this]()
+        {
+            signals.wireless_network_scan_finished();
+        });
+
+        it->second.signals.ap_added->connect([this, device_path](const core::dbus::types::ObjectPath& path)
+        {
+            try
+            {
+                std::unique_lock<std::mutex> ul{cached.guard};
+                on_access_point_added(path, device_path, ul);
+            }
+            catch (const std::exception& e)
+            {
+                VLOG(1) << "Error while creating ap/wifi: " << e.what();
+            }
+        });
+
+        it->second.signals.ap_removed->connect([this](const core::dbus::types::ObjectPath& path)
+        {
+            try
+            {
+                std::unique_lock<std::mutex> ul{cached.guard};
+                on_access_point_removed(path, ul);
+            }
+            catch (const std::exception& e)
+            {
+                VLOG(1) << "Error while removing ap/wifi: " << e.what();
+            }
+        });
+    }
+}
+
+void connectivity::OfonoNmConnectivityManager::Private::on_device_removed(
+        const core::dbus::types::ObjectPath& path,
+        std::unique_lock<std::mutex>&)
+{
+    cached.wireless_devices.erase(path);
+}
+
 void connectivity::OfonoNmConnectivityManager::Private::on_access_point_added(
         const core::dbus::types::ObjectPath& ap_path,
-        const core::dbus::types::ObjectPath& device_path)
+        const core::dbus::types::ObjectPath& device_path,
+        std::unique_lock<std::mutex>& ul)
 {
-    std::unique_lock<std::mutex> ul(cached.guard);
-
     // Let's see if we have a device known for the path. We return early
     // if we do not know about the device.
     auto itd = cached.wireless_devices.find(device_path);
@@ -571,10 +612,10 @@
     ul.unlock(); signals.wireless_network_added(wifi);
 }
 
-void connectivity::OfonoNmConnectivityManager::Private::on_access_point_removed(const core::dbus::types::ObjectPath& ap_path)
+void connectivity::OfonoNmConnectivityManager::Private::on_access_point_removed(
+        const core::dbus::types::ObjectPath& ap_path,
+        std::unique_lock<std::mutex>& ul)
 {
-    std::unique_lock<std::mutex> ul(cached.guard);
-
     // Let's see if we know about the wifi. We return early if not.
     auto itw = cached.wifis.find(ap_path);
     if (itw == cached.wifis.end())

=== modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h'
--- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h	2014-10-08 14:17:54 +0000
+++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h	2014-11-10 20:13:24 +0000
@@ -94,8 +94,10 @@
 
         // All network stack specific functionality goes here.
         void setup_network_stack_access();
-        void on_access_point_added(const core::dbus::types::ObjectPath& ap_path, const core::dbus::types::ObjectPath& device_path);
-        void on_access_point_removed(const core::dbus::types::ObjectPath& ap_path);
+        void on_device_added(const core::dbus::types::ObjectPath& device_path, std::unique_lock<std::mutex>& ul);
+        void on_device_removed(const core::dbus::types::ObjectPath& device_path, std::unique_lock<std::mutex>& ul);
+        void on_access_point_added(const core::dbus::types::ObjectPath& ap_path, const core::dbus::types::ObjectPath& device_path, std::unique_lock<std::mutex>& ul);
+        void on_access_point_removed(const core::dbus::types::ObjectPath& ap_path, std::unique_lock<std::mutex>& ul);
         com::ubuntu::location::connectivity::Characteristics characteristics_for_connection(const core::dbus::types::ObjectPath& path);        
 
         core::dbus::Bus::Ptr bus;