Mir

Merge lp:~albaguirre/mir/fix-1427976 into lp:mir

Proposed by Alberto Aguirre on 2015-03-04
Status: Rejected
Rejected by: Alberto Aguirre on 2015-03-05
Proposed branch: lp:~albaguirre/mir/fix-1427976
Merge into: lp:mir
Diff against target: 363 lines (+162/-23)
5 files modified
src/include/common/mir/semaphore_lock.h (+81/-0)
src/server/frontend/session_mediator.cpp (+22/-22)
src/server/frontend/session_mediator.h (+2/-1)
tests/unit-tests/CMakeLists.txt (+1/-0)
tests/unit-tests/test_semaphore_lock.cpp (+56/-0)
To merge this branch: bzr merge lp:~albaguirre/mir/fix-1427976
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) Disapprove on 2015-03-05
Alan Griffiths Needs Fixing on 2015-03-05
Alexandros Frantzis (community) Needs Fixing on 2015-03-05
Daniel van Vugt 2015-03-04 Needs Fixing on 2015-03-05
PS Jenkins bot (community) continuous-integration Approve on 2015-03-04
Review via email: mp+251817@code.launchpad.net

Commit message

Avoid unlocking sesssion mutex in a different thread.

In some cases, SessionMediator needs to lock the session
while it waits for Surface::swap_buffers which may complete
asynchronously on a different thread. Since a std::mutex
cannot be used for such purposes a SemaphoreLock type is introduced.

Description of the change

Avoid unlocking sesssion mutex in a different thread.

In some cases, SessionMediator needs to lock the session
while it waits for Surface::swap_buffers which may complete
asynchronously on a different thread. Since a std::mutex
cannot be used for such purposes a SemaphoreLock type is introduced.

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

I'm not yet sure what you're trying to express, but Semaphore(Lock) seems like the wrong name.

I recall semaphore as a type of lock with a counter. The counter is the defining feature of a semaphore. And a quick search seems to support this: http://en.wikipedia.org/wiki/Semaphore_(programming)

So I think "Semaphore" is an incorrect name.

review: Needs Fixing
Alexandros Frantzis (afrantzis) wrote :

I am OK with the name, perhaps I would slightly prefer BinarySemaphore.

103 - auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
104 + SemaphoreUnlockIfUnwinding lock{session_lock};1

147 - auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
148 + SemaphoreUnlockIfUnwinding lock{session_lock};

The current locking scheme (using shared<unique_lock>) is safer. With the new scheme we may never release the lock if something goes wrong with the caller of the functor or the functor itself, since we have dropped RAII.

> I recall semaphore as a type of lock with a counter. The counter is the defining feature
> of a semaphore. And a quick search seems to support this:
> http://en.wikipedia.org/wiki/Semaphore_(programming)

From the bottom of the page:

A mutex is essentially the same thing as a binary semaphore and sometimes uses the
same basic implementation ... Mutexes have a concept of an owner. Only the process
that locked the mutex is supposed to unlock it. If the owner is stored by the mutex
this can be verified at runtime.

"Needs fixing" for the less safe locking scheme, looks good otherwise.

review: Needs Fixing
Alan Griffiths (alan-griffiths) wrote :

*Needs Discussion*

I wonder whether the better fix is to ensure there is only a single IPC thread running and dispatch all activity to that thread.

review: Needs Information
Alberto Aguirre (albaguirre) wrote :

> *Needs Discussion*
>
> I wonder whether the better fix is to ensure there is only a single IPC thread
> running and dispatch all activity to that thread.

By default the IPC thread pool is set to 1 but it's configurable option (--ipc-thread-pool)

The other thread involved is the compositor thread, because of the completion given to surface.swap_buffers in SessionMediator::exchange_buffer (which we could make synchronous).

Alberto Aguirre (albaguirre) wrote :

So we have three options:

1. Use a BinarySemaphore as proposed here
2. Make SessionMediator::exchange_buffer (and others which advance the buffer) wait for the completion and execute done->Run synchronously, in which case we can use a std::mutex
3. Remove the session_mutex all together by removing the --ipc-thread-pool option and doing 2 above (so only a single IPC thread is active at any time)

Alberto Aguirre (albaguirre) wrote :

Well option 2,3 would block the IPC thread, so not good.

Option 4. Schedule IPC responses to the IPC thread that originated them (if we still want an IPC thread pool)

I think we can land this for now (pending addressing Alexandros comments) and then do some refactoring to allow option 4 in a separate MP.

Alan Griffiths (alan-griffiths) wrote :

OK.

Let's go with a variant of this scheme.

Can we avoid the dubious release semantics of SemaphoreUnlockIfUnwinding by using move capture? (Thereby proving it was right to change to C++14!)

review: Needs Fixing
Alberto Aguirre (albaguirre) wrote :

Well after looking at bit more at the code, it seems like the real reason for the session_mutex unlock in the completion is to avoid holding a lock when invoking IPC callbacks.

Since Surface::swap_buffers may execute the completion within, we need to detect such condition and only unlock the session_mutex then instead of using a binary semaphore.

review: Disapprove

Unmerged revisions

2361. By Alberto Aguirre on 2015-03-04

Avoid unlocking sesssion mutex in a different thread.

In some cases, SessionMediator needs to lock the session
while it waits for Surface::swap_buffers which may complete asynchronously on a different thread.
Since a std::mutex cannot be used for such purposes a SemaphoreLock type is introduced.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'src/include/common/mir/semaphore_lock.h'
2--- src/include/common/mir/semaphore_lock.h 1970-01-01 00:00:00 +0000
3+++ src/include/common/mir/semaphore_lock.h 2015-03-04 20:00:48 +0000
4@@ -0,0 +1,81 @@
5+/*
6+ * Copyright © 2015 Canonical Ltd.
7+ *
8+ * This program is free software: you can redistribute it and/or modify it
9+ * under the terms of the GNU Lesser General Public License version 3,
10+ * as published by the Free Software Foundation.
11+ *
12+ * This program is distributed in the hope that it will be useful,
13+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15+ * GNU Lesser General Public License for more details.
16+ *
17+ * You should have received a copy of the GNU Lesser General Public License
18+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
19+ *
20+ * Authored By: Alberto Aguirre <alberto.aguirre@canonical.com>
21+ */
22+
23+#ifndef MIR_SEMAPHORE_LOCK_H_
24+#define MIR_SEMAPHORE_LOCK_H_
25+
26+#include <boost/throw_exception.hpp>
27+
28+#include <exception>
29+#include <mutex>
30+#include <condition_variable>
31+
32+namespace mir
33+{
34+
35+class SemaphoreLock
36+{
37+public:
38+ // Make it a BasicLockable so it can be used with
39+ // std::lock_guard or std::unique_lock
40+ void lock()
41+ {
42+ std::unique_lock<decltype(mutex)> lock{mutex};
43+ cv.wait(lock, [this]{ return !locked;});
44+ locked = true;
45+ }
46+
47+ void unlock()
48+ {
49+ std::lock_guard<decltype(mutex)> lock{mutex};
50+
51+ if (!locked)
52+ BOOST_THROW_EXCEPTION(std::logic_error("Already unlocked"));
53+
54+ locked = false;
55+ cv.notify_one();
56+ }
57+
58+private:
59+ std::mutex mutex;
60+ std::condition_variable cv;
61+ bool locked = false;
62+};
63+
64+class SemaphoreUnlockIfUnwinding
65+{
66+public:
67+ explicit SemaphoreUnlockIfUnwinding(SemaphoreLock& guard)
68+ : guard(guard)
69+ {
70+ guard.lock();
71+ }
72+
73+ ~SemaphoreUnlockIfUnwinding()
74+ {
75+ if (std::uncaught_exception())
76+ guard.unlock();
77+ }
78+
79+private:
80+ SemaphoreLock& guard;
81+};
82+
83+}
84+
85+#endif /* MIR_SEMAPHORE_LOCK_H_ */
86
87=== modified file 'src/server/frontend/session_mediator.cpp'
88--- src/server/frontend/session_mediator.cpp 2015-02-13 06:12:34 +0000
89+++ src/server/frontend/session_mediator.cpp 2015-03-04 20:00:48 +0000
90@@ -122,7 +122,7 @@
91
92 auto const session = shell->open_session(client_pid_, request->application_name(), event_sink);
93 {
94- std::lock_guard<std::mutex> lock(session_mutex);
95+ std::lock_guard<decltype(session_lock)> lock{session_lock};
96 weak_session = session;
97 }
98 connection_context.handle_client_connect(session);
99@@ -172,7 +172,7 @@
100 google::protobuf::Closure* done)
101 {
102
103- auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
104+ SemaphoreUnlockIfUnwinding lock{session_lock};
105
106 auto const session = weak_session.lock();
107
108@@ -243,10 +243,10 @@
109 }
110
111 advance_buffer(surf_id, *surface,
112- [lock, this, &surf_id, response, done, session]
113+ [this, &surf_id, response, done, session]
114 (graphics::Buffer* client_buffer, graphics::BufferIpcMsgType msg_type)
115 {
116- lock->unlock();
117+ session_lock.unlock();
118
119 response->mutable_buffer_stream()->mutable_id()->set_value(
120 surf_id.as_value());
121@@ -271,7 +271,7 @@
122 {
123 SurfaceId const surf_id{request->value()};
124
125- auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
126+ SemaphoreUnlockIfUnwinding lock{session_lock};
127
128 auto const session = weak_session.lock();
129
130@@ -283,10 +283,10 @@
131 auto surface = session->get_surface(surf_id);
132
133 advance_buffer(surf_id, *surface,
134- [lock, this, response, done, session]
135+ [this, response, done, session]
136 (graphics::Buffer* client_buffer, graphics::BufferIpcMsgType msg_type)
137 {
138- lock->unlock();
139+ session_lock.unlock();
140
141 pack_protobuf_buffer(*response, client_buffer, msg_type);
142
143@@ -306,7 +306,7 @@
144 mfd::ProtobufBufferPacker request_msg{const_cast<mir::protobuf::Buffer*>(&request->buffer())};
145 ipc_operations->unpack_buffer(request_msg, *surface_tracker.last_buffer(surface_id));
146
147- auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
148+ SemaphoreUnlockIfUnwinding lock{session_lock};
149 auto const session = weak_session.lock();
150 if (!session)
151 BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
152@@ -316,15 +316,15 @@
153 auto const& surface = session->get_surface(surface_id);
154 surface->swap_buffers(
155 surface_tracker.buffer_from(buffer_id),
156- [this, surface_id, lock, response, done](mg::Buffer* new_buffer)
157+ [this, surface_id, response, done](mg::Buffer* new_buffer)
158 {
159- lock->unlock();
160-
161 if (surface_tracker.track_buffer(surface_id, new_buffer))
162 pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::update_msg);
163 else
164 pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::full_msg);
165
166+ // Unlock until now as surface_tracker may be modified above.
167+ session_lock.unlock();
168 done->Run();
169 });
170 }
171@@ -336,7 +336,7 @@
172 google::protobuf::Closure* done)
173 {
174 {
175- std::unique_lock<std::mutex> lock(session_mutex);
176+ std::lock_guard<decltype(session_lock)> lock{session_lock};
177
178 auto session = weak_session.lock();
179
180@@ -362,7 +362,7 @@
181 google::protobuf::Closure* done)
182 {
183 {
184- std::unique_lock<std::mutex> lock(session_mutex);
185+ std::lock_guard<decltype(session_lock)> lock{session_lock};
186
187 auto session = weak_session.lock();
188
189@@ -391,7 +391,7 @@
190 response->set_attrib(attrib);
191
192 {
193- std::unique_lock<std::mutex> lock(session_mutex);
194+ std::lock_guard<decltype(session_lock)> lock{session_lock};
195
196 auto session = weak_session.lock();
197
198@@ -417,7 +417,7 @@
199 ::google::protobuf::Closure* done)
200 {
201 {
202- std::unique_lock<std::mutex> lock(session_mutex);
203+ std::lock_guard<decltype(session_lock)> lock{session_lock};
204 auto session = weak_session.lock();
205
206 if (session.get() == nullptr)
207@@ -536,7 +536,7 @@
208 google::protobuf::Closure* done)
209 {
210 {
211- std::unique_lock<std::mutex> lock(session_mutex);
212+ std::lock_guard<decltype(session_lock)> lock{session_lock};
213
214 auto session = weak_session.lock();
215
216@@ -568,7 +568,7 @@
217 ::google::protobuf::Closure* done)
218 {
219 {
220- std::unique_lock<std::mutex> lock(session_mutex);
221+ std::lock_guard<decltype(session_lock)> lock{session_lock};
222 auto session = weak_session.lock();
223
224 if (session.get() == nullptr)
225@@ -600,7 +600,7 @@
226 ::google::protobuf::Closure *done)
227 {
228 {
229- std::unique_lock<std::mutex> lock(session_mutex);
230+ std::lock_guard<decltype(session_lock)> lock{session_lock};
231
232 auto session = weak_session.lock();
233
234@@ -626,7 +626,7 @@
235 google::protobuf::Closure* done)
236 {
237 {
238- std::unique_lock<std::mutex> lock(session_mutex);
239+ std::lock_guard<decltype(session_lock)> lock{session_lock};
240 auto session = weak_session.lock();
241
242 if (session.get() == nullptr)
243@@ -660,7 +660,7 @@
244 google::protobuf::Closure* done)
245 {
246 {
247- std::unique_lock<std::mutex> lock(session_mutex);
248+ std::lock_guard<decltype(session_lock)> lock{session_lock};
249 auto session = weak_session.lock();
250
251 if (session.get() == nullptr)
252@@ -695,7 +695,7 @@
253 ::google::protobuf::Closure* done)
254 {
255 {
256- std::unique_lock<std::mutex> lock(session_mutex);
257+ std::lock_guard<decltype(session_lock)> lock{session_lock};
258 auto const session = weak_session.lock();
259
260 if (!session)
261@@ -721,7 +721,7 @@
262 ::google::protobuf::Closure* done)
263 {
264 {
265- std::unique_lock<std::mutex> lock(session_mutex);
266+ std::lock_guard<decltype(session_lock)> lock{session_lock};
267 auto const session = weak_session.lock();
268
269 if (!session)
270
271=== modified file 'src/server/frontend/session_mediator.h'
272--- src/server/frontend/session_mediator.h 2015-01-21 07:34:50 +0000
273+++ src/server/frontend/session_mediator.h 2015-03-04 20:00:48 +0000
274@@ -20,6 +20,7 @@
275 #define MIR_FRONTEND_SESSION_MEDIATOR_H_
276
277 #include "display_server.h"
278+#include "mir/semaphore_lock.h"
279 #include "mir/frontend/connection_context.h"
280 #include "mir/frontend/surface_id.h"
281 #include "mir/graphics/platform_ipc_operations.h"
282@@ -213,7 +214,7 @@
283
284 SurfaceTracker surface_tracker;
285
286- std::mutex session_mutex;
287+ SemaphoreLock session_lock;
288 std::weak_ptr<Session> weak_session;
289 std::weak_ptr<PromptSession> weak_prompt_session;
290 };
291
292=== modified file 'tests/unit-tests/CMakeLists.txt'
293--- tests/unit-tests/CMakeLists.txt 2015-03-03 06:56:37 +0000
294+++ tests/unit-tests/CMakeLists.txt 2015-03-04 20:00:48 +0000
295@@ -60,6 +60,7 @@
296 test_shared_library_prober.cpp
297 test_lockable_callback.cpp
298 test_module_deleter.cpp
299+ test_semaphore_lock.cpp
300 )
301
302 add_subdirectory(options/)
303
304=== added file 'tests/unit-tests/test_semaphore_lock.cpp'
305--- tests/unit-tests/test_semaphore_lock.cpp 1970-01-01 00:00:00 +0000
306+++ tests/unit-tests/test_semaphore_lock.cpp 2015-03-04 20:00:48 +0000
307@@ -0,0 +1,56 @@
308+/*
309+ * Copyright © 2015 Canonical Ltd.
310+ *
311+ * This program is free software: you can redistribute it and/or modify it
312+ * under the terms of the GNU General Public License version 3,
313+ * as published by the Free Software Foundation.
314+ *
315+ * This program is distributed in the hope that it will be useful,
316+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
317+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
318+ * GNU General Public License for more details.
319+ *
320+ * You should have received a copy of the GNU General Public License
321+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
322+ *
323+ * Authored By: Alberto Aguirre <alberto.aguirre@canonical.com>
324+ */
325+
326+#include "mir/semaphore_lock.h"
327+
328+#include <gtest/gtest.h>
329+
330+#include <exception>
331+
332+using namespace testing;
333+
334+namespace
335+{
336+void lock_and_throw(mir::SemaphoreLock& guard)
337+{
338+ mir::SemaphoreUnlockIfUnwinding lock{guard};
339+ throw std::runtime_error("");
340+}
341+}
342+
343+TEST(SemaphoreLock, throws_on_unlock_if_already_unlocked)
344+{
345+ mir::SemaphoreLock guard;
346+ EXPECT_THROW(guard.unlock(), std::logic_error);
347+}
348+
349+TEST(SemaphoreUnlockIfUnwinding, keeps_lock_if_no_exception)
350+{
351+ mir::SemaphoreLock guard;
352+ {
353+ mir::SemaphoreUnlockIfUnwinding lock{guard};
354+ }
355+ EXPECT_NO_THROW(guard.unlock());
356+}
357+
358+TEST(SemaphoreUnlockIfUnwinding, unlocks_after_exception)
359+{
360+ mir::SemaphoreLock guard;
361+ EXPECT_THROW(lock_and_throw(guard), std::runtime_error);
362+ EXPECT_THROW(guard.unlock(), std::logic_error);
363+}

Subscribers

People subscribed via source and target branches