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

[Ted Gould]

Make resume/pause non-blocking

[lp:~ted/ubuntu-app-launch/non-blocking-resume]

274. By Ted Gould on 2016-11-09

Forgot to remove emit.h from the CMakeLists.txt

[Michał Sawicz (saviq)]

Kaboom: http://pastebin.ubuntu.com/23455077/

review: Needs Fixing

[lp:~ted/ubuntu-app-launch/non-blocking-resume]

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)]

This is the same as:

https://code.launchpad.net/~ted/ubuntu-app-launch/non-blocking-resume/+merge/310493

Just different target.

review: Approve

[Pat McGowan (pat-mcgowan)]

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

=== modified file 'libubuntu-app-launch/application-impl-base.cpp'
--- libubuntu-app-launch/application-impl-base.cpp	2016-10-03 23:54:20 +0000
+++ libubuntu-app-launch/application-impl-base.cpp	2016-11-10 15:35:24 +0000
@@ -257,8 +257,15 @@
 /** Returns all the PIDs that are in the cgroup for this application */
 std::vector<pid_t> UpstartInstance::pids()
 {
-    auto pids = registry_->impl->pidsFromCgroup(upstartJobPath());
-    g_debug("Got %d PIDs for AppID '%s'", int(pids.size()), std::string(appId_).c_str());
+    return pids(registry_, appId_, upstartJobPath());
+}
+
+std::vector<pid_t> UpstartInstance::pids(const std::shared_ptr<Registry>& reg,
+                                         const AppID& appid,
+                                         const std::string& jobpath)
+{
+    auto pids = reg->impl->pidsFromCgroup(jobpath);
+    g_debug("Got %d PIDs for AppID '%s'", int(pids.size()), std::string(appid).c_str());
     return pids;
 }
 
@@ -267,16 +274,23 @@
 void UpstartInstance::pause()
 {
     g_debug("Pausing application: %s", std::string(appId_).c_str());
+
+    auto registry = registry_;
+    auto appid = appId_;
+    auto jobpath = upstartJobPath();
+
+    registry->impl->thread.executeOnThread([registry, appid, jobpath] {
+        auto pids = forAllPids(registry, appid, jobpath, [](pid_t pid) {
+            auto oomval = oom::paused();
+            g_debug("Pausing PID: %d (%d)", pid, int(oomval));
+            signalToPid(pid, SIGSTOP);
+            oomValueToPid(pid, oomval);
+        });
+
+        pidListToDbus(registry, appid, pids, "ApplicationPaused");
+    });
+
     registry_->impl->zgSendEvent(appId_, ZEITGEIST_ZG_LEAVE_EVENT);
-
-    auto pids = forAllPids([this](pid_t pid) {
-        auto oomval = oom::paused();
-        g_debug("Pausing PID: %d (%d)", pid, int(oomval));
-        signalToPid(pid, SIGSTOP);
-        oomValueToPid(pid, oomval);
-    });
-
-    pidListToDbus(pids, "ApplicationPaused");
 }
 
 /** Resumes this application by sending SIGCONT to all the PIDs in the
@@ -284,16 +298,23 @@
 void UpstartInstance::resume()
 {
     g_debug("Resuming application: %s", std::string(appId_).c_str());
+
+    auto registry = registry_;
+    auto appid = appId_;
+    auto jobpath = upstartJobPath();
+
+    registry->impl->thread.executeOnThread([registry, appid, jobpath] {
+        auto pids = forAllPids(registry, appid, jobpath, [](pid_t pid) {
+            auto oomval = oom::focused();
+            g_debug("Resuming PID: %d (%d)", pid, int(oomval));
+            signalToPid(pid, SIGCONT);
+            oomValueToPid(pid, oomval);
+        });
+
+        pidListToDbus(registry, appid, pids, "ApplicationResumed");
+    });
+
     registry_->impl->zgSendEvent(appId_, ZEITGEIST_ZG_ACCESS_EVENT);
-
-    auto pids = forAllPids([this](pid_t pid) {
-        auto oomval = oom::focused();
-        g_debug("Resuming PID: %d (%d)", pid, int(oomval));
-        signalToPid(pid, SIGCONT);
-        oomValueToPid(pid, oomval);
-    });
-
-    pidListToDbus(pids, "ApplicationResumed");
 }
 
 /** Stops this instance by asking Upstart to stop it. Upstart will then
@@ -365,7 +386,7 @@
 */
 void UpstartInstance::setOomAdjustment(const oom::Score score)
 {
-    forAllPids([this, &score](pid_t pid) { oomValueToPid(pid, score); });
+    forAllPids(registry_, appId_, upstartJobPath(), [score](pid_t pid) { oomValueToPid(pid, score); });
 }
 
 /** Figures out the path to the primary PID of the application and
@@ -404,7 +425,10 @@
 
     \param eachPid Function to run on each PID
 */
-std::vector<pid_t> UpstartInstance::forAllPids(std::function<void(pid_t)> eachPid)
+std::vector<pid_t> UpstartInstance::forAllPids(const std::shared_ptr<Registry>& reg,
+                                               const AppID& appid,
+                                               const std::string& jobpath,
+                                               std::function<void(pid_t)> eachPid)
 {
     std::set<pid_t> seenPids;
     bool added = true;
@@ -412,7 +436,7 @@
     while (added)
     {
         added = false;
-        auto pidlist = pids();
+        auto pidlist = pids(reg, appid, jobpath);
         for (auto pid : pidlist)
         {
             if (seenPids.insert(pid).second)
@@ -572,61 +596,59 @@
     \param pids List of PIDs to turn into variants to send
     \param signal Name of the DBus signal to send
 */
-void UpstartInstance::pidListToDbus(const std::vector<pid_t>& pids, const std::string& signal)
+void UpstartInstance::pidListToDbus(const std::shared_ptr<Registry>& reg,
+                                    const AppID& appid,
+                                    const std::vector<pid_t>& pids,
+                                    const std::string& signal)
 {
-    auto registry = registry_;
-    auto lappid = appId_;
-
-    registry_->impl->thread.executeOnThread([registry, lappid, pids, signal] {
-        auto vpids = std::shared_ptr<GVariant>(
-            [pids]() {
-                GVariant* pidarray = nullptr;
-
-                if (pids.empty())
-                {
-                    pidarray = g_variant_new_array(G_VARIANT_TYPE_UINT64, nullptr, 0);
-                    g_variant_ref_sink(pidarray);
-                    return pidarray;
-                }
-
-                GVariantBuilder builder;
-                g_variant_builder_init(&builder, G_VARIANT_TYPE_ARRAY);
-                for (auto pid : pids)
-                {
-                    g_variant_builder_add_value(&builder, g_variant_new_uint64(pid));
-                }
-
-                pidarray = g_variant_builder_end(&builder);
+    auto vpids = std::shared_ptr<GVariant>(
+        [pids]() {
+            GVariant* pidarray = nullptr;
+
+            if (pids.empty())
+            {
+                pidarray = g_variant_new_array(G_VARIANT_TYPE_UINT64, nullptr, 0);
                 g_variant_ref_sink(pidarray);
                 return pidarray;
-            }(),
-            [](GVariant* var) { g_variant_unref(var); });
-
-        GVariantBuilder params;
-        g_variant_builder_init(&params, G_VARIANT_TYPE_TUPLE);
-        g_variant_builder_add_value(&params, g_variant_new_string(std::string(lappid).c_str()));
-        g_variant_builder_add_value(&params, vpids.get());
-
-        GError* error = nullptr;
-        g_dbus_connection_emit_signal(registry->impl->_dbus.get(),     /* bus */
-                                      nullptr,                         /* destination */
-                                      "/",                             /* path */
-                                      "com.canonical.UbuntuAppLaunch", /* interface */
-                                      signal.c_str(),                  /* signal */
-                                      g_variant_builder_end(&params),  /* params, the same */
-                                      &error);                         /* error */
-
-        if (error != nullptr)
-        {
-            g_warning("Unable to emit signal '%s' for appid '%s': %s", signal.c_str(), std::string(lappid).c_str(),
-                      error->message);
-            g_error_free(error);
-        }
-        else
-        {
-            g_debug("Emmitted '%s' to DBus", signal.c_str());
-        }
-    });
+            }
+
+            GVariantBuilder builder;
+            g_variant_builder_init(&builder, G_VARIANT_TYPE_ARRAY);
+            for (auto pid : pids)
+            {
+                g_variant_builder_add_value(&builder, g_variant_new_uint64(pid));
+            }
+
+            pidarray = g_variant_builder_end(&builder);
+            g_variant_ref_sink(pidarray);
+            return pidarray;
+        }(),
+        [](GVariant* var) { g_variant_unref(var); });
+
+    GVariantBuilder params;
+    g_variant_builder_init(&params, G_VARIANT_TYPE_TUPLE);
+    g_variant_builder_add_value(&params, g_variant_new_string(std::string(appid).c_str()));
+    g_variant_builder_add_value(&params, vpids.get());
+
+    GError* error = nullptr;
+    g_dbus_connection_emit_signal(reg->impl->_dbus.get(),          /* bus */
+                                  nullptr,                         /* destination */
+                                  "/",                             /* path */
+                                  "com.canonical.UbuntuAppLaunch", /* interface */
+                                  signal.c_str(),                  /* signal */
+                                  g_variant_builder_end(&params),  /* params, the same */
+                                  &error);                         /* error */
+
+    if (error != nullptr)
+    {
+        g_warning("Unable to emit signal '%s' for appid '%s': %s", signal.c_str(), std::string(appid).c_str(),
+                  error->message);
+        g_error_free(error);
+    }
+    else
+    {
+        g_debug("Emmitted '%s' to DBus", signal.c_str());
+    }
 }
 
 /** Create a new Upstart Instance object that can track the job and

=== modified file 'libubuntu-app-launch/application-impl-base.h'
--- libubuntu-app-launch/application-impl-base.h	2016-09-23 22:30:51 +0000
+++ libubuntu-app-launch/application-impl-base.h	2016-11-10 15:35:24 +0000
@@ -108,14 +108,23 @@
     /** A link to the registry we're using for connections */
     std::shared_ptr<Registry> registry_;
 
-    std::vector<pid_t> forAllPids(std::function<void(pid_t)> eachPid);
-    void signalToPid(pid_t pid, int signal);
-    std::string pidToOomPath(pid_t pid);
-    void oomValueToPid(pid_t pid, const oom::Score oomvalue);
-    void oomValueToPidHelper(pid_t pid, const oom::Score oomvalue);
-    void pidListToDbus(const std::vector<pid_t>& pids, const std::string& signal);
     std::string upstartJobPath();
 
+    static std::vector<pid_t> forAllPids(const std::shared_ptr<Registry>& reg,
+                                         const AppID& appid,
+                                         const std::string& jobpath,
+                                         std::function<void(pid_t)> eachPid);
+    static std::vector<pid_t> pids(const std::shared_ptr<Registry>& reg,
+                                   const AppID& appid,
+                                   const std::string& jobpath);
+    static void pidListToDbus(const std::shared_ptr<Registry>& reg,
+                              const AppID& appid,
+                              const std::vector<pid_t>& pids,
+                              const std::string& signal);
+    static void signalToPid(pid_t pid, int signal);
+    static void oomValueToPid(pid_t pid, const oom::Score oomvalue);
+    static void oomValueToPidHelper(pid_t pid, const oom::Score oomvalue);
+    static std::string pidToOomPath(pid_t pid);
     static std::shared_ptr<gchar*> urlsToStrv(const std::vector<Application::URL>& urls);
     static void application_start_cb(GObject* obj, GAsyncResult* res, gpointer user_data);
 };

=== modified file 'libubuntu-app-launch/application-impl-libertine.cpp'
--- libubuntu-app-launch/application-impl-libertine.cpp	2016-10-03 23:54:20 +0000
+++ libubuntu-app-launch/application-impl-libertine.cpp	2016-11-10 15:35:24 +0000
@@ -255,8 +255,7 @@
             }
             catch (std::runtime_error& e)
             {
-                g_debug("Unable to create application for libertine appname '%s': %s",
-                        apps.get()[j], e.what());
+                g_debug("Unable to create application for libertine appname '%s': %s", apps.get()[j], e.what());
             }
         }
     }

=== modified file 'libubuntu-app-launch/registry-impl.cpp'
--- libubuntu-app-launch/registry-impl.cpp	2016-09-23 22:30:51 +0000
+++ libubuntu-app-launch/registry-impl.cpp	2016-11-10 15:35:24 +0000
@@ -222,17 +222,13 @@
     if (cgManager_)
         return;
 
-    std::promise<std::shared_ptr<GDBusConnection>> promise;
-    auto future = promise.get_future();
-
-    thread.executeOnThread([this, &promise]() {
+    cgManager_ = thread.executeOnThread<std::shared_ptr<GDBusConnection>>([this]() {
         bool use_session_bus = g_getenv("UBUNTU_APP_LAUNCH_CG_MANAGER_SESSION_BUS") != nullptr;
         if (use_session_bus)
         {
             /* For working dbusmock */
             g_debug("Connecting to CG Manager on session bus");
-            promise.set_value(_dbus);
-            return;
+            return _dbus;
         }
 
         auto cancel =
@@ -241,28 +237,25 @@
         /* Ensure that we do not wait for more than a second */
         thread.timeoutSeconds(std::chrono::seconds{1}, [cancel]() { g_cancellable_cancel(cancel.get()); });
 
-        g_dbus_connection_new_for_address(
-            CGMANAGER_DBUS_PATH,                           /* cgmanager path */
-            G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT, /* flags */
-            nullptr,                                       /* Auth Observer */
-            cancel.get(),                                  /* Cancellable */
-            [](GObject* obj, GAsyncResult* res, gpointer data) -> void {
-                GError* error = nullptr;
-                auto promise = reinterpret_cast<std::promise<std::shared_ptr<GDBusConnection>>*>(data);
-
-                auto gcon = g_dbus_connection_new_for_address_finish(res, &error);
-                if (error != nullptr)
-                {
-                    g_error_free(error);
-                }
-
-                auto con = std::shared_ptr<GDBusConnection>(gcon, [](GDBusConnection* con) { g_clear_object(&con); });
-                promise->set_value(con);
-            },
-            &promise);
+        GError* error = nullptr;
+        auto retval = std::shared_ptr<GDBusConnection>(
+            g_dbus_connection_new_for_address_sync(CGMANAGER_DBUS_PATH,                           /* cgmanager path */
+                                                   G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT, /* flags */
+                                                   nullptr,                                       /* Auth Observer */
+                                                   cancel.get(),                                  /* Cancellable */
+                                                   &error),
+            [](GDBusConnection* con) { g_clear_object(&con); });
+
+        if (error != nullptr)
+        {
+            g_warning("Unable to get CGManager connection: %s", error->message);
+            g_error_free(error);
+        }
+
+        return retval;
     });
 
-    cgManager_ = future.get();
+    /* NOTE: This will execute on the thread */
     thread.timeoutSeconds(std::chrono::seconds{10}, [this]() { cgManager_.reset(); });
 }
 

=== modified file 'libubuntu-app-launch/snapd-info.cpp'
--- libubuntu-app-launch/snapd-info.cpp	2016-10-03 23:54:08 +0000
+++ libubuntu-app-launch/snapd-info.cpp	2016-11-10 15:35:24 +0000
@@ -217,7 +217,7 @@
     curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, snapd_writefunc);
 
     /* Overridable timeout */
-    if (g_getenv("UBUNTU_APP_LAUNCH_DISABLE_SNAPD_TIMEOUT") != nullptr)
+    if (g_getenv("UBUNTU_APP_LAUNCH_DISABLE_SNAPD_TIMEOUT") == nullptr)
     {
         curl_easy_setopt(curl, CURLOPT_TIMEOUT_MS, 100L);
     }

=== modified file 'tests/libual-cpp-test.cc'
--- tests/libual-cpp-test.cc	2016-09-23 20:36:31 +0000
+++ tests/libual-cpp-test.cc	2016-11-10 15:35:24 +0000
@@ -1523,13 +1523,7 @@
     }
 };
 
-static void signal_increment(GDBusConnection* connection,
-                             const gchar* sender,
-                             const gchar* path,
-                             const gchar* interface,
-                             const gchar* signal,
-                             GVariant* params,
-                             gpointer user_data)
+static void signal_increment(const gchar* appid, GPid* pids, gpointer user_data)
 {
     guint* count = (guint*)user_data;
     g_debug("Count incremented to: %d", *count + 1);
@@ -1577,12 +1571,9 @@
     /* Setup signal handling */
     guint paused_count = 0;
     guint resumed_count = 0;
-    guint paused_signal =
-        g_dbus_connection_signal_subscribe(bus, nullptr, "com.canonical.UbuntuAppLaunch", "ApplicationPaused", "/",
-                                           nullptr, G_DBUS_SIGNAL_FLAGS_NONE, signal_increment, &paused_count, nullptr);
-    guint resumed_signal = g_dbus_connection_signal_subscribe(
-        bus, nullptr, "com.canonical.UbuntuAppLaunch", "ApplicationResumed", "/", nullptr, G_DBUS_SIGNAL_FLAGS_NONE,
-        signal_increment, &resumed_count, nullptr);
+
+    ASSERT_TRUE(ubuntu_app_launch_observer_add_app_paused(signal_increment, &paused_count));
+    ASSERT_TRUE(ubuntu_app_launch_observer_add_app_resumed(signal_increment, &resumed_count));
 
     /* Get our app object */
     auto appid = ubuntu::app_launch::AppID::find(registry, "com.test.good_application_1.2.3");
@@ -1639,8 +1630,8 @@
 
     g_spawn_command_line_sync("rm -rf " CMAKE_BINARY_DIR "/libual-proc", NULL, NULL, NULL, NULL);
 
-    g_dbus_connection_signal_unsubscribe(bus, paused_signal);
-    g_dbus_connection_signal_unsubscribe(bus, resumed_signal);
+    ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_paused(signal_increment, &paused_count));
+    ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_resumed(signal_increment, &resumed_count));
 }
 
 TEST_F(LibUAL, MultiPause)
@@ -1686,10 +1677,17 @@
     do
     {
         g_debug("Giving mocks a chance to start");
-        pause(200);
+        pause(20);
     } while (dbus_test_task_get_state(DBUS_TEST_TASK(cgmock2)) != DBUS_TEST_TASK_STATE_RUNNING &&
              dbus_test_task_get_state(DBUS_TEST_TASK(zgmock)) != DBUS_TEST_TASK_STATE_RUNNING);
 
+    /* Setup signal handling */
+    guint paused_count = 0;
+    guint resumed_count = 0;
+
+    ASSERT_TRUE(ubuntu_app_launch_observer_add_app_paused(signal_increment, &paused_count));
+    ASSERT_TRUE(ubuntu_app_launch_observer_add_app_resumed(signal_increment, &resumed_count));
+
     /* Get our app object */
     auto appid = ubuntu::app_launch::AppID::find(registry, "com.test.good_application_1.2.3");
     auto app = ubuntu::app_launch::Application::create(appid, registry);
@@ -1705,6 +1703,8 @@
     /* Pause the app */
     instance->pause();
 
+    EXPECT_EVENTUALLY_EQ(1, paused_count);
+
     std::for_each(spews.begin(), spews.end(), [](SpewMaster& spew) { spew.reset(); });
     pause(50);
 
@@ -1715,6 +1715,8 @@
     /* Now Resume the App */
     instance->resume();
 
+    EXPECT_EVENTUALLY_EQ(1, resumed_count);
+
     pause(50);
 
     EXPECT_NE(0, std::accumulate(spews.begin(), spews.end(), int{0},
@@ -1723,6 +1725,8 @@
     /* Pause the app */
     instance->pause();
 
+    EXPECT_EVENTUALLY_EQ(2, paused_count);
+
     std::for_each(spews.begin(), spews.end(), [](SpewMaster& spew) { spew.reset(); });
     pause(50);
 
@@ -1733,12 +1737,17 @@
     /* Now Resume the App */
     instance->resume();
 
+    EXPECT_EVENTUALLY_EQ(2, resumed_count);
+
     pause(50);
 
     EXPECT_NE(0, std::accumulate(spews.begin(), spews.end(), int{0},
                                  [](const int& acc, SpewMaster& spew) { return acc + spew.dataCnt(); }));
 
     g_spawn_command_line_sync("rm -rf " CMAKE_BINARY_DIR "/libual-proc", NULL, NULL, NULL, NULL);
+
+    ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_paused(signal_increment, &paused_count));
+    ASSERT_TRUE(ubuntu_app_launch_observer_delete_app_resumed(signal_increment, &resumed_count));
 }
 
 TEST_F(LibUAL, OOMSet)