Merge lp:~ted/ubuntu-app-launch/non-blocking-resume into lp:ubuntu-app-launch/touch-vivid

Proposed by Ted Gould on 2016-11-09
Status: Merged
Approved by: Michał Sawicz on 2016-11-11
Approved revision: 275
Merged at revision: 267
Proposed branch: lp:~ted/ubuntu-app-launch/non-blocking-resume
Merge into: lp:ubuntu-app-launch/touch-vivid
Diff against target: 470 lines (+158/-126)
6 files modified
libubuntu-app-launch/application-impl-base.cpp (+97/-75)
libubuntu-app-launch/application-impl-base.h (+15/-6)
libubuntu-app-launch/application-impl-libertine.cpp (+1/-2)
libubuntu-app-launch/registry-impl.cpp (+19/-26)
libubuntu-app-launch/snapd-info.cpp (+1/-1)
tests/libual-cpp-test.cc (+25/-16)
To merge this branch: bzr merge lp:~ted/ubuntu-app-launch/non-blocking-resume
Reviewer Review Type Date Requested Status
Michał Sawicz 2016-11-09 Approve on 2016-11-11
Review via email: mp+310492@code.launchpad.net

Commit message

Make resume/pause non-blocking

To post a comment you must log in.
274. By Ted Gould on 2016-11-09

Forgot to remove emit.h from the CMakeLists.txt

Michał Sawicz (saviq) wrote :
review: Needs Fixing
275. By Ted Gould on 2016-11-10

Make several functions static so that they don't depend on the object existing for their execution

Michał Sawicz (saviq) wrote :
review: Approve
Pat McGowan (pat-mcgowan) wrote :

Tested silo 2181 on an MX4 running proposed and it fixed the symptoms from lp:1637996

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libubuntu-app-launch/application-impl-base.cpp'
2--- libubuntu-app-launch/application-impl-base.cpp 2016-10-03 23:54:20 +0000
3+++ libubuntu-app-launch/application-impl-base.cpp 2016-11-10 15:35:24 +0000
4@@ -257,8 +257,15 @@
5 /** Returns all the PIDs that are in the cgroup for this application */
6 std::vector<pid_t> UpstartInstance::pids()
7 {
8- auto pids = registry_->impl->pidsFromCgroup(upstartJobPath());
9- g_debug("Got %d PIDs for AppID '%s'", int(pids.size()), std::string(appId_).c_str());
10+ return pids(registry_, appId_, upstartJobPath());
11+}
12+
13+std::vector<pid_t> UpstartInstance::pids(const std::shared_ptr<Registry>& reg,
14+ const AppID& appid,
15+ const std::string& jobpath)
16+{
17+ auto pids = reg->impl->pidsFromCgroup(jobpath);
18+ g_debug("Got %d PIDs for AppID '%s'", int(pids.size()), std::string(appid).c_str());
19 return pids;
20 }
21
22@@ -267,16 +274,23 @@
23 void UpstartInstance::pause()
24 {
25 g_debug("Pausing application: %s", std::string(appId_).c_str());
26+
27+ auto registry = registry_;
28+ auto appid = appId_;
29+ auto jobpath = upstartJobPath();
30+
31+ registry->impl->thread.executeOnThread([registry, appid, jobpath] {
32+ auto pids = forAllPids(registry, appid, jobpath, [](pid_t pid) {
33+ auto oomval = oom::paused();
34+ g_debug("Pausing PID: %d (%d)", pid, int(oomval));
35+ signalToPid(pid, SIGSTOP);
36+ oomValueToPid(pid, oomval);
37+ });
38+
39+ pidListToDbus(registry, appid, pids, "ApplicationPaused");
40+ });
41+
42 registry_->impl->zgSendEvent(appId_, ZEITGEIST_ZG_LEAVE_EVENT);
43-
44- auto pids = forAllPids([this](pid_t pid) {
45- auto oomval = oom::paused();
46- g_debug("Pausing PID: %d (%d)", pid, int(oomval));
47- signalToPid(pid, SIGSTOP);
48- oomValueToPid(pid, oomval);
49- });
50-
51- pidListToDbus(pids, "ApplicationPaused");
52 }
53
54 /** Resumes this application by sending SIGCONT to all the PIDs in the
55@@ -284,16 +298,23 @@
56 void UpstartInstance::resume()
57 {
58 g_debug("Resuming application: %s", std::string(appId_).c_str());
59+
60+ auto registry = registry_;
61+ auto appid = appId_;
62+ auto jobpath = upstartJobPath();
63+
64+ registry->impl->thread.executeOnThread([registry, appid, jobpath] {
65+ auto pids = forAllPids(registry, appid, jobpath, [](pid_t pid) {
66+ auto oomval = oom::focused();
67+ g_debug("Resuming PID: %d (%d)", pid, int(oomval));
68+ signalToPid(pid, SIGCONT);
69+ oomValueToPid(pid, oomval);
70+ });
71+
72+ pidListToDbus(registry, appid, pids, "ApplicationResumed");
73+ });
74+
75 registry_->impl->zgSendEvent(appId_, ZEITGEIST_ZG_ACCESS_EVENT);
76-
77- auto pids = forAllPids([this](pid_t pid) {
78- auto oomval = oom::focused();
79- g_debug("Resuming PID: %d (%d)", pid, int(oomval));
80- signalToPid(pid, SIGCONT);
81- oomValueToPid(pid, oomval);
82- });
83-
84- pidListToDbus(pids, "ApplicationResumed");
85 }
86
87 /** Stops this instance by asking Upstart to stop it. Upstart will then
88@@ -365,7 +386,7 @@
89 */
90 void UpstartInstance::setOomAdjustment(const oom::Score score)
91 {
92- forAllPids([this, &score](pid_t pid) { oomValueToPid(pid, score); });
93+ forAllPids(registry_, appId_, upstartJobPath(), [score](pid_t pid) { oomValueToPid(pid, score); });
94 }
95
96 /** Figures out the path to the primary PID of the application and
97@@ -404,7 +425,10 @@
98
99 \param eachPid Function to run on each PID
100 */
101-std::vector<pid_t> UpstartInstance::forAllPids(std::function<void(pid_t)> eachPid)
102+std::vector<pid_t> UpstartInstance::forAllPids(const std::shared_ptr<Registry>& reg,
103+ const AppID& appid,
104+ const std::string& jobpath,
105+ std::function<void(pid_t)> eachPid)
106 {
107 std::set<pid_t> seenPids;
108 bool added = true;
109@@ -412,7 +436,7 @@
110 while (added)
111 {
112 added = false;
113- auto pidlist = pids();
114+ auto pidlist = pids(reg, appid, jobpath);
115 for (auto pid : pidlist)
116 {
117 if (seenPids.insert(pid).second)
118@@ -572,61 +596,59 @@
119 \param pids List of PIDs to turn into variants to send
120 \param signal Name of the DBus signal to send
121 */
122-void UpstartInstance::pidListToDbus(const std::vector<pid_t>& pids, const std::string& signal)
123+void UpstartInstance::pidListToDbus(const std::shared_ptr<Registry>& reg,
124+ const AppID& appid,
125+ const std::vector<pid_t>& pids,
126+ const std::string& signal)
127 {
128- auto registry = registry_;
129- auto lappid = appId_;
130-
131- registry_->impl->thread.executeOnThread([registry, lappid, pids, signal] {
132- auto vpids = std::shared_ptr<GVariant>(
133- [pids]() {
134- GVariant* pidarray = nullptr;
135-
136- if (pids.empty())
137- {
138- pidarray = g_variant_new_array(G_VARIANT_TYPE_UINT64, nullptr, 0);
139- g_variant_ref_sink(pidarray);
140- return pidarray;
141- }
142-
143- GVariantBuilder builder;
144- g_variant_builder_init(&builder, G_VARIANT_TYPE_ARRAY);
145- for (auto pid : pids)
146- {
147- g_variant_builder_add_value(&builder, g_variant_new_uint64(pid));
148- }
149-
150- pidarray = g_variant_builder_end(&builder);
151+ auto vpids = std::shared_ptr<GVariant>(
152+ [pids]() {
153+ GVariant* pidarray = nullptr;
154+
155+ if (pids.empty())
156+ {
157+ pidarray = g_variant_new_array(G_VARIANT_TYPE_UINT64, nullptr, 0);
158 g_variant_ref_sink(pidarray);
159 return pidarray;
160- }(),
161- [](GVariant* var) { g_variant_unref(var); });
162-
163- GVariantBuilder params;
164- g_variant_builder_init(&params, G_VARIANT_TYPE_TUPLE);
165- g_variant_builder_add_value(&params, g_variant_new_string(std::string(lappid).c_str()));
166- g_variant_builder_add_value(&params, vpids.get());
167-
168- GError* error = nullptr;
169- g_dbus_connection_emit_signal(registry->impl->_dbus.get(), /* bus */
170- nullptr, /* destination */
171- "/", /* path */
172- "com.canonical.UbuntuAppLaunch", /* interface */
173- signal.c_str(), /* signal */
174- g_variant_builder_end(&params), /* params, the same */
175- &error); /* error */
176-
177- if (error != nullptr)
178- {
179- g_warning("Unable to emit signal '%s' for appid '%s': %s", signal.c_str(), std::string(lappid).c_str(),
180- error->message);
181- g_error_free(error);
182- }
183- else
184- {
185- g_debug("Emmitted '%s' to DBus", signal.c_str());
186- }
187- });
188+ }
189+
190+ GVariantBuilder builder;
191+ g_variant_builder_init(&builder, G_VARIANT_TYPE_ARRAY);
192+ for (auto pid : pids)
193+ {
194+ g_variant_builder_add_value(&builder, g_variant_new_uint64(pid));
195+ }
196+
197+ pidarray = g_variant_builder_end(&builder);
198+ g_variant_ref_sink(pidarray);
199+ return pidarray;
200+ }(),
201+ [](GVariant* var) { g_variant_unref(var); });
202+
203+ GVariantBuilder params;
204+ g_variant_builder_init(&params, G_VARIANT_TYPE_TUPLE);
205+ g_variant_builder_add_value(&params, g_variant_new_string(std::string(appid).c_str()));
206+ g_variant_builder_add_value(&params, vpids.get());
207+
208+ GError* error = nullptr;
209+ g_dbus_connection_emit_signal(reg->impl->_dbus.get(), /* bus */
210+ nullptr, /* destination */
211+ "/", /* path */
212+ "com.canonical.UbuntuAppLaunch", /* interface */
213+ signal.c_str(), /* signal */
214+ g_variant_builder_end(&params), /* params, the same */
215+ &error); /* error */
216+
217+ if (error != nullptr)
218+ {
219+ g_warning("Unable to emit signal '%s' for appid '%s': %s", signal.c_str(), std::string(appid).c_str(),
220+ error->message);
221+ g_error_free(error);
222+ }
223+ else
224+ {
225+ g_debug("Emmitted '%s' to DBus", signal.c_str());
226+ }
227 }
228
229 /** Create a new Upstart Instance object that can track the job and
230
231=== modified file 'libubuntu-app-launch/application-impl-base.h'
232--- libubuntu-app-launch/application-impl-base.h 2016-09-23 22:30:51 +0000
233+++ libubuntu-app-launch/application-impl-base.h 2016-11-10 15:35:24 +0000
234@@ -108,14 +108,23 @@
235 /** A link to the registry we're using for connections */
236 std::shared_ptr<Registry> registry_;
237
238- std::vector<pid_t> forAllPids(std::function<void(pid_t)> eachPid);
239- void signalToPid(pid_t pid, int signal);
240- std::string pidToOomPath(pid_t pid);
241- void oomValueToPid(pid_t pid, const oom::Score oomvalue);
242- void oomValueToPidHelper(pid_t pid, const oom::Score oomvalue);
243- void pidListToDbus(const std::vector<pid_t>& pids, const std::string& signal);
244 std::string upstartJobPath();
245
246+ static std::vector<pid_t> forAllPids(const std::shared_ptr<Registry>& reg,
247+ const AppID& appid,
248+ const std::string& jobpath,
249+ std::function<void(pid_t)> eachPid);
250+ static std::vector<pid_t> pids(const std::shared_ptr<Registry>& reg,
251+ const AppID& appid,
252+ const std::string& jobpath);
253+ static void pidListToDbus(const std::shared_ptr<Registry>& reg,
254+ const AppID& appid,
255+ const std::vector<pid_t>& pids,
256+ const std::string& signal);
257+ static void signalToPid(pid_t pid, int signal);
258+ static void oomValueToPid(pid_t pid, const oom::Score oomvalue);
259+ static void oomValueToPidHelper(pid_t pid, const oom::Score oomvalue);
260+ static std::string pidToOomPath(pid_t pid);
261 static std::shared_ptr<gchar*> urlsToStrv(const std::vector<Application::URL>& urls);
262 static void application_start_cb(GObject* obj, GAsyncResult* res, gpointer user_data);
263 };
264
265=== modified file 'libubuntu-app-launch/application-impl-libertine.cpp'
266--- libubuntu-app-launch/application-impl-libertine.cpp 2016-10-03 23:54:20 +0000
267+++ libubuntu-app-launch/application-impl-libertine.cpp 2016-11-10 15:35:24 +0000
268@@ -255,8 +255,7 @@
269 }
270 catch (std::runtime_error& e)
271 {
272- g_debug("Unable to create application for libertine appname '%s': %s",
273- apps.get()[j], e.what());
274+ g_debug("Unable to create application for libertine appname '%s': %s", apps.get()[j], e.what());
275 }
276 }
277 }
278
279=== modified file 'libubuntu-app-launch/registry-impl.cpp'
280--- libubuntu-app-launch/registry-impl.cpp 2016-09-23 22:30:51 +0000
281+++ libubuntu-app-launch/registry-impl.cpp 2016-11-10 15:35:24 +0000
282@@ -222,17 +222,13 @@
283 if (cgManager_)
284 return;
285
286- std::promise<std::shared_ptr<GDBusConnection>> promise;
287- auto future = promise.get_future();
288-
289- thread.executeOnThread([this, &promise]() {
290+ cgManager_ = thread.executeOnThread<std::shared_ptr<GDBusConnection>>([this]() {
291 bool use_session_bus = g_getenv("UBUNTU_APP_LAUNCH_CG_MANAGER_SESSION_BUS") != nullptr;
292 if (use_session_bus)
293 {
294 /* For working dbusmock */
295 g_debug("Connecting to CG Manager on session bus");
296- promise.set_value(_dbus);
297- return;
298+ return _dbus;
299 }
300
301 auto cancel =
302@@ -241,28 +237,25 @@
303 /* Ensure that we do not wait for more than a second */
304 thread.timeoutSeconds(std::chrono::seconds{1}, [cancel]() { g_cancellable_cancel(cancel.get()); });
305
306- g_dbus_connection_new_for_address(
307- CGMANAGER_DBUS_PATH, /* cgmanager path */
308- G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT, /* flags */
309- nullptr, /* Auth Observer */
310- cancel.get(), /* Cancellable */
311- [](GObject* obj, GAsyncResult* res, gpointer data) -> void {
312- GError* error = nullptr;
313- auto promise = reinterpret_cast<std::promise<std::shared_ptr<GDBusConnection>>*>(data);
314-
315- auto gcon = g_dbus_connection_new_for_address_finish(res, &error);
316- if (error != nullptr)
317- {
318- g_error_free(error);
319- }
320-
321- auto con = std::shared_ptr<GDBusConnection>(gcon, [](GDBusConnection* con) { g_clear_object(&con); });
322- promise->set_value(con);
323- },
324- &promise);
325+ GError* error = nullptr;
326+ auto retval = std::shared_ptr<GDBusConnection>(
327+ g_dbus_connection_new_for_address_sync(CGMANAGER_DBUS_PATH, /* cgmanager path */
328+ G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT, /* flags */
329+ nullptr, /* Auth Observer */
330+ cancel.get(), /* Cancellable */
331+ &error),
332+ [](GDBusConnection* con) { g_clear_object(&con); });
333+
334+ if (error != nullptr)
335+ {
336+ g_warning("Unable to get CGManager connection: %s", error->message);
337+ g_error_free(error);
338+ }
339+
340+ return retval;
341 });
342
343- cgManager_ = future.get();
344+ /* NOTE: This will execute on the thread */
345 thread.timeoutSeconds(std::chrono::seconds{10}, [this]() { cgManager_.reset(); });
346 }
347
348
349=== modified file 'libubuntu-app-launch/snapd-info.cpp'
350--- libubuntu-app-launch/snapd-info.cpp 2016-10-03 23:54:08 +0000
351+++ libubuntu-app-launch/snapd-info.cpp 2016-11-10 15:35:24 +0000
352@@ -217,7 +217,7 @@
353 curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, snapd_writefunc);
354
355 /* Overridable timeout */
356- if (g_getenv("UBUNTU_APP_LAUNCH_DISABLE_SNAPD_TIMEOUT") != nullptr)
357+ if (g_getenv("UBUNTU_APP_LAUNCH_DISABLE_SNAPD_TIMEOUT") == nullptr)
358 {
359 curl_easy_setopt(curl, CURLOPT_TIMEOUT_MS, 100L);
360 }
361
362=== modified file 'tests/libual-cpp-test.cc'
363--- tests/libual-cpp-test.cc 2016-09-23 20:36:31 +0000
364+++ tests/libual-cpp-test.cc 2016-11-10 15:35:24 +0000
365@@ -1523,13 +1523,7 @@
366 }
367 };
368
369-static void signal_increment(GDBusConnection* connection,
370- const gchar* sender,
371- const gchar* path,
372- const gchar* interface,
373- const gchar* signal,
374- GVariant* params,
375- gpointer user_data)
376+static void signal_increment(const gchar* appid, GPid* pids, gpointer user_data)
377 {
378 guint* count = (guint*)user_data;
379 g_debug("Count incremented to: %d", *count + 1);
380@@ -1577,12 +1571,9 @@
381 /* Setup signal handling */
382 guint paused_count = 0;
383 guint resumed_count = 0;
384- guint paused_signal =
385- g_dbus_connection_signal_subscribe(bus, nullptr, "com.canonical.UbuntuAppLaunch", "ApplicationPaused", "/",
386- nullptr, G_DBUS_SIGNAL_FLAGS_NONE, signal_increment, &paused_count, nullptr);
387- guint resumed_signal = g_dbus_connection_signal_subscribe(
388- bus, nullptr, "com.canonical.UbuntuAppLaunch", "ApplicationResumed", "/", nullptr, G_DBUS_SIGNAL_FLAGS_NONE,
389- signal_increment, &resumed_count, nullptr);
390+
391+ ASSERT_TRUE(ubuntu_app_launch_observer_add_app_paused(signal_increment, &paused_count));
392+ ASSERT_TRUE(ubuntu_app_launch_observer_add_app_resumed(signal_increment, &resumed_count));
393
394 /* Get our app object */
395 auto appid = ubuntu::app_launch::AppID::find(registry, "com.test.good_application_1.2.3");
396@@ -1639,8 +1630,8 @@
397
398 g_spawn_command_line_sync("rm -rf " CMAKE_BINARY_DIR "/libual-proc", NULL, NULL, NULL, NULL);
399
400- g_dbus_connection_signal_unsubscribe(bus, paused_signal);
401- g_dbus_connection_signal_unsubscribe(bus, resumed_signal);
402+ ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_paused(signal_increment, &paused_count));
403+ ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_resumed(signal_increment, &resumed_count));
404 }
405
406 TEST_F(LibUAL, MultiPause)
407@@ -1686,10 +1677,17 @@
408 do
409 {
410 g_debug("Giving mocks a chance to start");
411- pause(200);
412+ pause(20);
413 } while (dbus_test_task_get_state(DBUS_TEST_TASK(cgmock2)) != DBUS_TEST_TASK_STATE_RUNNING &&
414 dbus_test_task_get_state(DBUS_TEST_TASK(zgmock)) != DBUS_TEST_TASK_STATE_RUNNING);
415
416+ /* Setup signal handling */
417+ guint paused_count = 0;
418+ guint resumed_count = 0;
419+
420+ ASSERT_TRUE(ubuntu_app_launch_observer_add_app_paused(signal_increment, &paused_count));
421+ ASSERT_TRUE(ubuntu_app_launch_observer_add_app_resumed(signal_increment, &resumed_count));
422+
423 /* Get our app object */
424 auto appid = ubuntu::app_launch::AppID::find(registry, "com.test.good_application_1.2.3");
425 auto app = ubuntu::app_launch::Application::create(appid, registry);
426@@ -1705,6 +1703,8 @@
427 /* Pause the app */
428 instance->pause();
429
430+ EXPECT_EVENTUALLY_EQ(1, paused_count);
431+
432 std::for_each(spews.begin(), spews.end(), [](SpewMaster& spew) { spew.reset(); });
433 pause(50);
434
435@@ -1715,6 +1715,8 @@
436 /* Now Resume the App */
437 instance->resume();
438
439+ EXPECT_EVENTUALLY_EQ(1, resumed_count);
440+
441 pause(50);
442
443 EXPECT_NE(0, std::accumulate(spews.begin(), spews.end(), int{0},
444@@ -1723,6 +1725,8 @@
445 /* Pause the app */
446 instance->pause();
447
448+ EXPECT_EVENTUALLY_EQ(2, paused_count);
449+
450 std::for_each(spews.begin(), spews.end(), [](SpewMaster& spew) { spew.reset(); });
451 pause(50);
452
453@@ -1733,12 +1737,17 @@
454 /* Now Resume the App */
455 instance->resume();
456
457+ EXPECT_EVENTUALLY_EQ(2, resumed_count);
458+
459 pause(50);
460
461 EXPECT_NE(0, std::accumulate(spews.begin(), spews.end(), int{0},
462 [](const int& acc, SpewMaster& spew) { return acc + spew.dataCnt(); }));
463
464 g_spawn_command_line_sync("rm -rf " CMAKE_BINARY_DIR "/libual-proc", NULL, NULL, NULL, NULL);
465+
466+ ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_paused(signal_increment, &paused_count));
467+ ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_resumed(signal_increment, &resumed_count));
468 }
469
470 TEST_F(LibUAL, OOMSet)

Subscribers

People subscribed via source and target branches