Ubuntu Application Launcher

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

non-blocking-resume
Merge into trunk.17.04

Proposed by Ted Gould on 2016-11-09

Status:

Merged

Approved by:

Charles Kerr on 2016-11-10

Approved revision:

275

Merged at revision:

266

Proposed branch:

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

Merge into:

lp:ubuntu-app-launch

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

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Charles Kerr (community)			Approve on 2016-11-10
unity-api-1-bot	continuous-integration		Approve on 2016-11-10
Michał Sawicz		2016-11-09	Needs Fixing on 2016-11-10
Review via email: mp+310493@code.launchpad.net

Commit message

Make pause/resume non-blocking

lp:~ted/ubuntu-app-launch/non-blocking-resume updated on 2016-11-09

274. By Ted Gould on 2016-11-09: Forgot to remove emit.h from the CMakeLists.txt

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2016-11-10:

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/115/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2016-11-10:

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

review: Needs Fixing

lp:~ted/ubuntu-app-launch/non-blocking-resume updated on 2016-11-10

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

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2016-11-10:

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/118/rebuild

review: Approve (continuous-integration)

Revision history for this message

Charles Kerr (charlesk) wrote on 2016-11-10:

LGTM, code all makes sense.

Approved if the captures and static change in r275 fixes the crash that Saviq's seeing

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Indicator Applet Developers

Ted Gould

 === 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:20 +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:20 +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:20 +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:20 +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:20 +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:20 +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)

Ubuntu Application Launcher

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

Commit message

Description of the change

Preview Diff

Subscribers