Mir

Merge lp:~albaguirre/mir/more-tsan-happiness into lp:mir

  1. more-tsan-happiness
  2. Merge into development-branch
Proposed by Alberto Aguirre
Status: Merged
Approved by: Alberto Aguirre
Approved revision: no longer in the source branch.
Merged at revision: 2690
Proposed branch: lp:~albaguirre/mir/more-tsan-happiness
Merge into: lp:mir
Diff against target: 150 lines (+33/-14)
4 files modified
include/test/mir_test_framework/server_runner.h (+2/-1)
src/client/surface_map.cpp (+25/-9)
tests/mir_test_framework/async_server_runner.cpp (+1/-1)
tests/mir_test_framework/server_runner.cpp (+5/-3)
To merge this branch: bzr merge lp:~albaguirre/mir/more-tsan-happiness
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+262456@code.launchpad.net

Commit message

Fix potential data races and mutex locking order issues.

Avoid FD data race in AsyncServerRunner
Avoid data race on display_server pointer in ServerRunner
Avoid potential lock ordering issues in mir::client::ConnectionSurfaceMap

Description of the change

Fix potential data races and mutex locking order issues.

Avoid FD data race in AsyncServerRunner
Avoid data race on display_server pointer in ServerRunner
Avoid potential lock ordering issues in mir::client::ConnectionSurfaceMap

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

29 + for (auto const& surface : surfaces)
30 + {
31 + surfaces_to_release.push_back(surface.second);
32 + }
33 +
34 + for (auto const& info : streams)
35 + {
36 + if (info.second.owned)
37 + streams_to_release.push_back(info.second.stream);
38 + }
39 + }

It might be slightly more efficient to std::move() the map than create a vector. But OK

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2689
http://jenkins.qa.ubuntu.com/job/mir-ci/4154/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2933
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/448
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2881
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/310
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/310/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2881
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2881/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5705
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21326

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

review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> It might be slightly more efficient to std::move() the map than create a
> vector. But OK

Oh yeah. Done.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2690
http://jenkins.qa.ubuntu.com/job/mir-ci/4159/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2940
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/455
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2888
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/315
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/315/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2888
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2888/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5712
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21338

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/test/mir_test_framework/server_runner.h'
2--- include/test/mir_test_framework/server_runner.h 2015-06-18 10:55:04 +0000
3+++ include/test/mir_test_framework/server_runner.h 2015-06-19 19:07:19 +0000
4@@ -21,6 +21,7 @@
5
6 #include <gtest/gtest.h>
7
8+#include <atomic>
9 #include <string>
10 #include <thread>
11
12@@ -58,7 +59,7 @@
13
14 char const* const old_env;
15 std::thread server_thread;
16- mir::DisplayServer* display_server = 0;
17+ std::atomic<mir::DisplayServer*> display_server;
18 };
19 }
20
21
22=== modified file 'src/client/surface_map.cpp'
23--- src/client/surface_map.cpp 2015-06-17 05:20:42 +0000
24+++ src/client/surface_map.cpp 2015-06-19 19:07:19 +0000
25@@ -31,17 +31,26 @@
26
27 mcl::ConnectionSurfaceMap::~ConnectionSurfaceMap() noexcept
28 {
29+ std::unordered_map<frontend::SurfaceId, MirSurface*> surface_map;
30+ std::unordered_map<frontend::BufferStreamId, StreamInfo> stream_map;
31+ {
32+ //Prevent TSAN from flagging lock ordering issues
33+ //as the surface/buffer_stream destructors acquire internal locks
34+ //The mutex lock is used mainly as a memory barrier here
35+ std::lock_guard<std::mutex> lk(guard);
36+ surface_map = std::move(surfaces);
37+ stream_map = std::move(streams);
38+ }
39+
40 // Unless the client has screwed up there should be no surfaces left
41 // here. (OTOH *we* don't need to leak memory when clients screw up.)
42- std::lock_guard<std::mutex> lk(guard);
43-
44- for (auto const& surface : surfaces)
45+ for (auto const& surface : surface_map)
46 {
47 if (MirSurface::is_valid(surface.second))
48 delete surface.second;
49 }
50
51- for (auto const& info : streams)
52+ for (auto const& info : stream_map)
53 {
54 if (info.second.owned)
55 delete info.second.stream;
56@@ -51,11 +60,13 @@
57 void mcl::ConnectionSurfaceMap::with_surface_do(
58 mf::SurfaceId surface_id, std::function<void(MirSurface*)> const& exec) const
59 {
60- std::lock_guard<std::mutex> lk(guard);
61+ std::unique_lock<std::mutex> lk(guard);
62 auto const it = surfaces.find(surface_id);
63 if (it != surfaces.end())
64 {
65- exec(it->second);
66+ auto const surface = it->second;
67+ lk.unlock();
68+ exec(surface);
69 }
70 else
71 {
72@@ -67,9 +78,12 @@
73
74 void mcl::ConnectionSurfaceMap::insert(mf::SurfaceId surface_id, MirSurface* surface)
75 {
76+ // get_buffer_stream has internal locks - call before locking mutex to
77+ // avoid locking ordering issues
78+ auto const stream = surface->get_buffer_stream();
79 std::lock_guard<std::mutex> lk(guard);
80 surfaces[surface_id] = surface;
81- streams[mf::BufferStreamId(surface_id.as_value())] = {surface->get_buffer_stream(), false};
82+ streams[mf::BufferStreamId(surface_id.as_value())] = {stream, false};
83 }
84
85 void mcl::ConnectionSurfaceMap::erase(mf::SurfaceId surface_id)
86@@ -81,11 +95,13 @@
87 void mcl::ConnectionSurfaceMap::with_stream_do(
88 mf::BufferStreamId stream_id, std::function<void(ClientBufferStream*)> const& exec) const
89 {
90- std::lock_guard<std::mutex> lk(guard);
91+ std::unique_lock<std::mutex> lk(guard);
92 auto const it = streams.find(stream_id);
93 if (it != streams.end())
94 {
95- exec(it->second.stream);
96+ auto const stream = it->second.stream;
97+ lk.unlock();
98+ exec(stream);
99 }
100 else
101 {
102
103=== modified file 'tests/mir_test_framework/async_server_runner.cpp'
104--- tests/mir_test_framework/async_server_runner.cpp 2015-06-17 05:20:42 +0000
105+++ tests/mir_test_framework/async_server_runner.cpp 2015-06-19 19:07:19 +0000
106@@ -108,9 +108,9 @@
107
108 void mtf::AsyncServerRunner::stop_server()
109 {
110- connections.clear();
111 server.stop();
112 wait_for_server_exit();
113+ connections.clear();
114 }
115
116 void mtf::AsyncServerRunner::wait_for_server_exit()
117
118=== modified file 'tests/mir_test_framework/server_runner.cpp'
119--- tests/mir_test_framework/server_runner.cpp 2015-06-17 05:20:42 +0000
120+++ tests/mir_test_framework/server_runner.cpp 2015-06-19 19:07:19 +0000
121@@ -40,13 +40,15 @@
122 mtf::ServerRunner::ServerRunner() :
123 old_env(getenv(env_no_file))
124 {
125+ //Initialize atomically
126+ display_server = nullptr;
127 if (!old_env) setenv(env_no_file, "", true);
128 }
129
130 void mtf::ServerRunner::start_server()
131 {
132 display_server = start_mir_server();
133- if (display_server == nullptr)
134+ if (display_server.load() == nullptr)
135 {
136 BOOST_THROW_EXCEPTION(std::runtime_error{"Failed to start server thread"});
137 }
138@@ -68,11 +70,11 @@
139
140 void mtf::ServerRunner::stop_server()
141 {
142- if (display_server == nullptr)
143+ if (display_server.load() == nullptr)
144 {
145 BOOST_THROW_EXCEPTION(std::logic_error{"stop_server() called without calling start_server()?"});
146 }
147- display_server->stop();
148+ display_server.load()->stop();
149 if (server_thread.joinable()) server_thread.join();
150 }
151

Subscribers

People subscribed via source and target branches