Mir

Merge lp:~alan-griffiths/mir/cleanup-some-locking into lp:mir

Proposed by Alan Griffiths on 2013-12-20
Status: Merged
Approved by: kevin gunn on 2014-01-08
Approved revision: 1320
Merged at revision: 1316
Proposed branch: lp:~alan-griffiths/mir/cleanup-some-locking
Merge into: lp:mir
Diff against target: 725 lines (+103/-112)
10 files modified
src/client/mir_client_library.cpp (+1/-2)
src/client/mir_connection.cpp (+46/-46)
src/client/mir_connection.h (+3/-3)
src/client/mir_surface.cpp (+28/-33)
src/client/mir_surface.h (+3/-3)
src/client/rpc/mir_basic_rpc_channel.cpp (+16/-17)
src/client/rpc/mir_basic_rpc_channel.h (+1/-2)
src/client/rpc/mir_socket_rpc_channel.cpp (+3/-4)
src/client/rpc/mir_socket_rpc_channel.h (+1/-1)
tests/acceptance-tests/test_client_focus_notification.cpp (+1/-1)
To merge this branch: bzr merge lp:~alan-griffiths/mir/cleanup-some-locking
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve on 2014-01-07
Daniel van Vugt 2013-12-20 Approve on 2014-01-07
Review via email: mp+199828@code.launchpad.net

Commit message

client: rework dubious locking design in client rpc
(LP: #1243576) (LP: #1243584) (LP: #1243578) (LP: #1243575)

Description of the change

client: rework dubious locking design in client rpc

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

Verified bug 1243584 fixed.
Bug 1243576 remains... unlinking it.

Daniel van Vugt (vanvugt) wrote :

To test and verify the bugs;
sudo valgrind --tool=helgrind --log-file=helgrind.log bin/mir_demo_client_egltriangle

The news is pretty good already as branch halves the size of the log.

Daniel van Vugt (vanvugt) wrote :

(1) Bug 1243576 is still happening. Please fix it or unlink.

(2) Adding error_message_mutex seems to not achieve anything. Because the string returned by MirConnection::get_error_message() remains unprotected.

(3) std::mutex mutex; // Protects all members of *this that don't have their own
Is there a logical or performance reason why the class has multiple mutexes? If neither, then it would be easier to follow using just one.

review: Needs Fixing
1315. By Alan Griffiths on 2014-01-06

Remove repitition of mutex type in mir_surface.cpp

1316. By Alan Griffiths on 2014-01-06

Fix recursive mutex in MirSurface

1317. By Alan Griffiths on 2014-01-06

Don't lock mutex in MirSurface::release_cpu_region() - it is a private function called with lock already held

Daniel van Vugt (vanvugt) wrote :

(1)-(3) Nice. All fixed, I think. As possibly more (linked 4 bugs now).

(4) surface_map is no longer protected by a mutex in MirConnection. But now I see it has its own built-in. That might be worth a comment where it's called by MirConnection.

(5) This is getting very complex and hard to review now. Not least because the diff doesn't show what's protected by each lock_guard. I suggest aiming to only fix one bug at a time in future :)

review: Approve
Daniel van Vugt (vanvugt) wrote :

I was able to reproduce Jenkins' failure of ServerDisconnect.client_detects_server_shutdown on mako. But only once and not repeatably, so can't prove it's due to this branch.

Alan Griffiths (alan-griffiths) wrote :

> I was able to reproduce Jenkins' failure of
> ServerDisconnect.client_detects_server_shutdown on mako. But only once and not
> repeatably, so can't prove it's due to this branch.

Thanks for trying - I've not been able to reproduce (yet) and your success gives me hope.

As it appears consistently on this branch in CI I'm assuming it is either due to the branch or exposed by it. Either way I need to track it down to land this.

Alan Griffiths (alan-griffiths) wrote :

> > I was able to reproduce Jenkins' failure of
> > ServerDisconnect.client_detects_server_shutdown on mako. But only once and
> not
> > repeatably, so can't prove it's due to this branch.
>
> Thanks for trying - I've not been able to reproduce (yet) and your success
> gives me hope.
>
> As it appears consistently on this branch in CI I'm assuming it is either due
> to the branch or exposed by it. Either way I need to track it down to land
> this.

Right, I've been able to reproduce and also on *development-branch && desktop*.

This seems to be a race condition in the client side error reporting - something like detecting the broken pipe on the sending thread "before" processing the disconnect event message on the receiving thread.

I've not yet figured out where it breaks, or why this MP is particularly susceptible on the phone. But look for a separate MP.

1318. By Alan Griffiths on 2014-01-07

Fix typo

1319. By Alan Griffiths on 2014-01-07

merge lp:mir/devel

1320. By Alan Griffiths on 2014-01-07

Calls into MirSurface::server can result in an error callback on the same thread - so release the mutex before making the call

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/mir_client_library.cpp'
2--- src/client/mir_client_library.cpp 2013-12-18 02:19:19 +0000
3+++ src/client/mir_client_library.cpp 2014-01-07 15:59:47 +0000
4@@ -108,9 +108,8 @@
5 }
6 catch (std::exception const& x)
7 {
8- MirConnection* error_connection = new MirConnection();
9+ MirConnection* error_connection = new MirConnection(x.what());
10 error_connections.insert(error_connection);
11- error_connection->set_error_message(x.what());
12 callback(error_connection, context);
13 return nullptr;
14 }
15
16=== modified file 'src/client/mir_connection.cpp'
17--- src/client/mir_connection.cpp 2013-12-20 05:06:28 +0000
18+++ src/client/mir_connection.cpp 2014-01-07 15:59:47 +0000
19@@ -66,10 +66,10 @@
20 std::unordered_set<MirConnection*> valid_connections;
21 }
22
23-MirConnection::MirConnection() :
24+MirConnection::MirConnection(std::string const& error_message) :
25 channel(),
26 server(0),
27- error_message("ERROR")
28+ error_message(error_message)
29 {
30 }
31
32@@ -84,11 +84,11 @@
33 lifecycle_control(conf.the_lifecycle_control()),
34 surface_map(conf.the_surface_map())
35 {
36+ connect_result.set_error("connect not called");
37 {
38 std::lock_guard<std::mutex> lock(connection_guard);
39 valid_connections.insert(this);
40 }
41- connect_result.set_error("connect not called");
42 }
43
44 MirConnection::~MirConnection() noexcept
45@@ -97,9 +97,12 @@
46 // But, if after 500ms we don't get a call, assume it won't happen.
47 connect_wait_handle.wait_for_pending(std::chrono::milliseconds(500));
48
49- std::lock_guard<std::mutex> lock(connection_guard);
50- valid_connections.erase(this);
51+ {
52+ std::lock_guard<std::mutex> lock(connection_guard);
53+ valid_connections.erase(this);
54+ }
55
56+ std::lock_guard<decltype(mutex)> lock(mutex);
57 if (connect_result.has_platform())
58 {
59 auto const& platform = connect_result.platform();
60@@ -113,8 +116,6 @@
61 mir_surface_callback callback,
62 void * context)
63 {
64- std::lock_guard<std::recursive_mutex> lock(mutex);
65-
66 auto surface = new MirSurface(this, server, platform->create_buffer_factory(), input_platform, params, callback, context);
67
68 return surface->get_create_wait_handle();
69@@ -122,22 +123,18 @@
70
71 char const * MirConnection::get_error_message()
72 {
73- std::lock_guard<std::recursive_mutex> lock(mutex);
74+ std::lock_guard<decltype(mutex)> lock(mutex);
75
76 if (connect_result.has_error())
77 {
78 return connect_result.error().c_str();
79 }
80- else
81- {
82+
83 return error_message.c_str();
84- }
85 }
86
87 void MirConnection::set_error_message(std::string const& error)
88 {
89- std::lock_guard<std::recursive_mutex> lock(mutex);
90-
91 error_message = error;
92 }
93
94@@ -154,10 +151,8 @@
95
96 void MirConnection::released(SurfaceRelease data)
97 {
98- {
99- std::lock_guard<std::recursive_mutex> lock(mutex);
100- surface_map->erase(data.surface->id());
101- }
102+ surface_map->erase(data.surface->id());
103+
104 // Erasing this surface from surface_map means that it will no longer receive events
105 // If it's still focused, send an unfocused event before we kill it entirely
106 if (data.surface->attrib(mir_surface_attrib_focus) == mir_surface_focused)
107@@ -179,8 +174,6 @@
108 mir_surface_callback callback,
109 void * context)
110 {
111- std::lock_guard<std::recursive_mutex> lock(mutex);
112-
113 auto new_wait_handle = new MirWaitHandle;
114
115 SurfaceRelease surf_release{surface, new_wait_handle, callback, context};
116@@ -189,7 +182,7 @@
117 message.set_value(surface->id());
118
119 {
120- std::lock_guard<std::mutex> rel_lock(release_wait_handle_guard);
121+ std::lock_guard<decltype(release_wait_handle_guard)> rel_lock(release_wait_handle_guard);
122 release_wait_handles.push_back(new_wait_handle);
123 }
124
125@@ -216,7 +209,7 @@
126 {
127 bool safe_to_callback = true;
128 {
129- std::lock_guard<std::recursive_mutex> lock(mutex);
130+ std::lock_guard<decltype(mutex)> lock(mutex);
131
132 if (!connect_result.has_platform() || !connect_result.has_display_configuration())
133 {
134@@ -248,10 +241,13 @@
135 mir_connected_callback callback,
136 void * context)
137 {
138- std::lock_guard<std::recursive_mutex> lock(mutex);
139-
140- connect_parameters.set_application_name(app_name);
141- connect_wait_handle.expect_result();
142+ {
143+ std::lock_guard<decltype(mutex)> lock(mutex);
144+
145+ connect_parameters.set_application_name(app_name);
146+ connect_wait_handle.expect_result();
147+ }
148+
149 server.connect(
150 0,
151 &connect_parameters,
152@@ -266,7 +262,7 @@
153 /* todo: keeping all MirWaitHandles from a release surface until the end of the connection
154 is a kludge until we have a better story about the lifetime of MirWaitHandles */
155 {
156- std::lock_guard<std::mutex> lock(release_wait_handle_guard);
157+ std::lock_guard<decltype(release_wait_handle_guard)> lock(release_wait_handle_guard);
158 for (auto handle : release_wait_handles)
159 delete handle;
160 }
161@@ -276,8 +272,6 @@
162
163 MirWaitHandle* MirConnection::disconnect()
164 {
165- std::lock_guard<std::recursive_mutex> lock(mutex);
166-
167 server.disconnect(0, &ignored, &ignored,
168 google::protobuf::NewCallback(this, &MirConnection::done_disconnect));
169
170@@ -297,8 +291,6 @@
171 mir_drm_auth_magic_callback callback,
172 void* context)
173 {
174- std::lock_guard<std::recursive_mutex> lock(mutex);
175-
176 mir::protobuf::DRMMagic request;
177 request.set_magic(magic);
178
179@@ -320,12 +312,13 @@
180 return false;
181 }
182
183+ std::lock_guard<decltype(connection->mutex)> lock(connection->mutex);
184 return !connection->connect_result.has_error();
185 }
186
187 void MirConnection::populate(MirPlatformPackage& platform_package)
188 {
189- std::lock_guard<std::recursive_mutex> lock(mutex);
190+ std::lock_guard<decltype(mutex)> lock(mutex);
191
192 if (!connect_result.has_error() && connect_result.has_platform())
193 {
194@@ -351,7 +344,7 @@
195
196 MirDisplayConfiguration* MirConnection::create_copy_of_display_config()
197 {
198- std::lock_guard<std::recursive_mutex> lock(mutex);
199+ std::lock_guard<decltype(mutex)> lock(mutex);
200 return display_configuration->copy_to_client();
201 }
202
203@@ -361,6 +354,8 @@
204 unsigned int& valid_formats)
205 {
206 valid_formats = 0;
207+
208+ std::lock_guard<decltype(mutex)> lock(mutex);
209 if (!connect_result.has_error())
210 {
211 valid_formats = std::min(
212@@ -376,7 +371,7 @@
213
214 std::shared_ptr<mir::client::ClientPlatform> MirConnection::get_client_platform()
215 {
216- std::lock_guard<std::recursive_mutex> lock(mutex);
217+ std::lock_guard<decltype(mutex)> lock(mutex);
218
219 return platform;
220 }
221@@ -388,7 +383,7 @@
222
223 EGLNativeDisplayType MirConnection::egl_native_display()
224 {
225- std::lock_guard<std::recursive_mutex> lock(mutex);
226+ std::lock_guard<decltype(mutex)> lock(mutex);
227
228 return *native_display;
229 }
230@@ -410,8 +405,6 @@
231
232 bool MirConnection::validate_user_display_config(MirDisplayConfiguration* config)
233 {
234- std::lock_guard<std::recursive_mutex> lock(mutex);
235-
236 MirDisplayConfigurationStore orig_config{display_configuration->copy_to_client()};
237
238 if ((!config) || (config->num_outputs == 0) || (config->outputs == NULL) ||
239@@ -437,6 +430,8 @@
240
241 void MirConnection::done_display_configure()
242 {
243+ std::lock_guard<decltype(mutex)> lock(mutex);
244+
245 set_error_message(display_configuration_response.error());
246
247 if (!display_configuration_response.has_error())
248@@ -447,23 +442,26 @@
249
250 MirWaitHandle* MirConnection::configure_display(MirDisplayConfiguration* config)
251 {
252- std::lock_guard<std::recursive_mutex> lock(mutex);
253 if (!validate_user_display_config(config))
254 {
255 return NULL;
256 }
257
258 mir::protobuf::DisplayConfiguration request;
259- for (auto i=0u; i < config->num_outputs; i++)
260 {
261- auto output = config->outputs[i];
262- auto display_request = request.add_display_output();
263- display_request->set_output_id(output.output_id);
264- display_request->set_used(output.used);
265- display_request->set_current_mode(output.current_mode);
266- display_request->set_position_x(output.position_x);
267- display_request->set_position_y(output.position_y);
268- display_request->set_power_mode(output.power_mode);
269+ std::lock_guard<decltype(mutex)> lock(mutex);
270+
271+ for (auto i=0u; i < config->num_outputs; i++)
272+ {
273+ auto output = config->outputs[i];
274+ auto display_request = request.add_display_output();
275+ display_request->set_output_id(output.output_id);
276+ display_request->set_used(output.used);
277+ display_request->set_current_mode(output.current_mode);
278+ display_request->set_position_x(output.position_x);
279+ display_request->set_position_y(output.position_y);
280+ display_request->set_power_mode(output.power_mode);
281+ }
282 }
283
284 server.configure_display(0, &request, &display_configuration_response,
285@@ -475,6 +473,8 @@
286 bool MirConnection::set_extra_platform_data(
287 std::vector<int> const& extra_platform_data_arg)
288 {
289+ std::lock_guard<decltype(mutex)> lock(mutex);
290+
291 auto const total_data_size =
292 connect_result.platform().data_size() + extra_platform_data_arg.size();
293
294
295=== modified file 'src/client/mir_connection.h'
296--- src/client/mir_connection.h 2013-12-20 05:06:28 +0000
297+++ src/client/mir_connection.h 2014-01-07 15:59:47 +0000
298@@ -69,7 +69,7 @@
299 struct MirConnection : mir::client::ClientContext
300 {
301 public:
302- MirConnection();
303+ MirConnection(std::string const& error_message);
304
305 MirConnection(mir::client::ConnectionConfiguration& conf);
306 ~MirConnection() noexcept;
307@@ -87,7 +87,6 @@
308 void *context);
309
310 char const * get_error_message();
311- void set_error_message(std::string const& error);
312
313 MirWaitHandle* connect(
314 const char* app_name,
315@@ -125,7 +124,7 @@
316 bool set_extra_platform_data(std::vector<int> const& extra_platform_data);
317
318 private:
319- std::recursive_mutex mutex; // Protects all members of *this
320+ std::mutex mutex; // Protects all members of *this (except release_wait_handles)
321
322 std::shared_ptr<mir::client::rpc::MirBasicRpcChannel> channel;
323 mir::protobuf::DisplayServer::Stub server;
324@@ -163,6 +162,7 @@
325
326 struct SurfaceRelease;
327
328+ void set_error_message(std::string const& error);
329 void done_disconnect();
330 void connected(mir_connected_callback callback, void * context);
331 void released(SurfaceRelease );
332
333=== modified file 'src/client/mir_surface.cpp'
334--- src/client/mir_surface.cpp 2014-01-03 05:37:54 +0000
335+++ src/client/mir_surface.cpp 2014-01-07 15:59:47 +0000
336@@ -64,7 +64,7 @@
337
338 MirSurface::~MirSurface()
339 {
340- std::lock_guard<std::recursive_mutex> lock(mutex);
341+ std::lock_guard<decltype(mutex)> lock(mutex);
342
343 if (input_thread)
344 {
345@@ -80,7 +80,7 @@
346
347 MirSurfaceParameters MirSurface::get_parameters() const
348 {
349- std::lock_guard<std::recursive_mutex> lock(mutex);
350+ std::lock_guard<decltype(mutex)> lock(mutex);
351
352 return MirSurfaceParameters {
353 0,
354@@ -93,7 +93,7 @@
355
356 char const * MirSurface::get_error_message()
357 {
358- std::lock_guard<std::recursive_mutex> lock(mutex);
359+ std::lock_guard<decltype(mutex)> lock(mutex);
360
361 if (surface.has_error())
362 {
363@@ -104,21 +104,21 @@
364
365 int MirSurface::id() const
366 {
367- std::lock_guard<std::recursive_mutex> lock(mutex);
368+ std::lock_guard<decltype(mutex)> lock(mutex);
369
370 return surface.id().value();
371 }
372
373 bool MirSurface::is_valid() const
374 {
375- std::lock_guard<std::recursive_mutex> lock(mutex);
376+ std::lock_guard<decltype(mutex)> lock(mutex);
377
378 return !surface.has_error();
379 }
380
381 void MirSurface::get_cpu_region(MirGraphicsRegion& region_out)
382 {
383- std::lock_guard<std::recursive_mutex> lock(mutex);
384+ std::lock_guard<decltype(mutex)> lock(mutex);
385
386 auto buffer = buffer_depository->current_buffer();
387
388@@ -132,21 +132,21 @@
389
390 void MirSurface::release_cpu_region()
391 {
392- std::lock_guard<std::recursive_mutex> lock(mutex);
393-
394 secured_region.reset();
395 }
396
397 MirWaitHandle* MirSurface::next_buffer(mir_surface_callback callback, void * context)
398 {
399- std::lock_guard<std::recursive_mutex> lock(mutex);
400-
401+ std::unique_lock<decltype(mutex)> lock(mutex);
402 release_cpu_region();
403+ auto const id = &surface.id();
404+ auto const mutable_buffer = surface.mutable_buffer();
405+ lock.unlock();
406
407 server.next_buffer(
408 0,
409- &surface.id(),
410- surface.mutable_buffer(),
411+ id,
412+ mutable_buffer,
413 google::protobuf::NewCallback(this, &MirSurface::new_buffer, callback, context));
414
415 return &next_buffer_wait_handle;
416@@ -166,8 +166,6 @@
417
418 void MirSurface::process_incoming_buffer()
419 {
420- std::lock_guard<std::recursive_mutex> lock(mutex);
421-
422 auto const& buffer = surface.buffer();
423
424 /*
425@@ -201,17 +199,16 @@
426
427 void MirSurface::created(mir_surface_callback callback, void * context)
428 {
429+ auto platform = connection->get_client_platform();
430+
431 {
432- std::lock_guard<std::recursive_mutex> lock(mutex);
433+ std::lock_guard<decltype(mutex)> lock(mutex);
434
435 process_incoming_buffer();
436-
437- auto platform = connection->get_client_platform();
438 accelerated_window = platform->create_egl_native_window(this);
439-
440- connection->on_surface_created(id(), this);
441 }
442
443+ connection->on_surface_created(id(), this);
444 callback(this, context);
445 create_wait_handle.result_received();
446 }
447@@ -219,7 +216,7 @@
448 void MirSurface::new_buffer(mir_surface_callback callback, void * context)
449 {
450 {
451- std::lock_guard<std::recursive_mutex> lock(mutex);
452+ std::lock_guard<decltype(mutex)> lock(mutex);
453 process_incoming_buffer();
454 }
455
456@@ -244,23 +241,21 @@
457
458 std::shared_ptr<mcl::ClientBuffer> MirSurface::get_current_buffer()
459 {
460- std::lock_guard<std::recursive_mutex> lock(mutex);
461+ std::lock_guard<decltype(mutex)> lock(mutex);
462
463 return buffer_depository->current_buffer();
464 }
465
466 uint32_t MirSurface::get_current_buffer_id() const
467 {
468- std::lock_guard<std::recursive_mutex> lock(mutex);
469+ std::lock_guard<decltype(mutex)> lock(mutex);
470
471 return buffer_depository->current_buffer_id();
472 }
473
474 void MirSurface::populate(MirBufferPackage& buffer_package)
475 {
476- std::lock_guard<std::recursive_mutex> lock(mutex);
477-
478- if (is_valid() && surface.has_buffer())
479+ if (!surface.has_error() && surface.has_buffer())
480 {
481 auto const& buffer = surface.buffer();
482
483@@ -292,19 +287,19 @@
484
485 EGLNativeWindowType MirSurface::generate_native_window()
486 {
487- std::lock_guard<std::recursive_mutex> lock(mutex);
488+ std::lock_guard<decltype(mutex)> lock(mutex);
489
490 return *accelerated_window;
491 }
492
493 MirWaitHandle* MirSurface::configure(MirSurfaceAttrib at, int value)
494 {
495- std::lock_guard<std::recursive_mutex> lock(mutex);
496-
497+ std::unique_lock<decltype(mutex)> lock(mutex);
498 mp::SurfaceSetting setting;
499 setting.mutable_surfaceid()->CopyFrom(surface.id());
500 setting.set_attrib(at);
501 setting.set_ivalue(value);
502+ lock.unlock();
503
504 configure_wait_handle.expect_result();
505 server.configure_surface(0, &setting, &configure_result,
506@@ -315,7 +310,7 @@
507
508 void MirSurface::on_configured()
509 {
510- std::lock_guard<std::recursive_mutex> lock(mutex);
511+ std::lock_guard<decltype(mutex)> lock(mutex);
512
513 if (configure_result.has_surfaceid() &&
514 configure_result.surfaceid().value() == surface.id().value() &&
515@@ -345,14 +340,14 @@
516
517 int MirSurface::attrib(MirSurfaceAttrib at) const
518 {
519- std::lock_guard<std::recursive_mutex> lock(mutex);
520+ std::lock_guard<decltype(mutex)> lock(mutex);
521
522 return attrib_cache[at];
523 }
524
525 void MirSurface::set_event_handler(MirEventDelegate const* delegate)
526 {
527- std::lock_guard<std::recursive_mutex> lock(mutex);
528+ std::lock_guard<decltype(mutex)> lock(mutex);
529
530 if (input_thread)
531 {
532@@ -378,7 +373,7 @@
533
534 void MirSurface::handle_event(MirEvent const& e)
535 {
536- std::unique_lock<std::recursive_mutex> lock(mutex);
537+ std::unique_lock<decltype(mutex)> lock(mutex);
538
539 if (e.type == mir_event_type_surface)
540 {
541@@ -397,7 +392,7 @@
542
543 MirPlatformType MirSurface::platform_type()
544 {
545- std::lock_guard<std::recursive_mutex> lock(mutex);
546+ std::lock_guard<decltype(mutex)> lock(mutex);
547
548 auto platform = connection->get_client_platform();
549 return platform->platform_type();
550
551=== modified file 'src/client/mir_surface.h'
552--- src/client/mir_surface.h 2013-12-18 02:19:19 +0000
553+++ src/client/mir_surface.h 2014-01-07 15:59:47 +0000
554@@ -83,7 +83,6 @@
555 std::shared_ptr<mir::client::ClientBuffer> get_current_buffer();
556 uint32_t get_current_buffer_id() const;
557 void get_cpu_region(MirGraphicsRegion& region);
558- void release_cpu_region();
559 EGLNativeWindowType generate_native_window();
560
561 MirWaitHandle* configure(MirSurfaceAttrib a, int value);
562@@ -93,7 +92,7 @@
563 void handle_event(MirEvent const& e);
564
565 private:
566- mutable std::recursive_mutex mutex; // Protects all members of *this
567+ mutable std::mutex mutex; // Protects all members of *this
568
569 void on_configured();
570 void process_incoming_buffer();
571@@ -101,6 +100,7 @@
572 void created(mir_surface_callback callback, void * context);
573 void new_buffer(mir_surface_callback callback, void * context);
574 MirPixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf);
575+ void release_cpu_region();
576
577 /* todo: race condition. protobuf does not guarantee that callbacks will be synchronized. potential
578 race in surface, last_buffer_id */
579@@ -108,7 +108,7 @@
580 mir::protobuf::Surface surface;
581 std::string error_message;
582
583- MirConnection *connection;
584+ MirConnection* const connection;
585 MirWaitHandle create_wait_handle;
586 MirWaitHandle next_buffer_wait_handle;
587 MirWaitHandle configure_wait_handle;
588
589=== modified file 'src/client/rpc/mir_basic_rpc_channel.cpp'
590--- src/client/rpc/mir_basic_rpc_channel.cpp 2013-09-26 13:23:44 +0000
591+++ src/client/rpc/mir_basic_rpc_channel.cpp 2014-01-07 15:59:47 +0000
592@@ -33,40 +33,39 @@
593 {
594 }
595
596-mclrd::SendBuffer& mclrd::PendingCallCache::save_completion_details(
597+void mclrd::PendingCallCache::save_completion_details(
598 mir::protobuf::wire::Invocation& invoke,
599 google::protobuf::Message* response,
600 std::shared_ptr<google::protobuf::Closure> const& complete)
601 {
602 std::unique_lock<std::mutex> lock(mutex);
603
604- auto& current = pending_calls[invoke.id()] = PendingCall(response, complete);
605- return current.send_buffer;
606+ pending_calls[invoke.id()] = PendingCall(response, complete);
607 }
608
609 void mclrd::PendingCallCache::complete_response(mir::protobuf::wire::Result& result)
610 {
611- std::unique_lock<std::mutex> lock(mutex);
612- auto call = pending_calls.find(result.id());
613- if (call == pending_calls.end())
614+ PendingCall completion;
615+
616+ {
617+ std::unique_lock<std::mutex> lock(mutex);
618+ auto call = pending_calls.find(result.id());
619+ if (call != pending_calls.end())
620+ {
621+ completion = call->second;
622+ pending_calls.erase(call);
623+ }
624+ }
625+
626+ if (!completion.complete)
627 {
628 rpc_report->orphaned_result(result);
629 }
630 else
631 {
632- auto& completion = call->second;
633-
634 rpc_report->complete_response(result);
635-
636 completion.response->ParseFromString(result.response());
637-
638- // Let the completion closure live a bit longer than our lock...
639- std::shared_ptr<google::protobuf::Closure> complete =
640- completion.complete;
641- pending_calls.erase(call);
642-
643- lock.unlock(); // Avoid deadlocks in callbacks
644- complete->Run();
645+ completion.complete->Run();
646 }
647 }
648
649
650=== modified file 'src/client/rpc/mir_basic_rpc_channel.h'
651--- src/client/rpc/mir_basic_rpc_channel.h 2013-09-26 13:23:44 +0000
652+++ src/client/rpc/mir_basic_rpc_channel.h 2014-01-07 15:59:47 +0000
653@@ -54,7 +54,7 @@
654 public:
655 PendingCallCache(std::shared_ptr<RpcReport> const& rpc_report);
656
657- SendBuffer& save_completion_details(
658+ void save_completion_details(
659 mir::protobuf::wire::Invocation& invoke,
660 google::protobuf::Message* response,
661 std::shared_ptr<google::protobuf::Closure> const& complete);
662@@ -78,7 +78,6 @@
663 PendingCall()
664 : response(0), complete() {}
665
666- SendBuffer send_buffer;
667 google::protobuf::Message* response;
668 std::shared_ptr<google::protobuf::Closure> complete;
669 };
670
671=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
672--- src/client/rpc/mir_socket_rpc_channel.cpp 2013-12-18 02:19:19 +0000
673+++ src/client/rpc/mir_socket_rpc_channel.cpp 2014-01-07 15:59:47 +0000
674@@ -267,15 +267,14 @@
675 google::protobuf::NewPermanentCallback(this, &MirSocketRpcChannel::receive_file_descriptors, response, complete));
676
677 // Only save details after serialization succeeds
678- auto& send_buffer = pending_calls.save_completion_details(invocation, response, callback);
679+ pending_calls.save_completion_details(invocation, response, callback);
680
681 // Only send message when details saved for handling response
682- send_message(invocation, send_buffer, invocation);
683+ send_message(invocation, invocation);
684 }
685
686 void mclr::MirSocketRpcChannel::send_message(
687 mir::protobuf::wire::Invocation const& body,
688- detail::SendBuffer& send_buffer,
689 mir::protobuf::wire::Invocation const& invocation)
690 {
691 const size_t size = body.ByteSize();
692@@ -285,7 +284,7 @@
693 static_cast<unsigned char>((size >> 0) & 0xff)
694 };
695
696- send_buffer.resize(sizeof header_bytes + size);
697+ detail::SendBuffer send_buffer(sizeof header_bytes + size);
698 std::copy(header_bytes, header_bytes + sizeof header_bytes, send_buffer.begin());
699 body.SerializeToArray(send_buffer.data() + sizeof header_bytes, size);
700
701
702=== modified file 'src/client/rpc/mir_socket_rpc_channel.h'
703--- src/client/rpc/mir_socket_rpc_channel.h 2013-12-18 02:19:19 +0000
704+++ src/client/rpc/mir_socket_rpc_channel.h 2014-01-07 15:59:47 +0000
705@@ -83,7 +83,7 @@
706
707 void receive_file_descriptors(google::protobuf::Message* response, google::protobuf::Closure* complete);
708 void receive_file_descriptors(std::vector<int> &fds);
709- void send_message(mir::protobuf::wire::Invocation const& body, detail::SendBuffer& buffer,
710+ void send_message(mir::protobuf::wire::Invocation const& body,
711 mir::protobuf::wire::Invocation const& invocation);
712 void on_header_read(const boost::system::error_code& error);
713
714
715=== modified file 'tests/acceptance-tests/test_client_focus_notification.cpp'
716--- tests/acceptance-tests/test_client_focus_notification.cpp 2013-12-18 02:19:19 +0000
717+++ tests/acceptance-tests/test_client_focus_notification.cpp 2014-01-07 15:59:47 +0000
718@@ -136,7 +136,7 @@
719
720 }
721
722-TEST_F(BespokeDisplayServerTestFixture, two_scene_are_notified_of_gaining_and_losing_focus)
723+TEST_F(BespokeDisplayServerTestFixture, two_surfaces_are_notified_of_gaining_and_losing_focus)
724 {
725 using namespace ::testing;
726

Subscribers

People subscribed via source and target branches