Merge lp:~alan-griffiths/mir/fix-1661151 into lp:mir

[Alan Griffiths]

DefaultInputDeviceHub should not hold a lock while notifying observers
(fixes deadlock LP: #1661151)

[Alan Griffiths (alan-griffiths)]

I'm seeing an intermittent test failure locally, but not spotted the connection.

Submitted for CI review to see what happens there...

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

If its the input device thingy, ive been hitting that as well often.

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:4056
https://mir-jenkins.ubuntu.com/job/mir-ci/3068/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4107/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4194
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4184
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4184
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4184
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4134
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4134/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4134
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4134/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4134
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4134/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4134
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4134/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4134
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4134/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4134/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3068/rebuild

review: Needs Fixing (continuous-integration)

[Brandon Schaefer (brandontschaefer)]

lp.1646558 ^^

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:4056
https://mir-jenkins.ubuntu.com/job/mir-ci/3069/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4108
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4195
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4185
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4185
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4185
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4135
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4135/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4135
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4135/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4135
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4135/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4135
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4135/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4135
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4135/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4135
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4135/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3069/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Verified the bug is fixed.

I'm not sure about the remaining callbacks there that still hold (different) locks, but seeing the bug is fixed is more important.

review: Approve (manual testing)

[Alan Griffiths (alan-griffiths)]

The failure I've been seeing locally is lp:1661187 - and (seeing it hasn't occurred in the above CI runs) I've convinced myself that I've done nothing to make that worse.

review: Abstain

[Andreas Pokorny (andreas-pokorny)]

Yes lp:1661187 is a different problem

review: Approve

=== modified file 'src/server/input/default_input_device_hub.cpp'
--- src/server/input/default_input_device_hub.cpp	2017-02-15 07:38:33 +0000
+++ src/server/input/default_input_device_hub.cpp	2017-02-27 18:36:25 +0000
@@ -226,8 +226,7 @@
         this,
         [observer,this]
         {
-            std::unique_lock<std::mutex> lock(observer_guard);
-            observers.push_back(observer);
+            observers.add(observer);
             for (auto const& item : handles)
             {
                 observer->device_added(item);
@@ -239,7 +238,7 @@
 
 void mi::DefaultInputDeviceHub::for_each_input_device(std::function<void(Device const&)> const& callback)
 {
-    std::unique_lock<std::mutex> lock(observer_guard);
+    std::unique_lock<std::mutex> lock(handles_guard);
     for (auto const item : handles)
         callback(*item);
 }
@@ -252,7 +251,7 @@
     }
 
     {
-        std::unique_lock<std::mutex> lock(observer_guard);
+        std::unique_lock<std::mutex> lock(handles_guard);
         for (auto const& item : handles)
             callback(*item);
     }
@@ -267,23 +266,24 @@
     observer_queue->enqueue(this,
                             [observer, this]
                             {
-                                std::unique_lock<std::mutex> lock(observer_guard);
-                                observers.erase(remove(begin(observers), end(observers), observer), end(observers));
+                                observers.remove(observer);
                             });
 }
 
 void mi::DefaultInputDeviceHub::add_device_handle(std::shared_ptr<DefaultDevice> const& handle)
 {
-    std::unique_lock<std::mutex> lock(observer_guard);
-    handles.push_back(handle);
-
-    for (auto const& observer : observers)
     {
-        observer->device_added(handles.back());
-        observer->changes_complete();
+        std::unique_lock<std::mutex> lock(handles_guard);
+        handles.push_back(handle);
     }
 
-    if (!ready && handles.size())
+    observers.for_each([&handle](std::shared_ptr<InputDeviceObserver> const& observer)
+        {
+            observer->device_added(handle);
+            observer->changes_complete();
+        });
+
+    if (!ready)
     {
         server_status_listener->ready_for_user_input();
         ready = true;
@@ -292,28 +292,41 @@
 
 void mi::DefaultInputDeviceHub::remove_device_handle(MirInputDeviceId id)
 {
-    std::unique_lock<std::mutex> lock(observer_guard);
-    auto handle_it = remove_if(
-        begin(handles),
-        end(handles),
-        [this,&id](auto const& handle)
-        {
-            if (handle->id() != id)
-                return false;
-            for (auto const& observer : observers)
+    decltype(handles) removed_devices;
+    decltype(handles.size()) no_of_devices{};
+
+    {
+        std::unique_lock<std::mutex> lock(handles_guard);
+        auto handle_it = remove_if(
+            begin(handles),
+            end(handles),
+            [&](auto const& handle)
+                {
+                if (handle->id() != id)
+                    return false;
+                removed_devices.push_back(handle);
+                return true;
+                });
+
+        if (handle_it == end(handles))
+            return;
+
+        handles.erase(handle_it, end(handles));
+        no_of_devices = handles.size();
+    }
+
+    for (auto const& handle : removed_devices)
+    {
+        observers.for_each([&](std::shared_ptr<InputDeviceObserver> const& observer)
             {
                 observer->device_removed(handle);
                 observer->changes_complete();
-            }
-            return true;
-        });
-
-    if (handle_it == end(handles))
-        return;
-
-    handles.erase(handle_it, end(handles));
-
-    if (ready && 0 == handles.size())
+            });
+    }
+
+    removed_devices.clear();
+
+    if (ready && 0 == no_of_devices)
     {
         ready = false;
         server_status_listener->stop_receiving_input();
@@ -335,17 +348,22 @@
 
     if (!more_changes_in_progress)
     {
-        std::unique_lock<std::mutex> lock(observer_guard);
-        auto dev_it = find_if(begin(handles), end(handles), [dev](auto const& ptr){return ptr.get() == dev;});
-
-        if (dev_it==end(handles))
-            return;
-
-        for (auto const& observer : observers)
+        std::shared_ptr<Device> device;
         {
-            observer->device_changed(*dev_it);
-            observer->changes_complete();
+            std::unique_lock<std::mutex> lock(handles_guard);
+            auto dev_it = find_if(begin(handles), end(handles), [dev](auto const& ptr){return ptr.get() == dev;});
+
+            if (dev_it==end(handles))
+                return;
+
+            device = *dev_it;
         }
+
+        observers.for_each([&](std::shared_ptr<InputDeviceObserver> const& observer)
+            {
+                observer->device_changed(device);
+                observer->changes_complete();
+            });
     }
 }
 
@@ -361,13 +379,12 @@
         }
     }
     {
-        std::unique_lock<std::mutex> lock(observer_guard);
-        for (auto const& observer : observers)
-        {
-            for (auto const& dev : devices_to_notify)
-                observer->device_changed(dev);
-            observer->changes_complete();
-        }
+        observers.for_each([&](std::shared_ptr<InputDeviceObserver> const& observer)
+            {
+                for (auto const& dev : devices_to_notify)
+                    observer->device_changed(dev);
+                observer->changes_complete();
+            });
     }
 }
 

=== modified file 'src/server/input/default_input_device_hub.h'
--- src/server/input/default_input_device_hub.h	2017-02-15 07:38:33 +0000
+++ src/server/input/default_input_device_hub.h	2017-02-27 18:36:25 +0000
@@ -27,6 +27,7 @@
 #include "mir/input/input_device_hub.h"
 #include "mir/input/input_device_info.h"
 #include "mir/input/mir_input_config.h"
+#include "mir/thread_safe_list.h"
 
 #include "mir_toolkit/event.h"
 
@@ -87,7 +88,7 @@
     MirInputDeviceId create_new_device_id();
     std::shared_ptr<Seat> const seat;
     std::shared_ptr<dispatch::MultiplexingDispatchable> const input_dispatchable;
-    std::mutex observer_guard;
+    std::mutex handles_guard;
     std::shared_ptr<ServerActionQueue> const observer_queue;
     std::shared_ptr<dispatch::ActionQueue> const device_queue;
     std::shared_ptr<cookie::Authority> const cookie_authority;
@@ -124,7 +125,7 @@
     std::vector<std::shared_ptr<Device>> handles;
     MirInputConfig config;
     std::vector<std::unique_ptr<RegisteredDevice>> devices;
-    std::vector<std::shared_ptr<InputDeviceObserver>> observers;
+    ThreadSafeList<std::shared_ptr<InputDeviceObserver>> observers;
     std::mutex changed_devices_guard;
     std::unique_ptr<std::vector<std::shared_ptr<Device>>> changed_devices;