Merge into trunk.16.10 : adbd-client-test-failure : Code : Indicator Display

Status:	Merged
Approved by:	dobey on 2016-10-21
Approved revision:	40
Merged at revision:	21
Proposed branch:	lp:~charlesk/indicator-display/adbd-client-test-failure
Merge into:	lp:indicator-display
Diff against target:	1012 lines (+474/-170) 14 files modified src/adbd-client.cpp (+16/-9) src/greeter.cpp (+132/-82) src/greeter.h (+8/-2) src/usb-manager.cpp (+65/-48) src/usb-snap.cpp (+1/-0) tests/integration/usb-manager-test.cpp (+11/-5) tests/unit/CMakeLists.txt (+5/-0) tests/unit/adbd-client-test.cpp (+3/-2) tests/unit/greeter-test.cpp (+159/-0) tests/unit/usb-snap-test.cpp (+4/-9) tests/utils/adbd-server.h (+9/-11) tests/utils/gtest-print-helpers.h (+18/-0) tests/utils/mock-greeter.h (+2/-2) tests/utils/mock-unity-greeter.py (+41/-0)
To merge this branch:	bzr merge lp:~charlesk/indicator-display/adbd-client-test-failure
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
dobey (community)		2016-09-21	Approve on 2016-10-21
unity-api-1-bot	continuous-integration		Approve on 2016-10-21
Review via email: mp+306399@code.launchpad.net

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-21:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/4/rebuild

review: Needs Fixing (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-21

21. By Charles Kerr on 2016-09-21: in PKIdleData, take the std::string argument as a const& to make cppcheck happy on yakkety

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-21:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/6/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

22. By Charles Kerr on 2016-09-22: in adbd-client's dtor, check the worker thread's joinable() state before calling join() on it

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/5/rebuild

review: Needs Fixing (continuous-integration)

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/7/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

23. By Charles Kerr on 2016-09-22: adbd-client's std::condition_variable::wait() call seems to be the system_error culprit, so wrapping in try-catch to be cautious

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/8/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

24. By Charles Kerr on 2016-09-22: add a plethora of log statements to help figure out what's causing the silo test failures

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/9/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

25. By Charles Kerr on 2016-09-22: GAdbdClient::Impl::on_public_key_response() doesn't need a condition variable wait, so use std::lock_guard rather than std::unique_lock

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/10/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

26. By Charles Kerr on 2016-09-22: fix r25 copypaste error, lock on the correct mutex
27. By Charles Kerr on 2016-09-22: The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization.

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/11/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

28. By Charles Kerr on 2016-09-22: more g_debug() tracers

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/12/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

29. By Charles Kerr on 2016-09-22: in AdbdClientDest, explicitly disconnect the core::Signals at the end of the test
30. By Charles Kerr on 2016-09-22: in AdbdClientTest, raise the test timeout interval from 2s to 5s
31. By Charles Kerr on 2016-09-22: in UsbSnapTest, explicitly disconnect the core::Signals at the end of the test

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/13/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

32. By Charles Kerr on 2016-09-22: more g_debug() tracers

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/14/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

33. By Charles Kerr on 2016-09-22: tweak the debug tracers
34. By Charles Kerr on 2016-09-22: keep piling 'em on

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/15/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

35. By Charles Kerr on 2016-09-22: sync with lp:~charlesk/indicator-display/lp-1572545-prompt-in-lockscreen
36. By Charles Kerr on 2016-09-22: fix tyop

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/17/rebuild

review: Needs Fixing (continuous-integration)

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/18/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-09-22

37. By Charles Kerr on 2016-09-22: use a std::atomic for adbd-client's m_pkresponse_ready

unity-api-1-bot (unity-api-1-bot) wrote on 2016-09-22:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/19/rebuild

review: Approve (continuous-integration)

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-10-19

38. By Charles Kerr on 2016-10-19: remove the temporary tracers from r34 and r35
39. By Charles Kerr on 2016-10-19: remove dead code

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

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/20/rebuild

review: Approve (continuous-integration)

dobey (dobey) wrote on 2016-10-20:

#

Generally looks OK to me, but I noticed many uses of if or if/else without curly braces around a single result statement. I think it'd be better to use braces or convert these statements to the ternary operator.

lp:~charlesk/indicator-display/adbd-client-test-failure updated on 2016-10-21

40. By Charles Kerr on 2016-10-21: add block braces as suggested by dobey

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

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-display-ci/21/rebuild

review: Approve (continuous-integration)

dobey (dobey) wrote on 2016-10-21:

#

LGTM. Ship it.

review: Approve

Indicator Display

Merge lp:~charlesk/indicator-display/adbd-client-test-failure into lp:indicator-display

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'src/adbd-client.cpp'
 --- src/adbd-client.cpp	2016-03-24 16:01:16 +0000
 +++ src/adbd-client.cpp	2016-10-21 11:40:57 +0000
@@ -23,6 +23,7 @@
  #include <gio/gunixsocketaddress.h>
  #include <algorithm>
++#include <atomic>
  #include <cctype>
  #include <cstring>
  #include <chrono>
@@ -48,7 +49,9 @@
          g_cancellable_cancel(m_cancellable);
          m_pkresponse_cv.notify_one();
          m_sleep_cv.notify_one();
--        m_worker_thread.join();
++        if (m_worker_thread.joinable()) {
++            m_worker_thread.join();
++        }
          g_clear_object(&m_cancellable);
+     }
@@ -66,7 +69,7 @@
          GCancellable* cancellable = nullptr;
          const std::string public_key;
--        PKIdleData(Impl* self_, GCancellable* cancellable_, std::string public_key_):
++        PKIdleData(Impl* self_, GCancellable* cancellable_, const std::string& public_key_):
              self(self_),
              cancellable(G_CANCELLABLE(g_object_ref(cancellable_))),
              public_key(public_key_) {}
@@ -104,8 +107,9 @@
      void on_public_key_response(PKResponse response)
+     {
++        g_debug("%s thread %p got response %d", G_STRLOC, g_thread_self(), int(response));
++
          // set m_pkresponse and wake up the waiting worker thread
--        std::unique_lock<std::mutex> lk(m_pkresponse_mutex);
          m_pkresponse = response;
          m_pkresponse_ready = true;
          m_pkresponse_cv.notify_one();
@@ -121,11 +125,11 @@
          while (!g_cancellable_is_cancelled(m_cancellable))
+         {
--            g_debug("%s creating a client socket to '%s'", G_STRLOC, socket_path.c_str());
++            g_debug("%s thread %p creating a client socket to '%s'", G_STRLOC, g_thread_self(), socket_path.c_str());
              auto socket = create_client_socket(socket_path);
              bool got_valid_req = false;
--            g_debug("%s calling read_request", G_STRLOC);
++            g_debug("%s thread %p calling read_request", g_thread_self(), G_STRLOC);
              std::string reqstr;
              if (socket != nullptr)
                  reqstr = read_request(socket);
@@ -135,22 +139,25 @@
              if (reqstr.substr(0,2) == "PK") {
                  PKResponse response = PKResponse::DENY;
                  const auto public_key = reqstr.substr(2);
--                g_debug("%s got pk [%s]", G_STRLOC, public_key.c_str());
++                g_debug("%s thread %p got pk [%s]", G_STRLOC, g_thread_self(), public_key.c_str());
                  if (!public_key.empty()) {
                      got_valid_req = true;
                      std::unique_lock<std::mutex> lk(m_pkresponse_mutex);
                      m_pkresponse_ready = false;
++                    m_pkresponse = AdbdClient::PKResponse::DENY;
                      pass_public_key_to_main_thread(public_key);
                      m_pkresponse_cv.wait(lk, [this](){
                          return m_pkresponse_ready || g_cancellable_is_cancelled(m_cancellable);
                      });
                      response = m_pkresponse;
--                    g_debug("%s got response '%d', is-cancelled %d", G_STRLOC,
++                    g_debug("%s thread %p got response '%d', is-cancelled %d", G_STRLOC,
++                            g_thread_self(),
                              int(response),
                              int(g_cancellable_is_cancelled(m_cancellable)));
+                 }
--                if (!g_cancellable_is_cancelled(m_cancellable))
++                if (!g_cancellable_is_cancelled(m_cancellable)) {
                      send_pk_response(socket, response);
++                }
              } else if (!reqstr.empty()) {
                  g_warning("Invalid ADB request: [%s]", reqstr.c_str());
+             }
@@ -270,7 +277,7 @@
      std::mutex m_pkresponse_mutex;
      std::condition_variable m_pkresponse_cv;
--    bool m_pkresponse_ready = false;
++    std::atomic<bool> m_pkresponse_ready {false};
      PKResponse m_pkresponse = PKResponse::DENY;
  };
 === modified file 'src/greeter.cpp'
 --- src/greeter.cpp	2016-03-22 13:15:38 +0000
 +++ src/greeter.cpp	2016-10-21 11:40:57 +0000
@@ -26,117 +26,167 @@
+ {
  public:
--    Impl():
--        m_cancellable{g_cancellable_new()}
--    {
--        g_bus_get(G_BUS_TYPE_SESSION, m_cancellable, on_bus_ready_static, this);
--    }
--
--    ~Impl()
--    {
--        g_cancellable_cancel(m_cancellable);
--        g_clear_object(&m_cancellable);
--
--        if (m_subscription_id != 0)
--            g_dbus_connection_signal_unsubscribe (m_bus, m_subscription_id);
--
--        g_clear_object(&m_bus);
--    }
--
--    core::Property<bool>& is_active()
--    {
--        return m_is_active;
++    Impl()
++    {
++        m_cancellable.reset(
++            g_cancellable_new(),
++            [](GCancellable* o){
++                g_cancellable_cancel(o);
++                g_clear_object(&o);
++            }
++        );
++
++        g_bus_get(G_BUS_TYPE_SESSION, m_cancellable.get(), on_bus_ready, this);
++    }
++
++    ~Impl() =default;
++
++    core::Property<State>& state()
++    {
++        return m_state;
+     }
  private:
--    static void on_bus_ready_static(GObject* /*source*/, GAsyncResult* res, gpointer gself)
++    void set_state(const State& state)
++    {
++        m_state.set(state);
++    }
++
++    static void on_bus_ready(
++        GObject* /*source*/,
++        GAsyncResult* res,
++        gpointer gself)
+     {
          GError* error {};
--        auto bus = g_bus_get_finish (res, &error);
--        if (error != nullptr) {
++        auto bus = g_bus_get_finish(res, &error);
++        if (error != nullptr)
++        {
              if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
--                g_warning("UsbSnap: Error getting session bus: %s", error->message);
++                g_warning("Greeter: Error getting bus: %s", error->message);
              g_clear_error(&error);
--        } else {
--            static_cast<Impl*>(gself)->on_bus_ready(bus);
--        }
--        g_clear_object(&bus);
--    }
--
--    void on_bus_ready(GDBusConnection* bus)
--    {
--        m_bus = G_DBUS_CONNECTION(g_object_ref(G_OBJECT(bus)));
--
--        g_dbus_connection_call(m_bus,
--                               DBusNames::UnityGreeter::NAME,
--                               DBusNames::UnityGreeter::PATH,
--                               DBusNames::Properties::INTERFACE,
--                               "Get",
--                               g_variant_new("(ss)", DBusNames::UnityGreeter::INTERFACE, "IsActive"),
--                               G_VARIANT_TYPE("(v)"),
--                               G_DBUS_CALL_FLAGS_NONE,
--                               -1,
--                               m_cancellable,
--                               on_get_is_active_ready,
--                               this);
--
--        m_subscription_id = g_dbus_connection_signal_subscribe(m_bus,
--                                                               DBusNames::UnityGreeter::NAME,
--                                                               DBusNames::Properties::INTERFACE,
--                                                               DBusNames::Properties::PropertiesChanged::NAME,
--                                                               DBusNames::UnityGreeter::PATH,
--                                                               DBusNames::UnityGreeter::INTERFACE,
--                                                               G_DBUS_SIGNAL_FLAGS_NONE,
--                                                               on_properties_changed_signal,
--                                                               this,
--                                                               nullptr);
--    }
--
--    static void on_get_is_active_ready(GObject* source, GAsyncResult* res, gpointer gself)
++        }
++        else
++        {
++            auto self = static_cast<Impl*>(gself);
++
++            const auto watcher_id = g_bus_watch_name_on_connection(
++                bus,
++                DBusNames::UnityGreeter::NAME,
++                G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
++                on_greeter_appeared,
++                on_greeter_vanished,
++                gself,
++                nullptr);
++
++            const auto subscription_id = g_dbus_connection_signal_subscribe(
++                bus,
++                DBusNames::UnityGreeter::NAME,
++                DBusNames::Properties::INTERFACE,
++                DBusNames::Properties::PropertiesChanged::NAME,
++                DBusNames::UnityGreeter::PATH,
++                DBusNames::UnityGreeter::INTERFACE,
++                G_DBUS_SIGNAL_FLAGS_NONE,
++                on_properties_changed_signal,
++                gself,
++                nullptr);
++
++            self->m_bus.reset(
++                bus,
++                [watcher_id, subscription_id](GDBusConnection* o){
++                    g_bus_unwatch_name(watcher_id);
++                    g_dbus_connection_signal_unsubscribe(o, subscription_id);
++                    g_clear_object(&o);
++                }
++            );
++        }
++    }
++
++    static void on_greeter_appeared(
++        GDBusConnection* bus,
++        const char* /*name*/,
++        const char* name_owner,
++        gpointer gself)
++    {
++        auto self = static_cast<Impl*>(gself);
++
++        self->m_owner = name_owner;
++
++        g_dbus_connection_call(
++            bus,
++            DBusNames::UnityGreeter::NAME,
++            DBusNames::UnityGreeter::PATH,
++            DBusNames::Properties::INTERFACE,
++            "Get",
++            g_variant_new("(ss)", DBusNames::UnityGreeter::INTERFACE, "IsActive"),
++            G_VARIANT_TYPE("(v)"),
++            G_DBUS_CALL_FLAGS_NONE,
++            -1,
++            self->m_cancellable.get(),
++            on_get_is_active_ready,
++            gself);
++    }
++
++    static void on_greeter_vanished(
++        GDBusConnection* /*bus*/,
++        const char* /*name*/,
++        gpointer gself)
++    {
++        auto self = static_cast<Impl*>(gself);
++
++        self->m_owner.clear();
++        self->set_state(State::UNAVAILABLE);
++    }
++
++    static void on_get_is_active_ready(
++        GObject* source,
++        GAsyncResult* res,
++        gpointer gself)
+     {
          GError* error {};
          auto v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), res, &error);
          if (error != nullptr) {
--            if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
--                g_warning("UsbSnap: Error getting session bus: %s", error->message);
++            if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
++                g_warning("Greeter: Error getting IsActive property: %s", error->message);
++            }
              g_clear_error(&error);
          } else {
              GVariant* is_active {};
              g_variant_get_child(v, 0, "v", &is_active);
--            static_cast<Impl*>(gself)->m_is_active.set(g_variant_get_boolean(is_active));
++            static_cast<Impl*>(gself)->set_state(g_variant_get_boolean(is_active) ? State::ACTIVE : State::INACTIVE);
              g_clear_pointer(&is_active, g_variant_unref);
+         }
          g_clear_pointer(&v, g_variant_unref);
+     }
--    static void on_properties_changed_signal(GDBusConnection* /*connection*/,
--                                             const gchar* /*sender_name*/,
--                                             const gchar* object_path,
--                                             const gchar* interface_name,
--                                             const gchar* signal_name,
--                                             GVariant* parameters,
--                                             gpointer gself)
++    static void on_properties_changed_signal(
++        GDBusConnection* /*bus*/,
++        const gchar* sender_name,
++        const gchar* object_path,
++        const gchar* interface_name,
++        const gchar* signal_name,
++        GVariant* parameters,
++        gpointer gself)
+     {
++        auto self = static_cast<Impl*>(gself);
++
++        g_return_if_fail(!g_strcmp0(sender_name, self->m_owner.c_str()));
          g_return_if_fail(!g_strcmp0(object_path, DBusNames::UnityGreeter::PATH));
          g_return_if_fail(!g_strcmp0(interface_name, DBusNames::Properties::INTERFACE));
          g_return_if_fail(!g_strcmp0(signal_name, DBusNames::Properties::PropertiesChanged::NAME));
          g_return_if_fail(g_variant_is_of_type(parameters, G_VARIANT_TYPE(DBusNames::Properties::PropertiesChanged::ARGS_VARIANT_TYPE)));
--        auto v = g_variant_get_child_value (parameters, 1);
++        auto v = g_variant_get_child_value(parameters, 1);
          gboolean is_active {};
          if (g_variant_lookup(v, "IsActive", "b", &is_active))
--        {
--            g_debug("%s is_active changed to %d", G_STRLOC, int(is_active));
--            static_cast<Impl*>(gself)->m_is_active.set(is_active);
--        }
++            self->set_state(is_active ? State::ACTIVE : State::INACTIVE);
          g_clear_pointer(&v, g_variant_unref);
+     }
--    core::Property<bool> m_is_active;
--    GCancellable* m_cancellable {};
--    GDBusConnection* m_bus {};
--    unsigned int m_subscription_id {};
++    core::Property<State> m_state {State::UNAVAILABLE};
++    std::shared_ptr<GCancellable> m_cancellable;
++    std::shared_ptr<GDBusConnection> m_bus;
++    std::string m_owner;
  };
  /***
@@ -154,8 +204,8 @@
  UnityGreeter::~UnityGreeter() =default;
--core::Property<bool>&
--UnityGreeter::is_active()
++core::Property<Greeter::State>&
++UnityGreeter::state()
+ {
--    return impl->is_active();
++    return impl->state();
+ }
 === modified file 'src/greeter.h'
 --- src/greeter.h	2016-03-21 23:15:05 +0000
 +++ src/greeter.h	2016-10-21 11:40:57 +0000
@@ -29,7 +29,13 @@
  public:
      Greeter();
      virtual ~Greeter();
--    virtual core::Property<bool>& is_active() =0;
++
++    enum class State { UNAVAILABLE, INACTIVE, ACTIVE };
++static inline const char* state_str(const State& state) {
++    static constexpr char const * state_str[] = { "Unavailable", "Inactive", "Active" };
++    return state_str[int(state)];
++}
++    virtual core::Property<State>& state() =0;
  };
@@ -38,7 +44,7 @@
  public:
      UnityGreeter();
      virtual ~UnityGreeter();
--    core::Property<bool>& is_active() override;
++    core::Property<State>& state() override;
  protected:
      class Impl;
 === modified file 'src/usb-manager.cpp'
 --- src/usb-manager.cpp	2016-03-23 19:44:23 +0000
 +++ src/usb-manager.cpp	2016-10-21 11:40:57 +0000
@@ -46,72 +46,89 @@
          m_greeter{greeter}
+     {
          m_usb_monitor->on_usb_disconnected().connect([this](const std::string& /*usb_name*/) {
--            restart();
--        });
--
--        m_greeter->is_active().changed().connect([this](bool /*is_active*/) {
--            maybe_snap();
--        });
--
--        restart();
++            m_req.reset();
++        });
++
++        m_greeter->state().changed().connect([this](const Greeter::State& state) {
++            if (state == Greeter::State::INACTIVE) {
++                maybe_snap();
++            } else {
++                stop_snap();
++            }
++        });
++
++        // create a new adbd client
++        m_adbd_client.reset(new GAdbdClient{m_socket_path});
++        m_adbd_client->on_pk_request().connect(
++            [this](const AdbdClient::PKRequest& req) {
++                g_debug("%s got pk request: %s, calling maybe_snap()", G_STRLOC, req.fingerprint.c_str());
++
++                m_response = AdbdClient::PKResponse::DENY; // set the fallback response
++                m_req.reset(
++                    new AdbdClient::PKRequest(req),
++                    [this](AdbdClient::PKRequest* r) {
++                        stop_snap();
++                        r->respond(m_response);
++                        delete r;
++                    }
++                );
++                maybe_snap();
++            }
++        );
+     }
      ~Impl()
+     {
--        if (m_restart_idle_tag)
--            g_source_remove(m_restart_idle_tag);
--
--        clear();
++        if (m_request_complete_idle_tag) {
++            g_source_remove(m_request_complete_idle_tag);
++        }
+     }
  private:
--    void clear()
++    void stop_snap()
+     {
--        // clear out old state
          m_snap_connections.clear();
          m_snap.reset();
--        m_req = decltype(m_req)();
--        m_adbd_client.reset();
--    }
--
--    void restart()
--    {
--        clear();
--
--        // set a new client
--        m_adbd_client.reset(new GAdbdClient{m_socket_path});
--        m_adbd_client->on_pk_request().connect(
--            [this](const AdbdClient::PKRequest& req) {
--                g_debug("%s got pk request: %s", G_STRLOC, req.fingerprint.c_str());
--                m_req = req;
--                maybe_snap();
--            }
--        );
+     }
      void maybe_snap()
+     {
--        // don't prompt in the greeter!
--        if (!m_req.public_key.empty() && !m_greeter->is_active().get())
--            snap();
++        // only prompt if there's something to prompt about
++        if (!m_req) {
++            return;
++        }
++
++        // only prompt in an unlocked session
++        if (m_greeter->state().get() != Greeter::State::INACTIVE) {
++            return;
++        }
++
++        snap();
+     }
      void snap()
+     {
--        m_snap = std::make_shared<UsbSnap>(m_req.fingerprint);
++        m_snap = std::make_shared<UsbSnap>(m_req->fingerprint);
          m_snap_connections.insert((*m_snap).on_user_response().connect(
              [this](AdbdClient::PKResponse response, bool remember_choice){
--                g_debug("%s user responded! response %d, remember %d", G_STRLOC, int(response), int(remember_choice));
--                m_req.respond(response);
--                if (remember_choice && (response == AdbdClient::PKResponse::ALLOW))
--                    write_public_key(m_req.public_key);
--                m_restart_idle_tag = g_idle_add([](gpointer gself){
--                    auto self = static_cast<Impl*>(gself);
--                    self->m_restart_idle_tag = 0;
--                    self->restart();
--                    return G_SOURCE_REMOVE;
--                }, this);
++
++                if (remember_choice && (response == AdbdClient::PKResponse::ALLOW)) {
++                    write_public_key(m_req->public_key);
++                }
++
++                m_response = response;
++
++                // defer finishing the request into an idle func because
++                // ScopedConnections can't be destroyed inside their callbacks
++                if (m_request_complete_idle_tag == 0) {
++                    m_request_complete_idle_tag = g_idle_add([](gpointer gself){
++                        auto self = static_cast<Impl*>(gself);
++                        self->m_request_complete_idle_tag = 0;
++                        self->m_req.reset();
++                        return G_SOURCE_REMOVE;
++                    }, this);
++                }
+             }
          ));
+     }
@@ -152,12 +169,13 @@
      const std::shared_ptr<UsbMonitor> m_usb_monitor;
      const std::shared_ptr<Greeter> m_greeter;
--    unsigned int m_restart_idle_tag {};
++    unsigned int m_request_complete_idle_tag {};
      std::shared_ptr<GAdbdClient> m_adbd_client;
--    AdbdClient::PKRequest m_req;
      std::shared_ptr<UsbSnap> m_snap;
      std::set<core::ScopedConnection> m_snap_connections;
++    AdbdClient::PKResponse m_response {AdbdClient::PKResponse::DENY};
++    std::shared_ptr<AdbdClient::PKRequest> m_req;
  };
  /***
@@ -177,4 +195,3 @@
  UsbManager::~UsbManager()
+ {
+ }
--
 === modified file 'src/usb-snap.cpp'
 --- src/usb-snap.cpp	2016-03-23 17:16:06 +0000
 +++ src/usb-snap.cpp	2016-10-21 11:40:57 +0000
@@ -208,6 +208,7 @@
          const bool remember_this_choice = response == AdbdClient::PKResponse::ALLOW;
          m_on_user_response(response, remember_this_choice);
++        m_notification_id = 0;
+     }
      void on_notification_closed(uint32_t close_reason)
 === modified file 'tests/integration/usb-manager-test.cpp'
 --- tests/integration/usb-manager-test.cpp	2016-03-23 17:16:06 +0000
 +++ tests/integration/usb-manager-test.cpp	2016-10-21 11:40:57 +0000
@@ -143,7 +143,7 @@
      );
      // confirm that the AdbdServer got the right response
--    wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 2000);
++    wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 5000);
      ASSERT_EQ(1, adbd_server->m_responses.size());
      EXPECT_EQ("OK", adbd_server->m_responses.front());
@@ -163,13 +163,16 @@
      const std::shared_ptr<std::string> public_keys_path {new std::string{*m_tmpdir+"/adb_keys"}, file_deleter};
      // start a mock AdbdServer ready to submit a request
++    const size_t N_TESTS {3};
      const std::string public_key {"public_key"};
--    auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, std::vector<std::string>{"PK"+public_key});
++    const std::vector<std::string> requests(N_TESTS, "PK"+public_key);
++    const std::vector<std::string> expected_responses(N_TESTS, "NO");
++    auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, requests);
      // set up a UsbManager to process the request
      auto usb_manager = std::make_shared<UsbManager>(*socket_path, *public_keys_path, m_usb_monitor, m_greeter);
--    for (int i=0; i<3; i++)
++    for (std::remove_const<decltype(N_TESTS)>::type i=0; i<N_TESTS; ++i)
+     {
          // add a signal spy to listen to the notification daemon
          QSignalSpy notificationsSpy(
@@ -194,6 +197,9 @@
          EXPECT_EQ("CloseNotification", notificationsSpy.at(0).at(0));
          notificationsSpy.clear();
+     }
++
++    EXPECT_TRUE(wait_for([adbd_server, N_TESTS](){return adbd_server->m_responses.size() == N_TESTS;}, 5000));
++    EXPECT_EQ(expected_responses, adbd_server->m_responses);
+ }
  TEST_F(UsbManagerFixture, Greeter)
@@ -206,7 +212,7 @@
      auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, std::vector<std::string>{"PK"+public_key});
      // set up a UsbManager to process the request
--    m_greeter->m_is_active.set(true);
++    m_greeter->m_state.set(Greeter::State::ACTIVE);
      auto usb_manager = std::make_shared<UsbManager>(*socket_path, *public_keys_path, m_usb_monitor, m_greeter);
      // add a signal spy to listen to the notification daemon
@@ -219,7 +225,7 @@
      EXPECT_FALSE(notificationsSpy.wait(2000));
      // disable the greeter, the notification should appear
--    m_greeter->m_is_active.set(false);
++    m_greeter->m_state.set(Greeter::State::INACTIVE);
      wait_for_signals(notificationsSpy, 1);
      EXPECT_EQ("Notify", notificationsSpy.at(0).at(0));
      notificationsSpy.clear();
 === modified file 'tests/unit/CMakeLists.txt'
 --- tests/unit/CMakeLists.txt	2016-03-16 00:25:33 +0000
 +++ tests/unit/CMakeLists.txt	2016-10-21 11:40:57 +0000
@@ -14,6 +14,10 @@
      ${GMOCK_LIBRARIES}
+ )
++add_definitions(
++  -DGREETER_TEMPLATE="${CMAKE_SOURCE_DIR}/tests/utils/mock-unity-greeter.py"
++)
++
  function(add_test_by_name name)
    set(TEST_NAME ${name})
    add_executable (${TEST_NAME} ${TEST_NAME}.cpp)
@@ -31,4 +35,5 @@
    set_property(TEST ${TEST_NAME} APPEND PROPERTY ENVIRONMENT ${CTEST_ENVIRONMENT})
    target_link_libraries(${TEST_NAME} ${SERVICE_LINK_LIBRARIES} ${QT_LINK_LIBRARIES} ${TEST_LINK_LIBRARIES} ${THREAD_LINK_LIBRARIES})
  endfunction()
++add_qt_test_by_name(greeter-test)
  add_qt_test_by_name(usb-snap-test)
 === modified file 'tests/unit/adbd-client-test.cpp'
 --- tests/unit/adbd-client-test.cpp	2016-03-17 15:24:41 +0000
 +++ tests/unit/adbd-client-test.cpp	2016-10-21 11:40:57 +0000
@@ -73,7 +73,7 @@
          // start an AdbdClient that listens for PKRequests
          std::string pk;
          auto adbd_client = std::make_shared<GAdbdClient>(socket_path);
--        adbd_client->on_pk_request().connect([&pk, main_thread, test](const AdbdClient::PKRequest& req){
++        auto connection = adbd_client->on_pk_request().connect([&pk, main_thread, test](const AdbdClient::PKRequest& req){
              EXPECT_EQ(main_thread, g_thread_self());
              g_message("in on_pk_request with %s", req.public_key.c_str());
              pk = req.public_key;
@@ -82,12 +82,13 @@
          // start a mock AdbdServer with to fire test key requests and wait for a response
          auto adbd_server = std::make_shared<GAdbdServer>(socket_path, std::vector<std::string>{test.request});
--        wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 2000);
++        wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 5000);
          EXPECT_EQ(test.expected_pk, pk);
          ASSERT_EQ(1, adbd_server->m_responses.size());
          EXPECT_EQ(test.expected_response, adbd_server->m_responses.front());
          // cleanup
++        connection.disconnect();
          adbd_client.reset();
          adbd_server.reset();
          g_unlink(socket_path.c_str());
 === added file 'tests/unit/greeter-test.cpp'
 --- tests/unit/greeter-test.cpp	1970-01-01 00:00:00 +0000
 +++ tests/unit/greeter-test.cpp	2016-10-21 11:40:57 +0000
@@ -0,0 +1,159 @@
++/*
++ * Copyright 2016 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3, as published
++ * by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful, but
++ * WITHOUT ANY WARRANTY; without even the implied warranties of
++ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
++ * PURPOSE.  See the GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License along
++ * with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authors:
++ *   Charles Kerr <charles.kerr@canonical.com>
++ */
++
++#include <tests/utils/qt-fixture.h>
++#include <tests/utils/gtest-print-helpers.h>
++
++#include <src/dbus-names.h>
++#include <src/greeter.h>
++
++#include <libqtdbustest/DBusTestRunner.h>
++#include <libqtdbustest/QProcessDBusService.h>
++#include <libqtdbusmock/DBusMock.h>
++
++class GreeterFixture: public QtFixture
++{
++private:
++
++    using super = QtFixture;
++
++public:
++
++    GreeterFixture() =default;
++    ~GreeterFixture() =default;
++
++protected:
++
++    std::shared_ptr<QtDBusTest::DBusTestRunner> m_dbus_runner;
++    std::shared_ptr<QtDBusMock::DBusMock> m_dbus_mock;
++    GDBusConnection* m_bus {};
++
++    void SetUp() override
++    {
++        super::SetUp();
++
++        // use a fresh bus for each test run
++        m_dbus_runner.reset(new QtDBusTest::DBusTestRunner());
++        m_dbus_mock.reset(new QtDBusMock::DBusMock(*m_dbus_runner.get()));
++
++        GError* error {};
++        m_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, nullptr, &error);
++        g_assert_no_error(error);
++        g_dbus_connection_set_exit_on_close(m_bus, FALSE);
++    }
++
++    void TearDown() override
++    {
++        g_clear_object(&m_bus);
++
++        super::TearDown();
++    }
++
++    void start_greeter_service(bool is_active)
++    {
++        // set a watcher to look for our mock greeter to appear
++        bool owned {};
++        QDBusServiceWatcher watcher(
++            DBusNames::UnityGreeter::NAME,
++            m_dbus_runner->sessionConnection()
++        );
++        QObject::connect(
++            &watcher,
++            &QDBusServiceWatcher::serviceRegistered,
++            [&owned](const QString&){owned = true;}
++        );
++
++        // start the mock greeter
++        QVariantMap parameters;
++        parameters["IsActive"] = QVariant(is_active);
++        m_dbus_mock->registerTemplate(
++            DBusNames::UnityGreeter::NAME,
++            GREETER_TEMPLATE,
++            parameters,
++            QDBusConnection::SessionBus
++        );
++        m_dbus_runner->startServices();
++
++        // wait for the watcher
++        ASSERT_TRUE(wait_for([&owned]{return owned;}));
++    }
++};
++
++#define ASSERT_PROPERTY_EQ_EVENTUALLY(expected_in, property_in) \
++    do { \
++        const auto& e = expected_in; \
++        const auto& p = property_in; \
++        ASSERT_TRUE(wait_for([e, &p](){return e == p.get();})) \
++            << "expected " << e << " but got " << p.get(); \
++   } while(0)
++
++/**
++ * Test startup timing by looking at four different cases:
++ * [unity greeter shows up on bus (before, after) we start listening]
++ *   x [unity greeter is (active, inactive)]
++ */
++
++TEST_F(GreeterFixture, ActiveServiceStartsBeforeWatcher)
++{
++    constexpr bool is_active {true};
++    constexpr Greeter::State expected {Greeter::State::ACTIVE};
++
++    start_greeter_service(is_active);
++
++    UnityGreeter greeter;
++
++    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
++}
++
++TEST_F(GreeterFixture, WatcherStartsBeforeActiveService)
++{
++    constexpr bool is_active {true};
++    constexpr Greeter::State expected {Greeter::State::ACTIVE};
++
++    UnityGreeter greeter;
++
++    start_greeter_service(is_active);
++
++    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
++}
++
++TEST_F(GreeterFixture, InactiveServiceStartsBeforeWatcher)
++{
++    constexpr bool is_active {false};
++    constexpr Greeter::State expected {Greeter::State::INACTIVE};
++
++    start_greeter_service(is_active);
++
++    UnityGreeter greeter;
++
++    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
++}
++
++TEST_F(GreeterFixture, WatcherStartsBeforeInactiveService)
++{
++    constexpr bool is_active {false};
++    constexpr Greeter::State expected {Greeter::State::INACTIVE};
++
++    UnityGreeter greeter;
++
++    start_greeter_service(is_active);
++
++    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
++}
++
 === modified file 'tests/unit/usb-snap-test.cpp'
 --- tests/unit/usb-snap-test.cpp	2016-03-23 17:16:06 +0000
 +++ tests/unit/usb-snap-test.cpp	2016-10-21 11:40:57 +0000
@@ -88,7 +88,7 @@
          auto snap = std::make_shared<UsbSnap>(test.fingerprint);
          AdbdClient::PKResponse user_response {};
          bool user_response_set = false;
--        snap->on_user_response().connect([&user_response,&user_response_set](AdbdClient::PKResponse response, bool /*remember*/){
++        auto connection = snap->on_user_response().connect([&user_response,&user_response_set](AdbdClient::PKResponse response, bool /*remember*/){
              user_response = response;
              user_response_set = true;
          });
@@ -130,14 +130,9 @@
          EXPECT_TRUE(user_response_set);
          ASSERT_EQ(test.expected_response, user_response);
--        // confirm that the snap dtor cleans up the notification
++        // confirm that the snap dtor doesn't try to close
++        // the notification that's already been closed by user choice
          snap.reset();
--        wait_for_signals(notificationsSpy, 1);
--        {
--            QVariantList const& call(notificationsSpy.at(0));
--            EXPECT_EQ("CloseNotification", call.at(0));
--            QVariantList const& args(call.at(1).toList());
--            EXPECT_EQ(id, args.at(0));
--        }
++        EXPECT_FALSE(notificationsSpy.wait(1000));
+     }
+ }
 === modified file 'tests/utils/adbd-server.h'
 --- tests/utils/adbd-server.h	2016-03-24 16:01:16 +0000
 +++ tests/utils/adbd-server.h	2016-10-21 11:40:57 +0000
@@ -38,7 +38,7 @@
      GAdbdServer(const std::string& socket_path,
                  const std::vector<std::string>& requests):
          m_requests{requests},
--        m_socket_path{socket_path},
++        m_server_socket{create_server_socket(socket_path)},
          m_cancellable{g_cancellable_new()},
          m_worker_thread{&GAdbdServer::worker_func, this}
+     {
@@ -50,6 +50,7 @@
          g_cancellable_cancel(m_cancellable);
          m_worker_thread.join();
          g_clear_object(&m_cancellable);
++        g_clear_object(&m_server_socket);
+     }
      const std::vector<std::string> m_requests;
@@ -59,18 +60,14 @@
      void worker_func() // runs in worker thread
+     {
--        auto server_socket = create_server_socket(m_socket_path);
          auto requests = m_requests;
--        GError* error {};
--        g_socket_listen (server_socket, &error);
--        g_assert_no_error (error);
--
          while (!g_cancellable_is_cancelled(m_cancellable) && !requests.empty())
+         {
              // wait for a client connection
              g_message("GAdbdServer::Impl::worker_func() calling g_socket_accept()");
--            auto client_socket = g_socket_accept(server_socket, m_cancellable, &error);
++            GError* error {};
++            auto client_socket = g_socket_accept(m_server_socket, m_cancellable, &error);
              if (error != nullptr) {
                  if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
                      g_message("GAdbdServer: Error accepting socket connection: %s", error->message);
@@ -121,8 +118,6 @@
              // cleanup
              g_clear_object(&client_socket);
+         }
--
--        g_clear_object(&server_socket);
+     }
      // bind to a local domain socket
@@ -139,11 +134,14 @@
          g_assert_no_error (error);
          g_clear_object (&address);
++        g_socket_listen (socket, &error);
++        g_assert_no_error (error);
++
          return socket;
+     }
--    const std::string m_socket_path;
--    GCancellable* m_cancellable = nullptr;
++    GSocket* m_server_socket {};
++    GCancellable* m_cancellable {};
      std::thread m_worker_thread;
  };
 === added file 'tests/utils/gtest-print-helpers.h'
 --- tests/utils/gtest-print-helpers.h	1970-01-01 00:00:00 +0000
 +++ tests/utils/gtest-print-helpers.h	2016-10-21 11:40:57 +0000
@@ -0,0 +1,18 @@
++
++#pragma once
++
++#include <src/greeter.h>
++
++inline void PrintTo(const Greeter::State& state, std::ostream* os) {
++    switch(state) {
++        case Greeter::State::ACTIVE:      *os << "Active"; break;
++        case Greeter::State::INACTIVE:    *os << "Inactive"; break;
++        case Greeter::State::UNAVAILABLE: *os << "Unavailable"; break;
++    }
++}
++
++inline std::ostream& operator<<(std::ostream& os, const Greeter::State& state) {
++    PrintTo(state, &os);
++    return os;
++}
++
 === modified file 'tests/utils/mock-greeter.h'
 --- tests/utils/mock-greeter.h	2016-03-22 20:39:36 +0000
 +++ tests/utils/mock-greeter.h	2016-10-21 11:40:57 +0000
@@ -26,7 +26,7 @@
  public:
      MockGreeter() =default;
      virtual ~MockGreeter() =default;
--    core::Property<bool>& is_active() override {return m_is_active;}
--    core::Property<bool> m_is_active {false};
++    core::Property<Greeter::State>& state() override {return m_state;}
++    core::Property<Greeter::State> m_state {State::INACTIVE};
  };
 === added file 'tests/utils/mock-unity-greeter.py'
 --- tests/utils/mock-unity-greeter.py	1970-01-01 00:00:00 +0000
 +++ tests/utils/mock-unity-greeter.py	2016-10-21 11:40:57 +0000
@@ -0,0 +1,41 @@
++'''unity greeter mock template
++
++Very basic template that just mocks the greeter is-active flag
++'''
++
++# This program is free software; you can redistribute it and/or modify it under
++# the terms of the GNU Lesser General Public License as published by the Free
++# Software Foundation; either version 3 of the License, or (at your option) any
++# later version.  See http://www.gnu.org/copyleft/lgpl.html for the full text
++# of the license.
++
++__author__ = 'Charles Kerr'
++__email__ = 'charles.kerr@canonical.com'
++__copyright__ = '(c) 2016 Canonical Ltd.'
++__license__ = 'LGPL 3+'
++
++import dbus
++import os
++
++from dbusmock import MOCK_IFACE, mockobject
++
++BUS_NAME = 'com.canonical.UnityGreeter'
++MAIN_OBJ = '/'
++MAIN_IFACE = 'com.canonical.UnityGreeter'
++SYSTEM_BUS = False
++
++
++def load(mock, parameters):
++    mock.AddMethods(
++        MAIN_IFACE, [
++            ('HideGreeter', '', '', 'self.Set("com.canonical.UnityGreeter", "IsActive", False)'),
++            ('ShowGreeter', '', '', 'self.Set("com.canonical.UnityGreeter", "IsActive", True)')
++        ]
++    )
++    mock.AddProperties(
++        MAIN_IFACE,
++        dbus.Dictionary({
++            'IsActive': parameters.get('IsActive', False),
++        }, signature='sv')
++    )
++