Mir

Merge lp:~vanvugt/mir/fix-1194384 into lp:~mir-team/mir/trunk

Proposed by Daniel van Vugt on 2013-06-26
Status: Rejected
Rejected by: Daniel van Vugt on 2013-06-26
Proposed branch: lp:~vanvugt/mir/fix-1194384
Merge into: lp:~mir-team/mir/trunk
Diff against target: 346 lines (+182/-18)
6 files modified
examples/fingerpaint.c (+4/-2)
include/client/mir_toolkit/mir_client_library.h (+22/-0)
src/client/mir_client_library.cpp (+10/-0)
src/client/mir_surface.cpp (+33/-12)
src/client/mir_surface.h (+8/-4)
tests/acceptance-tests/test_client_library.cpp (+105/-0)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1194384
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve on 2013-06-26
Mir development team 2013-06-26 Pending
Review via email: mp+171520@code.launchpad.net

Commit message

Extend the client API to support thread-safe data sharing between event
callbacks and the rest of the client. Also document the risks. (LP: #1194384)

No existing behaviour changes. Thread safety is optional, but recommended. :)

To post a comment you must log in.
lp:~vanvugt/mir/fix-1194384 updated on 2013-06-26
781. By Daniel van Vugt on 2013-06-26

Fix a possible race in:
TEST_F(DefaultDisplayServerTestFixture, client_event_handler_is_thread_safe)

Daniel van Vugt (vanvugt) wrote :

One can argue that this whole proposal is unnecessary, except for the docs improvements. Because client authors can use their own locking instead.

The rationale for the proposal then is: Without this, it is impossible to write a reliable, predictable client that handles events without also forcing the client to depend on some third-party locking primitives (pthread, C++11). That's really the issue. With all the features you may choose to include or exclude from an API, determinism is something you need to include.

Also, assuming that all toolkits are thread-aware is a bad assumption. I'm sure there are plenty of toolkits out there that don't have, or care about, threads. And Mir should support them too.

Daniel van Vugt (vanvugt) wrote :

One can also argue that a more reusable alternative would be to create a mir_mutex API for clients. That would work too...

Daniel van Vugt (vanvugt) wrote :

Hmm, I forgot about non-event callbacks. They probably deserve consideration for a generic approach.

Work in progress again.

Unmerged revisions

781. By Daniel van Vugt on 2013-06-26

Fix a possible race in:
TEST_F(DefaultDisplayServerTestFixture, client_event_handler_is_thread_safe)

780. By Daniel van Vugt on 2013-06-26

Add acceptance test for {lock,unlock}_event_handler

779. By Daniel van Vugt on 2013-06-26

Clarify that set_event_handler is a synchronization point too.

778. By Daniel van Vugt on 2013-06-26

Prototype event handler locking

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'examples/fingerpaint.c'
2--- examples/fingerpaint.c 2013-06-13 12:12:58 +0000
3+++ examples/fingerpaint.c 2013-06-26 11:58:26 +0000
4@@ -253,7 +253,6 @@
5 MirDisplayInfo dinfo;
6 MirSurface *surf;
7 MirGraphicsRegion canvas;
8- MirEventDelegate delegate = {&on_event, &canvas};
9 int f;
10
11 (void)argc;
12@@ -290,7 +289,6 @@
13 surf = mir_connection_create_surface_sync(conn, &parm);
14 if (surf != NULL)
15 {
16- mir_surface_set_event_handler(surf, &delegate);
17
18 canvas.width = parm.width;
19 canvas.height = parm.height;
20@@ -300,12 +298,16 @@
21
22 if (canvas.vaddr != NULL)
23 {
24+ MirEventDelegate delegate = {&on_event, &canvas};
25+
26 signal(SIGINT, shutdown);
27 signal(SIGTERM, shutdown);
28
29 clear_region(&canvas, &background);
30 redraw(surf, &canvas);
31
32+ mir_surface_set_event_handler(surf, &delegate);
33+
34 while (running)
35 {
36 sleep(1); /* Is there a better way yet? */
37
38=== modified file 'include/client/mir_toolkit/mir_client_library.h'
39--- include/client/mir_toolkit/mir_client_library.h 2013-06-20 09:43:30 +0000
40+++ include/client/mir_toolkit/mir_client_library.h 2013-06-26 11:58:26 +0000
41@@ -136,12 +136,34 @@
42
43 /**
44 * Set the event handler to be called when events arrive for a surface.
45+ * \warning Event callbacks are called from a <b>different thread</b>, so if
46+ * you don't already have your own locking, you will need to use
47+ * mir_surface_lock_event_handler to protect any data shared between
48+ * event_handler and your other code.
49 * \param [in] surface The surface
50 * \param [in] event_handler The event handler to call
51+ * \post The old event handler (if any) has returned
52 */
53 void mir_surface_set_event_handler(MirSurface *surface,
54 MirEventDelegate const *event_handler);
55
56+/**
57+ * Acquire the mutex that surrounds event callbacks for a given surface. Use
58+ * this to guarantee safe access to shared data you use inside the event
59+ * handler.
60+ * \param [in] surface The surface whose event handler to lock
61+ * \post The event handler is not executing and will not be
62+ * entered until you mir_surface_unlock_event_handler
63+ */
64+void mir_surface_lock_event_handler(MirSurface *surface);
65+
66+/**
67+ * Release the mutex that surrounds event callbacks for a given surface.
68+ * \pre You have called mir_surface_lock_event_handler
69+ * \param [in] surface The surface whose event handler to unlock
70+ */
71+void mir_surface_unlock_event_handler(MirSurface *surface);
72+
73 /**
74 * Get a window type that can be used for OpenGL ES 2.0 acceleration.
75 * \param [in] surface The surface
76
77=== modified file 'src/client/mir_client_library.cpp'
78--- src/client/mir_client_library.cpp 2013-06-20 09:43:30 +0000
79+++ src/client/mir_client_library.cpp 2013-06-26 11:58:26 +0000
80@@ -178,6 +178,16 @@
81 surface->set_event_handler(event_handler);
82 }
83
84+void mir_surface_lock_event_handler(MirSurface *surface)
85+{
86+ surface->lock_event_handler();
87+}
88+
89+void mir_surface_unlock_event_handler(MirSurface *surface)
90+{
91+ surface->unlock_event_handler();
92+}
93+
94 MirWaitHandle* mir_surface_release(
95 MirSurface * surface,
96 mir_surface_callback callback, void * context)
97
98=== modified file 'src/client/mir_surface.cpp'
99--- src/client/mir_surface.cpp 2013-06-20 09:43:30 +0000
100+++ src/client/mir_surface.cpp 2013-06-26 11:58:26 +0000
101@@ -43,7 +43,8 @@
102 : server(server),
103 connection(allocating_connection),
104 buffer_depository(std::make_shared<mcl::ClientBufferDepository>(factory, mir::frontend::client_buffer_cache_size)),
105- input_platform(input_platform)
106+ input_platform(input_platform),
107+ event_delegate(nullptr)
108 {
109 mir::protobuf::SurfaceParameters message;
110 message.set_surface_name(params.name ? params.name : std::string());
111@@ -288,6 +289,14 @@
112 return attrib_cache[at];
113 }
114
115+void MirSurface::event_callback(MirEvent const* event)
116+{
117+ lock_event_handler();
118+ if (event_delegate)
119+ event_delegate->callback(this, event, event_delegate->context);
120+ unlock_event_handler();
121+}
122+
123 void MirSurface::set_event_handler(MirEventDelegate const* delegate)
124 {
125 if (input_thread)
126@@ -297,19 +306,32 @@
127 input_thread = nullptr;
128 }
129
130- if (delegate)
131+ lock_event_handler(); // can only block if the user still owns it
132+
133+ event_delegate = delegate; // can be NULL, to remove the handler
134+
135+ if (delegate && surface.fd_size() > 0)
136 {
137- handle_event_callback = std::bind(delegate->callback, this,
138- std::placeholders::_1,
139- delegate->context);
140+ auto handle_event_callback = std::bind(&MirSurface::event_callback,
141+ this,
142+ std::placeholders::_1);
143
144- if (surface.fd_size() > 0 && handle_event_callback)
145- {
146- input_thread = input_platform->create_input_thread(surface.fd(0),
147+ input_thread = input_platform->create_input_thread(surface.fd(0),
148 handle_event_callback);
149- input_thread->start();
150- }
151+ input_thread->start();
152 }
153+
154+ unlock_event_handler();
155+}
156+
157+void MirSurface::lock_event_handler()
158+{
159+ event_mutex.lock();
160+}
161+
162+void MirSurface::unlock_event_handler()
163+{
164+ event_mutex.unlock();
165 }
166
167 void MirSurface::handle_event(MirEvent const& e)
168@@ -321,8 +343,7 @@
169 attrib_cache[a] = e.surface.value;
170 }
171
172- if (handle_event_callback)
173- handle_event_callback(&e);
174+ event_callback(&e);
175 }
176
177 MirPlatformType MirSurface::platform_type()
178
179=== modified file 'src/client/mir_surface.h'
180--- src/client/mir_surface.h 2013-06-20 09:43:30 +0000
181+++ src/client/mir_surface.h 2013-06-26 11:58:26 +0000
182@@ -31,7 +31,7 @@
183 #include "client_platform.h"
184
185 #include <memory>
186-#include <functional>
187+#include <mutex>
188
189 namespace mir
190 {
191@@ -88,7 +88,10 @@
192 MirWaitHandle* configure(MirSurfaceAttrib a, int value);
193 int attrib(MirSurfaceAttrib a) const;
194
195+ void event_callback(MirEvent const* event);
196 void set_event_handler(MirEventDelegate const* delegate);
197+ void lock_event_handler();
198+ void unlock_event_handler();
199 void handle_event(MirEvent const& e);
200
201 private:
202@@ -99,8 +102,6 @@
203 void new_buffer(mir_surface_callback callback, void * context);
204 mir::geometry::PixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf);
205
206- /* todo: race condition. protobuf does not guarantee that callbacks will be synchronized. potential
207- race in surface, last_buffer_id */
208 mir::protobuf::DisplayServer::Stub & server;
209 mir::protobuf::Surface surface;
210 std::string error_message;
211@@ -121,8 +122,11 @@
212 // Cache of latest SurfaceSettings returned from the server
213 int attrib_cache[mir_surface_attrib_arraysize_];
214
215- std::function<void(MirEvent const*)> handle_event_callback;
216 std::shared_ptr<mir::input::receiver::InputReceiverThread> input_thread;
217+
218+ // event_mutex protects the execution of event_delgate->callback only.
219+ std::mutex event_mutex;
220+ const MirEventDelegate *event_delegate;
221 };
222
223 #endif /* MIR_CLIENT_PRIVATE_MIR_WAIT_HANDLE_H_ */
224
225=== modified file 'tests/acceptance-tests/test_client_library.cpp'
226--- tests/acceptance-tests/test_client_library.cpp 2013-06-06 16:44:49 +0000
227+++ tests/acceptance-tests/test_client_library.cpp 2013-06-26 11:58:26 +0000
228@@ -27,6 +27,7 @@
229
230 #include <gtest/gtest.h>
231 #include <chrono>
232+#include <thread>
233 #include <cstring>
234
235 namespace mf = mir::frontend;
236@@ -345,6 +346,110 @@
237 launch_client_process(client_config);
238 }
239
240+TEST_F(DefaultDisplayServerTestFixture, client_event_handler_is_thread_safe)
241+{
242+ struct ClientConfig : ClientConfigCommon
243+ {
244+ static void event_callback(MirSurface* surface, MirEvent const* event,
245+ void* ctx)
246+ {
247+ std::this_thread::sleep_for(std::chrono::seconds(1));
248+ ClientConfig* self = static_cast<ClientConfig*>(ctx);
249+ self->last_event = *event;
250+ self->last_event_surface = surface;
251+ self->prev_owner = self->owner;
252+ self->owner = "event_callback";
253+ }
254+
255+ void exec()
256+ {
257+ connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
258+ ASSERT_TRUE(connection != NULL);
259+ ASSERT_TRUE(mir_connection_is_valid(connection));
260+
261+ MirSurfaceParameters const request_params =
262+ {
263+ __PRETTY_FUNCTION__,
264+ 640, 480,
265+ mir_pixel_format_abgr_8888,
266+ mir_buffer_usage_hardware
267+ };
268+
269+ memset(&last_event, 0, sizeof last_event);
270+ last_event_surface = nullptr;
271+ owner = "no one";
272+ prev_owner = "nobody";
273+
274+ MirEventDelegate delegate{&event_callback, this};
275+
276+ surface =
277+ mir_connection_create_surface_sync(connection, &request_params);
278+ ASSERT_TRUE(surface != NULL);
279+ ASSERT_TRUE(mir_surface_is_valid(surface));
280+
281+ mir_surface_set_event_handler(surface, &delegate);
282+
283+ int surface_id = mir_surface_get_id(surface);
284+
285+ mir_surface_lock_event_handler(surface);
286+ MirWaitHandle *w = mir_surface_set_state(surface,
287+ mir_surface_state_fullscreen);
288+ std::this_thread::sleep_for(std::chrono::seconds(3));
289+ // by now, event_callback is trying to start, but is blocked.
290+ EXPECT_STREQ("no one", owner);
291+ EXPECT_STREQ("nobody", prev_owner);
292+ mir_surface_unlock_event_handler(surface);
293+
294+ mir_wait_for(w);
295+ mir_surface_lock_event_handler(surface);
296+ EXPECT_STREQ("event_callback", owner);
297+ EXPECT_STREQ("no one", prev_owner);
298+ prev_owner = owner;
299+ owner = "exec";
300+ mir_surface_unlock_event_handler(surface);
301+
302+ EXPECT_EQ(surface, last_event_surface);
303+ EXPECT_EQ(mir_event_type_surface, last_event.type);
304+ EXPECT_EQ(surface_id, last_event.surface.id);
305+ EXPECT_EQ(mir_surface_attrib_state, last_event.surface.attrib);
306+ EXPECT_EQ(mir_surface_state_fullscreen, last_event.surface.value);
307+
308+ memset(&last_event, 0, sizeof last_event);
309+ last_event_surface = nullptr;
310+
311+ mir_wait_for(mir_surface_set_state(surface,
312+ static_cast<MirSurfaceState>(777)));
313+ EXPECT_EQ(0, last_event_surface);
314+ EXPECT_EQ(0, last_event.type);
315+ EXPECT_EQ(0, last_event.surface.id);
316+ EXPECT_EQ(0, last_event.surface.attrib);
317+ EXPECT_EQ(0, last_event.surface.value);
318+
319+ mir_surface_lock_event_handler(surface);
320+ EXPECT_STREQ("exec", owner);
321+ EXPECT_STREQ("event_callback", prev_owner);
322+ mir_surface_unlock_event_handler(surface);
323+
324+ mir_wait_for(mir_surface_set_state(surface,
325+ mir_surface_state_minimized));
326+ mir_surface_set_event_handler(surface, nullptr);
327+ // Event handler is dead. Locking no longer required.
328+ EXPECT_STREQ("event_callback", owner);
329+ EXPECT_STREQ("exec", prev_owner);
330+
331+ mir_surface_release_sync(surface);
332+ mir_connection_release(connection);
333+ }
334+
335+ MirEvent last_event;
336+ MirSurface* last_event_surface;
337+ const char* prev_owner;
338+ const char* owner;
339+ } client_config;
340+
341+ launch_client_process(client_config);
342+}
343+
344 TEST_F(DefaultDisplayServerTestFixture, client_receives_surface_state_events)
345 {
346 struct ClientConfig : ClientConfigCommon

Subscribers

People subscribed via source and target branches