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

[Alberto Aguirre]

Fix data race in ProtobufResponder::send_response

Fix data race in ProtobufResponder::send_response

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2365
http://jenkins.qa.ubuntu.com/job/mir-ci/3133/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1517
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1516
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1471
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1130
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1130/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1471
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1471/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4490
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18568

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3133/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Looks like send_response_result doesn't need to be a class member at all (?)

If not, then convert it to a local variable and no locking is required. Lock-less it might perform better, despite having to construct a local each time.

review: Needs Information

[Kevin DuBois (kdub)]

> Looks like send_response_result doesn't need to be a class member at all (?)

+1, it looks like this would work, and the MessageSender/ResourceCache have locks around their internals.

[Alan Griffiths (alan-griffiths)]

> Looks like send_response_result doesn't need to be a class member at all (?)
>
> If not, then convert it to a local variable and no locking is required. Lock-
> less it might perform better, despite having to construct a local each time.

IIRC this became a class member based on profiling a long time ago (it being "good practice to reuse protobuf objects"). I think in those days we only had one thread accessing it.

As contention free locks are fairly cheap I'm not convinced making it local would be a performance gain.

[Alan Griffiths (alan-griffiths)]

*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)]

> *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)]

So then we have three options:

1. Make send_response_result a local
2. Protect send_response_result
3. Make SessionMediator::exchange_buffer synchronous (i.e. don't execute done->Run() in the surface.swap_buffers completion function, instead wait for the completion to occur) and remove the --ipc-thread-pool option

[Alberto Aguirre (albaguirre)]

So option 3 is out of the question, since it would block the IPC thread.

Option 1 would potentially have performance impacts (protobuf objects could be heavy to construct).

So I rather keep the current optimization, protect it with a lock. When we re-profile IPC we can then revisit this decision.

[Alan Griffiths (alan-griffiths)]

OK, good enough for a bugfix.

review: Approve

[Daniel van Vugt (vanvugt)]

It's not a blocker if we don't know the answer...

The only issue to consider is that CPU hogs (excessive construction) are easy to find because any profiler can find them. Real-time wasters (excessive locking or sleeping) are harder; only real-time profilers like Google Profiler or Rational Quantify will notice those.

[Daniel van Vugt (vanvugt)]

We are presently assuming no thread does:
  send_response_result.set_events(...
If one did then the threads could interfere with each other.

Otherwise OK.

review: Approve

=== modified file 'src/server/frontend/protobuf_responder.cpp'
--- src/server/frontend/protobuf_responder.cpp	2015-01-21 07:34:50 +0000
+++ src/server/frontend/protobuf_responder.cpp	2015-03-05 01:06:08 +0000
@@ -43,11 +43,15 @@
 
     response->SerializeWithCachedSizesToArray(send_response_buffer.data());
 
-    send_response_result.set_id(id);
-    send_response_result.set_response(send_response_buffer.data(), send_response_buffer.size());
-
-    send_response_buffer.resize(send_response_result.ByteSize());
-    send_response_result.SerializeWithCachedSizesToArray(send_response_buffer.data());
+    {
+        std::lock_guard<decltype(result_guard)> lock{result_guard};
+
+        send_response_result.set_id(id);
+        send_response_result.set_response(send_response_buffer.data(), send_response_buffer.size());
+
+        send_response_buffer.resize(send_response_result.ByteSize());
+        send_response_result.SerializeWithCachedSizesToArray(send_response_buffer.data());
+    }
 
     sender->send(reinterpret_cast<char*>(send_response_buffer.data()), send_response_buffer.size(), fd_sets);
     resource_cache->free_resource(response);

=== modified file 'src/server/frontend/protobuf_responder.h'
--- src/server/frontend/protobuf_responder.h	2015-01-21 07:34:50 +0000
+++ src/server/frontend/protobuf_responder.h	2015-03-05 01:06:08 +0000
@@ -23,6 +23,7 @@
 #include "mir_protobuf_wire.pb.h"
 
 #include <memory>
+#include <mutex>
 
 namespace mir
 {
@@ -50,6 +51,7 @@
     std::shared_ptr<MessageSender> const sender;
     std::shared_ptr<ResourceCache> const resource_cache;
 
+    std::mutex result_guard;
     mir::protobuf::wire::Result send_response_result;
 };
 }