Merge lp:~albaguirre/mir/fix-data-race-in-client-test into lp:mir

[Alberto Aguirre]

Fix data race in ClientLibrary.client_library_accesses_and_advances_buffers

Fix data race in ClientLibrary.client_library_accesses_and_advances_buffers

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2732
http://jenkins.qa.ubuntu.com/job/mir-ci/4284/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3112
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/634
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3060
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/433
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/433/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3060
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3060/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5866
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21736

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

This isn't actually a data race; every access to buffers is synchronised.

That said, it's harmless, so if it silences a warning...

review: Approve

[Alberto Aguirre (albaguirre)]

> This isn't actually a data race;

It is most certainly a data race. The race is between next_buffer called by the rpc thread and " EXPECT_THAT(buffers, Eq(1));" There's no synchronization between those two threads in regards to the member "buffers".

[Chris Halse Rogers (raof)]

All of the EXPECT_THAT(buffers, ...) calls I can see are synchronised by a previous mir_wait_for() call; am I missing one?

[Alberto Aguirre (albaguirre)]

But that doesn't guarantee synchronization for "buffers". The data needs to be protected with the same mutex when writing and reading from (or use atomic writes/reads) otherwise there are no guarantees on memory ordering.

[Chris Halse Rogers (raof)]

Ah, true, and somewhat unintuitive. Yay threading!

[Daniel van Vugt (vanvugt)]

review: Approve

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2015-07-02 03:53:29 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2015-07-08 21:31:15 +0000
@@ -39,6 +39,7 @@
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
+#include <atomic>
 #include <chrono>
 #include <thread>
 #include <cstring>
@@ -60,7 +61,7 @@
     std::set<MirSurface*> surfaces;
     MirConnection* connection = nullptr;
     MirSurface* surface  = nullptr;
-    int buffers = 0;
+    std::atomic<int> buffers{0};
 
     static void connection_callback(MirConnection* connection, void* context)
     {