Mir

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

Proposed by Alan Griffiths on 2017-02-27
Status: Merged
Approved by: Alan Griffiths on 2017-02-28
Approved revision: 4056
Merged at revision: 4057
Proposed branch: lp:~alan-griffiths/mir/fix-1661151
Merge into: lp:mir
Diff against target: 207 lines (+68/-50)
2 files modified
src/server/input/default_input_device_hub.cpp (+65/-48)
src/server/input/default_input_device_hub.h (+3/-2)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1661151
Reviewer Review Type Date Requested Status
Andreas Pokorny (community) Approve on 2017-02-28
Alan Griffiths Abstain on 2017-02-28
Daniel van Vugt manual testing Approve on 2017-02-28
Mir CI Bot continuous-integration Approve on 2017-02-27
Review via email: mp+318393@code.launchpad.net

Commit message

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

To post a comment you must log in.
Alan Griffiths (alan-griffiths) wrote :

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) wrote :

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

Mir CI Bot (mir-ci-bot) wrote :

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) wrote :

lp.1646558 ^^

Mir CI Bot (mir-ci-bot) wrote :

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) wrote :

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) wrote :

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) wrote :

Yes lp:1661187 is a different problem

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/input/default_input_device_hub.cpp'
2--- src/server/input/default_input_device_hub.cpp 2017-02-15 07:38:33 +0000
3+++ src/server/input/default_input_device_hub.cpp 2017-02-27 18:36:25 +0000
4@@ -226,8 +226,7 @@
5 this,
6 [observer,this]
7 {
8- std::unique_lock<std::mutex> lock(observer_guard);
9- observers.push_back(observer);
10+ observers.add(observer);
11 for (auto const& item : handles)
12 {
13 observer->device_added(item);
14@@ -239,7 +238,7 @@
15
16 void mi::DefaultInputDeviceHub::for_each_input_device(std::function<void(Device const&)> const& callback)
17 {
18- std::unique_lock<std::mutex> lock(observer_guard);
19+ std::unique_lock<std::mutex> lock(handles_guard);
20 for (auto const item : handles)
21 callback(*item);
22 }
23@@ -252,7 +251,7 @@
24 }
25
26 {
27- std::unique_lock<std::mutex> lock(observer_guard);
28+ std::unique_lock<std::mutex> lock(handles_guard);
29 for (auto const& item : handles)
30 callback(*item);
31 }
32@@ -267,23 +266,24 @@
33 observer_queue->enqueue(this,
34 [observer, this]
35 {
36- std::unique_lock<std::mutex> lock(observer_guard);
37- observers.erase(remove(begin(observers), end(observers), observer), end(observers));
38+ observers.remove(observer);
39 });
40 }
41
42 void mi::DefaultInputDeviceHub::add_device_handle(std::shared_ptr<DefaultDevice> const& handle)
43 {
44- std::unique_lock<std::mutex> lock(observer_guard);
45- handles.push_back(handle);
46-
47- for (auto const& observer : observers)
48 {
49- observer->device_added(handles.back());
50- observer->changes_complete();
51+ std::unique_lock<std::mutex> lock(handles_guard);
52+ handles.push_back(handle);
53 }
54
55- if (!ready && handles.size())
56+ observers.for_each([&handle](std::shared_ptr<InputDeviceObserver> const& observer)
57+ {
58+ observer->device_added(handle);
59+ observer->changes_complete();
60+ });
61+
62+ if (!ready)
63 {
64 server_status_listener->ready_for_user_input();
65 ready = true;
66@@ -292,28 +292,41 @@
67
68 void mi::DefaultInputDeviceHub::remove_device_handle(MirInputDeviceId id)
69 {
70- std::unique_lock<std::mutex> lock(observer_guard);
71- auto handle_it = remove_if(
72- begin(handles),
73- end(handles),
74- [this,&id](auto const& handle)
75- {
76- if (handle->id() != id)
77- return false;
78- for (auto const& observer : observers)
79+ decltype(handles) removed_devices;
80+ decltype(handles.size()) no_of_devices{};
81+
82+ {
83+ std::unique_lock<std::mutex> lock(handles_guard);
84+ auto handle_it = remove_if(
85+ begin(handles),
86+ end(handles),
87+ [&](auto const& handle)
88+ {
89+ if (handle->id() != id)
90+ return false;
91+ removed_devices.push_back(handle);
92+ return true;
93+ });
94+
95+ if (handle_it == end(handles))
96+ return;
97+
98+ handles.erase(handle_it, end(handles));
99+ no_of_devices = handles.size();
100+ }
101+
102+ for (auto const& handle : removed_devices)
103+ {
104+ observers.for_each([&](std::shared_ptr<InputDeviceObserver> const& observer)
105 {
106 observer->device_removed(handle);
107 observer->changes_complete();
108- }
109- return true;
110- });
111-
112- if (handle_it == end(handles))
113- return;
114-
115- handles.erase(handle_it, end(handles));
116-
117- if (ready && 0 == handles.size())
118+ });
119+ }
120+
121+ removed_devices.clear();
122+
123+ if (ready && 0 == no_of_devices)
124 {
125 ready = false;
126 server_status_listener->stop_receiving_input();
127@@ -335,17 +348,22 @@
128
129 if (!more_changes_in_progress)
130 {
131- std::unique_lock<std::mutex> lock(observer_guard);
132- auto dev_it = find_if(begin(handles), end(handles), [dev](auto const& ptr){return ptr.get() == dev;});
133-
134- if (dev_it==end(handles))
135- return;
136-
137- for (auto const& observer : observers)
138+ std::shared_ptr<Device> device;
139 {
140- observer->device_changed(*dev_it);
141- observer->changes_complete();
142+ std::unique_lock<std::mutex> lock(handles_guard);
143+ auto dev_it = find_if(begin(handles), end(handles), [dev](auto const& ptr){return ptr.get() == dev;});
144+
145+ if (dev_it==end(handles))
146+ return;
147+
148+ device = *dev_it;
149 }
150+
151+ observers.for_each([&](std::shared_ptr<InputDeviceObserver> const& observer)
152+ {
153+ observer->device_changed(device);
154+ observer->changes_complete();
155+ });
156 }
157 }
158
159@@ -361,13 +379,12 @@
160 }
161 }
162 {
163- std::unique_lock<std::mutex> lock(observer_guard);
164- for (auto const& observer : observers)
165- {
166- for (auto const& dev : devices_to_notify)
167- observer->device_changed(dev);
168- observer->changes_complete();
169- }
170+ observers.for_each([&](std::shared_ptr<InputDeviceObserver> const& observer)
171+ {
172+ for (auto const& dev : devices_to_notify)
173+ observer->device_changed(dev);
174+ observer->changes_complete();
175+ });
176 }
177 }
178
179
180=== modified file 'src/server/input/default_input_device_hub.h'
181--- src/server/input/default_input_device_hub.h 2017-02-15 07:38:33 +0000
182+++ src/server/input/default_input_device_hub.h 2017-02-27 18:36:25 +0000
183@@ -27,6 +27,7 @@
184 #include "mir/input/input_device_hub.h"
185 #include "mir/input/input_device_info.h"
186 #include "mir/input/mir_input_config.h"
187+#include "mir/thread_safe_list.h"
188
189 #include "mir_toolkit/event.h"
190
191@@ -87,7 +88,7 @@
192 MirInputDeviceId create_new_device_id();
193 std::shared_ptr<Seat> const seat;
194 std::shared_ptr<dispatch::MultiplexingDispatchable> const input_dispatchable;
195- std::mutex observer_guard;
196+ std::mutex handles_guard;
197 std::shared_ptr<ServerActionQueue> const observer_queue;
198 std::shared_ptr<dispatch::ActionQueue> const device_queue;
199 std::shared_ptr<cookie::Authority> const cookie_authority;
200@@ -124,7 +125,7 @@
201 std::vector<std::shared_ptr<Device>> handles;
202 MirInputConfig config;
203 std::vector<std::unique_ptr<RegisteredDevice>> devices;
204- std::vector<std::shared_ptr<InputDeviceObserver>> observers;
205+ ThreadSafeList<std::shared_ptr<InputDeviceObserver>> observers;
206 std::mutex changed_devices_guard;
207 std::unique_ptr<std::vector<std::shared_ptr<Device>>> changed_devices;
208

Subscribers

People subscribed via source and target branches