Mir

Merge lp:~alan-griffiths/mir/fix-1201436 into lp:mir

Proposed by Alan Griffiths
Status: Merged
Approved by: kevin gunn
Approved revision: no longer in the source branch.
Merged at revision: 1099
Proposed branch: lp:~alan-griffiths/mir/fix-1201436
Merge into: lp:mir
Prerequisite: lp:~alan-griffiths/mir/client-dies-without-server
Diff against target: 162 lines (+39/-9)
6 files modified
src/client/mir_client_library.cpp (+4/-3)
src/client/mir_connection.cpp (+10/-2)
src/client/mir_wait_handle.cpp (+13/-0)
src/client/mir_wait_handle.h (+2/-0)
src/client/rpc/mir_socket_rpc_channel.cpp (+9/-4)
src/client/rpc/mir_socket_rpc_channel.h (+1/-0)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1201436
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Daniel van Vugt Needs Fixing
Kevin DuBois (community) Approve
Review via email: mp+188535@code.launchpad.net

Commit message

client: Fix race in connection failures

Description of the change

client: Fix race in connection failures

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

nit:
+ // We don't die while if are pending callbacks (as they touch this).

lgtm otherwise

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

You need to retest your conditions after a wait returns. So:
    if (received < expecting)
        wait_condition.wait_for(lock, limit);
should be:
    while (received < expecting)
        wait_condition.wait_for(lock, limit);

This is for two reasons:
  1. Some other thread could be waiting too, and racing against you. So you need to recheck the condition via a while loop after the wait returns (and when you have regained the lock) to verify the condition is really met, and you didn't lose a race.
  2. Historically OS wait functions could return prematurely (eg signals interrupting) so needed sanity checking. But I don't think this would still be true in the C++11 implementation... ?

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

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-09-11 21:20:15 +0000
3+++ src/client/mir_client_library.cpp 2013-10-02 08:59:32 +0000
4@@ -97,9 +97,10 @@
5
6 mcl::DefaultConnectionConfiguration conf{sock};
7
8- MirConnection* connection = new MirConnection(conf);
9-
10- return connection->connect(name, callback, context);
11+ std::unique_ptr<MirConnection> connection{new MirConnection(conf)};
12+ auto const result = connection->connect(name, callback, context);
13+ connection.release();
14+ return result;
15 }
16 catch (std::exception const& x)
17 {
18
19=== modified file 'src/client/mir_connection.cpp'
20--- src/client/mir_connection.cpp 2013-09-27 13:40:30 +0000
21+++ src/client/mir_connection.cpp 2013-10-02 08:59:32 +0000
22@@ -63,7 +63,6 @@
23
24 }
25
26-
27 MirConnection::MirConnection() :
28 channel(),
29 server(0),
30@@ -91,6 +90,10 @@
31
32 MirConnection::~MirConnection() noexcept
33 {
34+ // We don't die while if are pending callbacks (as they touch this).
35+ // But, if after 500ms we don't get a call, assume it won't happen.
36+ connect_wait_handle.wait_for_pending(std::chrono::milliseconds(500));
37+
38 std::lock_guard<std::mutex> lock(connection_guard);
39 valid_connections.erase(this);
40
41@@ -200,6 +203,7 @@
42
43 void MirConnection::connected(mir_connected_callback callback, void * context)
44 {
45+ bool safe_to_callback = true;
46 {
47 std::lock_guard<std::recursive_mutex> lock(mutex);
48
49@@ -207,6 +211,9 @@
50 {
51 if (!connect_result.has_error())
52 {
53+ // We're handling an error scenario that means we're not sync'd
54+ // with the client code - a callback isn't safe (or needed)
55+ safe_to_callback = false;
56 set_error_message("Connect failed");
57 }
58 }
59@@ -220,7 +227,7 @@
60 display_configuration->set_configuration(connect_result.display_configuration());
61 }
62
63- callback(this, context);
64+ if (safe_to_callback) callback(this, context);
65 connect_wait_handle.result_received();
66 }
67
68@@ -232,6 +239,7 @@
69 std::lock_guard<std::recursive_mutex> lock(mutex);
70
71 connect_parameters.set_application_name(app_name);
72+ connect_wait_handle.expect_result();
73 server.connect(
74 0,
75 &connect_parameters,
76
77=== modified file 'src/client/mir_wait_handle.cpp'
78--- src/client/mir_wait_handle.cpp 2013-08-28 03:41:48 +0000
79+++ src/client/mir_wait_handle.cpp 2013-10-02 08:59:32 +0000
80@@ -57,6 +57,19 @@
81 expecting = 0;
82 }
83
84+void MirWaitHandle::wait_for_pending(std::chrono::milliseconds limit)
85+{
86+ using std::chrono::steady_clock;
87+
88+ std::unique_lock<std::mutex> lock(guard);
89+
90+ auto time_limit = steady_clock::now() + limit;
91+
92+ while (received < expecting && steady_clock::now() < time_limit)
93+ wait_condition.wait_until(lock, time_limit);
94+}
95+
96+
97 void MirWaitHandle::wait_for_one() // wait for any single result
98 {
99 std::unique_lock<std::mutex> lock(guard);
100
101=== modified file 'src/client/mir_wait_handle.h'
102--- src/client/mir_wait_handle.h 2013-08-28 03:41:48 +0000
103+++ src/client/mir_wait_handle.h 2013-10-02 08:59:32 +0000
104@@ -20,6 +20,7 @@
105 #ifndef MIR_CLIENT_MIR_WAIT_HANDLE_H_
106 #define MIR_CLIENT_MIR_WAIT_HANDLE_H_
107
108+#include <chrono>
109 #include <condition_variable>
110 #include <mutex>
111
112@@ -37,6 +38,7 @@
113 void result_received();
114 void wait_for_all();
115 void wait_for_one();
116+ void wait_for_pending(std::chrono::milliseconds limit);
117
118 private:
119 std::mutex guard;
120
121=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
122--- src/client/rpc/mir_socket_rpc_channel.cpp 2013-09-30 09:20:06 +0000
123+++ src/client/rpc/mir_socket_rpc_channel.cpp 2013-10-02 08:59:32 +0000
124@@ -82,6 +82,14 @@
125 init();
126 }
127
128+void mclr::MirSocketRpcChannel::notify_disconnected()
129+{
130+ // TODO enable configuring the kill mechanism
131+ io_service.stop();
132+ raise (SIGTERM);
133+ pending_calls.force_completion();
134+}
135+
136 void mclr::MirSocketRpcChannel::init()
137 {
138 io_service_thread = std::thread([&]
139@@ -110,10 +118,7 @@
140 {
141 rpc_report->connection_failure(x);
142
143- // TODO enable configuring the kill mechanism
144- io_service.stop();
145- raise(SIGTERM);
146- pending_calls.force_completion();
147+ notify_disconnected();
148 }
149 });
150 }
151
152=== modified file 'src/client/rpc/mir_socket_rpc_channel.h'
153--- src/client/rpc/mir_socket_rpc_channel.h 2013-09-20 18:42:07 +0000
154+++ src/client/rpc/mir_socket_rpc_channel.h 2013-10-02 08:59:32 +0000
155@@ -93,6 +93,7 @@
156 size_t read_message_header();
157
158 mir::protobuf::wire::Result read_message_body(const size_t body_size);
159+ void notify_disconnected();
160
161 std::shared_ptr<SurfaceMap> surface_map;
162 std::shared_ptr<DisplayConfiguration> display_configuration;

Subscribers

People subscribed via source and target branches