Merge lp:~mir-team/mir/enable-late-release into lp:mir
- enable-late-release
- Merge into development-branch
Status: | Merged |
---|---|
Approved by: | Alan Griffiths |
Approved revision: | no longer in the source branch. |
Merged at revision: | 1785 |
Proposed branch: | lp:~mir-team/mir/enable-late-release |
Merge into: | lp:mir |
Diff against target: |
380 lines (+219/-22) 8 files modified
examples/CMakeLists.txt (+3/-0) examples/release_at_exit.c (+109/-0) src/client/android/android_native_display_container.cpp (+24/-2) src/client/connection_configuration.h (+3/-1) src/client/default_connection_configuration.cpp (+1/-3) src/client/mesa/mesa_native_display_container.cpp (+21/-3) src/client/mir_connection.cpp (+52/-13) src/client/mir_connection.h (+6/-0) |
To merge this branch: | bzr merge lp:~mir-team/mir/enable-late-release |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alexandros Frantzis (community) | Approve | ||
PS Jenkins bot (community) | continuous-integration | Approve | |
Robert Carr (community) | Approve | ||
Review via email: mp+227178@code.launchpad.net |
Commit message
client: don't allow shared libraries to be unloaded before platform-api calls connection_
Description of the change
client: don't allow shared libraries to be unloaded before platform-api calls connection_
PS Jenkins bot (ps-jenkins) wrote : | # |
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1776
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Daniel van Vugt (vanvugt) wrote : | # |
(1) If the client library isn't guaranteeing this is cleaned up atexit for all clients already, I think it should. That's better than having to remember to reimplement the same in every client...
26 + // We should always release our connection
27 + atexit(
Alan Griffiths (alan-griffiths) wrote : | # |
> (1) If the client library isn't guaranteeing this is cleaned up atexit for all
> clients already, I think it should. That's better than having to remember to
Without this MP we require clients to release connections before the end of main().
We have one client (platform-api) that releases a connection after main() exits.
This MP adds support for the way that client behaves.
All client side resources are released when the process exits. The server will detect the dead socket and releases resources corresponding in that process. What further cleanup are you suggesting is needed?
> reimplement the same in every client...
>
> 26 + // We should always release our connection
> 27 + atexit(
Yes, maybe I should put the example back the way it was - this is not the canonical way to implement a client.
Alexandros Frantzis (afrantzis) wrote : | # |
The code looks ok.
I would prefer to revert basic_client.c to its previous form. If we want to be able to try this scenario we could add a flag to release after main (or perhaps a completely different example).
Ideally, though, we would have an automated test instead. Can the problem be reproduced deterministically? Couldn't we implement a test that launches a client process that releases after main and ensure that the process shuts down properly?
Alan Griffiths (alan-griffiths) wrote : | # |
> The code looks ok.
>
> I would prefer to revert basic_client.c to its previous form. If we want to be
> able to try this scenario we could add a flag to release after main (or
> perhaps a completely different example).
>
> Ideally, though, we would have an automated test instead. Can the problem be
> reproduced deterministically? Couldn't we implement a test that launches a
> client process that releases after main and ensure that the process shuts down
> properly?
The problem is deterministic with the hacked example code and the original implementation.
I'm not sure how easy it is within the test framework as the failure occurs as the program is exiting. I.e. after the test suite exits, after function scoped statics are deleted, and among the namespace scoped destructor executions.
For now, I'll just revert the example.
Alan Griffiths (alan-griffiths) wrote : | # |
I've added mir_test_
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1780
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1781
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Alexandros Frantzis (afrantzis) wrote : | # |
OK. It would be very nice to have an automated test, but not a blocker for this MP.
Preview Diff
1 | === modified file 'examples/CMakeLists.txt' |
2 | --- examples/CMakeLists.txt 2014-06-11 16:11:32 +0000 |
3 | +++ examples/CMakeLists.txt 2014-07-21 16:56:41 +0000 |
4 | @@ -50,6 +50,9 @@ |
5 | ${CMAKE_THREAD_LIBS_INIT} |
6 | ) |
7 | |
8 | +add_executable(mir_test_client_release_at_exit release_at_exit.c) |
9 | +target_link_libraries(mir_test_client_release_at_exit mirclient) |
10 | + |
11 | add_executable(mir_demo_client_multiwin multiwin.c) |
12 | target_link_libraries(mir_demo_client_multiwin mirclient) |
13 | |
14 | |
15 | === added file 'examples/release_at_exit.c' |
16 | --- examples/release_at_exit.c 1970-01-01 00:00:00 +0000 |
17 | +++ examples/release_at_exit.c 2014-07-21 16:56:41 +0000 |
18 | @@ -0,0 +1,109 @@ |
19 | +/* |
20 | + * Copyright © 2012-2014 Canonical Ltd. |
21 | + * |
22 | + * This program is free software: you can redistribute it and/or modify |
23 | + * it under the terms of the GNU General Public License version 3 as |
24 | + * published by the Free Software Foundation. |
25 | + * |
26 | + * This program is distributed in the hope that it will be useful, |
27 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
28 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
29 | + * GNU General Public License for more details. |
30 | + * |
31 | + * You should have received a copy of the GNU General Public License |
32 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
33 | + * |
34 | + * Authored by: Alan Griffiths <alan@octopull.co.uk> |
35 | + */ |
36 | + |
37 | +#include "mir_toolkit/mir_client_library.h" |
38 | + |
39 | +#include <assert.h> |
40 | +#include <string.h> |
41 | +#include <stdio.h> |
42 | +#include <stdlib.h> |
43 | +#include <getopt.h> |
44 | + |
45 | +static MirConnection *connection = 0; |
46 | +static MirSurface *surface = 0; |
47 | + |
48 | +static void connection_callback(MirConnection *new_connection, void *context) |
49 | +{ |
50 | + (void)context; |
51 | + connection = new_connection; |
52 | +} |
53 | + |
54 | +static void surface_create_callback(MirSurface *new_surface, void *context) |
55 | +{ |
56 | + (void)context; |
57 | + surface = new_surface; |
58 | +} |
59 | + |
60 | +static void surface_release_callback(MirSurface *old_surface, void *context) |
61 | +{ |
62 | + (void)old_surface; |
63 | + (void)context; |
64 | + surface = 0; |
65 | +} |
66 | + |
67 | +static void close_connection() |
68 | +{ |
69 | + if (connection) |
70 | + { |
71 | + mir_connection_release(connection); |
72 | + } |
73 | +} |
74 | + |
75 | + |
76 | +void demo_client(const char* server) |
77 | +{ |
78 | + atexit(&close_connection); |
79 | + |
80 | + mir_wait_for(mir_connect(server, __FILE__, connection_callback, NULL)); |
81 | + assert(mir_connection_is_valid(connection)); |
82 | + |
83 | + MirPixelFormat pixel_format; |
84 | + unsigned int valid_formats; |
85 | + mir_connection_get_available_surface_formats(connection, &pixel_format, 1, &valid_formats); |
86 | + MirSurfaceParameters const request_params = |
87 | + {NULL, 640, 480, pixel_format, |
88 | + mir_buffer_usage_hardware, mir_display_output_id_invalid}; |
89 | + |
90 | + mir_wait_for(mir_connection_create_surface(connection, &request_params, surface_create_callback, NULL)); |
91 | + mir_wait_for(mir_surface_release(surface, surface_release_callback, NULL)); |
92 | +} |
93 | + |
94 | +int main(int argc, char* argv[]) |
95 | +{ |
96 | + // Some variables for holding command line options |
97 | + char const *server = NULL; |
98 | + |
99 | + // Parse the command line |
100 | + { |
101 | + int arg; |
102 | + opterr = 0; |
103 | + while ((arg = getopt (argc, argv, "hm:")) != -1) |
104 | + { |
105 | + switch (arg) |
106 | + { |
107 | + case 'm': |
108 | + server = optarg; |
109 | + break; |
110 | + |
111 | + case '?': |
112 | + case 'h': |
113 | + default: |
114 | + puts(argv[0]); |
115 | + puts("Cutdown version of mir_demo_client_basic to show that"); |
116 | + puts("clients can release the connection after main() exits."); |
117 | + puts("Usage:"); |
118 | + puts(" -m <Mir server socket>"); |
119 | + puts(" -h: this help text"); |
120 | + return -1; |
121 | + } |
122 | + } |
123 | + } |
124 | + |
125 | + demo_client(server); |
126 | + return 0; |
127 | +} |
128 | |
129 | === modified file 'src/client/android/android_native_display_container.cpp' |
130 | --- src/client/android/android_native_display_container.cpp 2013-04-24 05:22:20 +0000 |
131 | +++ src/client/android/android_native_display_container.cpp 2014-07-21 16:56:41 +0000 |
132 | @@ -20,13 +20,35 @@ |
133 | |
134 | #include "mir_toolkit/mir_client_library.h" |
135 | |
136 | +#include <mutex> |
137 | + |
138 | namespace mcl = mir::client; |
139 | namespace mcla = mcl::android; |
140 | |
141 | +namespace |
142 | +{ |
143 | +// default_display_container needs to live until the library is unloaded |
144 | +std::mutex default_display_container_mutex; |
145 | +mcla::AndroidNativeDisplayContainer* default_display_container{nullptr}; |
146 | + |
147 | +extern "C" int __attribute__((destructor)) destroy() |
148 | +{ |
149 | + std::lock_guard<std::mutex> lock(default_display_container_mutex); |
150 | + |
151 | + delete default_display_container; |
152 | + |
153 | + return 0; |
154 | +} |
155 | +} |
156 | + |
157 | mcl::EGLNativeDisplayContainer& mcl::EGLNativeDisplayContainer::instance() |
158 | { |
159 | - static mcla::AndroidNativeDisplayContainer default_display_container; |
160 | - return default_display_container; |
161 | + std::lock_guard<std::mutex> lock(default_display_container_mutex); |
162 | + |
163 | + if (!default_display_container) |
164 | + default_display_container = new mcla::AndroidNativeDisplayContainer; |
165 | + |
166 | + return *default_display_container; |
167 | } |
168 | |
169 | mcla::AndroidNativeDisplayContainer::AndroidNativeDisplayContainer() |
170 | |
171 | === modified file 'src/client/connection_configuration.h' |
172 | --- src/client/connection_configuration.h 2014-05-29 13:21:08 +0000 |
173 | +++ src/client/connection_configuration.h 2014-07-21 16:56:41 +0000 |
174 | @@ -39,8 +39,11 @@ |
175 | class Logger; |
176 | } |
177 | |
178 | +class SharedLibrary; |
179 | + |
180 | namespace client |
181 | { |
182 | +std::shared_ptr<SharedLibrary>& libraries_cache(std::string const& lib_name); |
183 | |
184 | class ConnectionSurfaceMap; |
185 | class Logger; |
186 | @@ -70,7 +73,6 @@ |
187 | ConnectionConfiguration(ConnectionConfiguration const&) = delete; |
188 | ConnectionConfiguration& operator=(ConnectionConfiguration const&) = delete; |
189 | }; |
190 | - |
191 | } |
192 | } |
193 | |
194 | |
195 | === modified file 'src/client/default_connection_configuration.cpp' |
196 | --- src/client/default_connection_configuration.cpp 2014-05-29 13:21:08 +0000 |
197 | +++ src/client/default_connection_configuration.cpp 2014-07-21 16:56:41 +0000 |
198 | @@ -45,10 +45,8 @@ |
199 | |
200 | mir::SharedLibrary const* load_library(std::string const& libname) |
201 | { |
202 | - // There's no point in loading twice, and it isn't safe to unload... |
203 | - static std::map<std::string, std::shared_ptr<mir::SharedLibrary>> libraries_cache; |
204 | |
205 | - if (auto& ptr = libraries_cache[libname]) |
206 | + if (auto& ptr = mcl::libraries_cache(libname)) |
207 | { |
208 | return ptr.get(); |
209 | } |
210 | |
211 | === modified file 'src/client/mesa/mesa_native_display_container.cpp' |
212 | --- src/client/mesa/mesa_native_display_container.cpp 2014-03-06 06:05:17 +0000 |
213 | +++ src/client/mesa/mesa_native_display_container.cpp 2014-07-21 16:56:41 +0000 |
214 | @@ -46,12 +46,30 @@ |
215 | } |
216 | |
217 | } |
218 | -} |
219 | + |
220 | +// default_display_container needs to live until the library is unloaded |
221 | +std::mutex default_display_container_mutex; |
222 | +mclm::MesaNativeDisplayContainer* default_display_container{nullptr}; |
223 | + |
224 | +extern "C" int __attribute__((destructor)) destroy() |
225 | +{ |
226 | + std::lock_guard<std::mutex> lock(default_display_container_mutex); |
227 | + |
228 | + delete default_display_container; |
229 | + |
230 | + return 0; |
231 | +} |
232 | +} |
233 | + |
234 | |
235 | mcl::EGLNativeDisplayContainer& mcl::EGLNativeDisplayContainer::instance() |
236 | { |
237 | - static mclm::MesaNativeDisplayContainer default_display_container; |
238 | - return default_display_container; |
239 | + std::lock_guard<std::mutex> lock(default_display_container_mutex); |
240 | + |
241 | + if (!default_display_container) |
242 | + default_display_container = new mclm::MesaNativeDisplayContainer; |
243 | + |
244 | + return *default_display_container; |
245 | } |
246 | |
247 | mclm::MesaNativeDisplayContainer::MesaNativeDisplayContainer() |
248 | |
249 | === modified file 'src/client/mir_connection.cpp' |
250 | --- src/client/mir_connection.cpp 2014-06-16 14:47:14 +0000 |
251 | +++ src/client/mir_connection.cpp 2014-07-21 16:56:41 +0000 |
252 | @@ -63,11 +63,48 @@ |
253 | MirDisplayConfiguration* const config; |
254 | }; |
255 | |
256 | +using LibrariesCache = std::map<std::string, std::shared_ptr<mir::SharedLibrary>>; |
257 | + |
258 | std::mutex connection_guard; |
259 | -std::unordered_set<MirConnection*> valid_connections; |
260 | +MirConnection* valid_connections{nullptr}; |
261 | +// There's no point in loading twice, and it isn't safe to |
262 | +// unload while there are valid connections |
263 | +std::map<std::string, std::shared_ptr<mir::SharedLibrary>>* libraries_cache_ptr{nullptr}; |
264 | +} |
265 | + |
266 | +std::shared_ptr<mir::SharedLibrary>& mcl::libraries_cache(std::string const& libname) |
267 | +{ |
268 | + std::lock_guard<std::mutex> lock(connection_guard); |
269 | + |
270 | + if (!libraries_cache_ptr) |
271 | + libraries_cache_ptr = new LibrariesCache; |
272 | + |
273 | + return (*libraries_cache_ptr)[libname]; |
274 | +} |
275 | + |
276 | +MirConnection::Deregisterer::~Deregisterer() |
277 | +{ |
278 | + std::lock_guard<std::mutex> lock(connection_guard); |
279 | + |
280 | + for (auto current = &valid_connections; *current; current = &(*current)->next_valid) |
281 | + { |
282 | + if (self == *current) |
283 | + { |
284 | + *current = self->next_valid; |
285 | + break; |
286 | + } |
287 | + } |
288 | + |
289 | + // When the last valid connection goes we can clear the libraries cache |
290 | + if (!valid_connections) |
291 | + { |
292 | + delete libraries_cache_ptr; |
293 | + libraries_cache_ptr = nullptr; |
294 | + } |
295 | } |
296 | |
297 | MirConnection::MirConnection(std::string const& error_message) : |
298 | + deregisterer{this}, |
299 | channel(), |
300 | server(0), |
301 | error_message(error_message) |
302 | @@ -76,6 +113,7 @@ |
303 | |
304 | MirConnection::MirConnection( |
305 | mir::client::ConnectionConfiguration& conf) : |
306 | + deregisterer{this}, |
307 | channel(conf.the_rpc_channel()), |
308 | server(channel.get(), ::google::protobuf::Service::STUB_DOESNT_OWN_CHANNEL), |
309 | logger(conf.the_logger()), |
310 | @@ -89,7 +127,8 @@ |
311 | connect_result.set_error("connect not called"); |
312 | { |
313 | std::lock_guard<std::mutex> lock(connection_guard); |
314 | - valid_connections.insert(this); |
315 | + next_valid = valid_connections; |
316 | + valid_connections = this; |
317 | } |
318 | } |
319 | |
320 | @@ -99,11 +138,6 @@ |
321 | // But, if after 500ms we don't get a call, assume it won't happen. |
322 | connect_wait_handle.wait_for_pending(std::chrono::milliseconds(500)); |
323 | |
324 | - { |
325 | - std::lock_guard<std::mutex> lock(connection_guard); |
326 | - valid_connections.erase(this); |
327 | - } |
328 | - |
329 | std::lock_guard<decltype(mutex)> lock(mutex); |
330 | if (connect_result.has_platform()) |
331 | { |
332 | @@ -317,13 +351,18 @@ |
333 | bool MirConnection::is_valid(MirConnection *connection) |
334 | { |
335 | { |
336 | - std::lock_guard<std::mutex> lock(connection_guard); |
337 | - if (valid_connections.count(connection) == 0) |
338 | - return false; |
339 | + std::unique_lock<std::mutex> lock(connection_guard); |
340 | + for (auto current = valid_connections; current; current = current->next_valid) |
341 | + { |
342 | + if (connection == current) |
343 | + { |
344 | + lock.unlock(); |
345 | + std::lock_guard<decltype(connection->mutex)> lock(connection->mutex); |
346 | + return !connection->connect_result.has_error(); |
347 | + } |
348 | + } |
349 | } |
350 | - |
351 | - std::lock_guard<decltype(connection->mutex)> lock(connection->mutex); |
352 | - return !connection->connect_result.has_error(); |
353 | + return false; |
354 | } |
355 | |
356 | void MirConnection::populate(MirPlatformPackage& platform_package) |
357 | |
358 | === modified file 'src/client/mir_connection.h' |
359 | --- src/client/mir_connection.h 2014-06-11 16:11:32 +0000 |
360 | +++ src/client/mir_connection.h 2014-07-21 16:56:41 +0000 |
361 | @@ -134,6 +134,10 @@ |
362 | mir::protobuf::DisplayServer& display_server(); |
363 | |
364 | private: |
365 | + // MUST be first data member so it is destroyed last. |
366 | + struct Deregisterer |
367 | + { MirConnection* const self; ~Deregisterer(); } deregisterer; |
368 | + |
369 | std::mutex mutex; // Protects all members of *this (except release_wait_handles) |
370 | |
371 | std::shared_ptr<google::protobuf::RpcChannel> const channel; |
372 | @@ -174,6 +178,8 @@ |
373 | |
374 | struct SurfaceRelease; |
375 | |
376 | + MirConnection* next_valid{nullptr}; |
377 | + |
378 | void set_error_message(std::string const& error); |
379 | void done_disconnect(); |
380 | void connected(mir_connected_callback callback, void * context); |
FAILED: Continuous integration, rev:1775 jenkins. qa.ubuntu. com/job/ mir-team- mir-development -branch- ci/2182/ jenkins. qa.ubuntu. com/job/ mir-android- utopic- i386-build/ 981/console jenkins. qa.ubuntu. com/job/ mir-clang- utopic- amd64-build/ 987 jenkins. qa.ubuntu. com/job/ mir-mediumtests -utopic- touch/976/ console jenkins. qa.ubuntu. com/job/ mir-team- mir-development -branch- utopic- amd64-ci/ 703/console jenkins. qa.ubuntu. com/job/ mir-team- mir-development -branch- utopic- armhf-ci/ 701/console jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- utopic- armhf/3286/ console
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/mir- team-mir- development- branch- ci/2182/ rebuild
http://