commit 2fa0f5509bde0d6922ad605b35566358a78a1151
author Steven Moreland <smoreland@google.com> Fri Feb 01 19:27:33 2019 -0800
committer Steven Moreland <smoreland@google.com> Tue Feb 05 13:51:24 2019 -0800
tree 1628b2975c05feeec22154f4424c04a38fe282d6
parent 9cd9e88f7e83518a1e4fcdf612d1f2b11aa29749

registerClientCallback: immediate notification

If there is an existing service, registerClientCallback should
immediately send onClients(true). This is part of the IServiceManager
contract.

Fixes: 123318663
Test: hwservicemanager_test
    HidlServiceLazyTest NotificationSentForNewClientCallback (now passes)
Test: internal 'lazy_hidl_test' fuzzing health storage HAL, health
    storage HAL no longer reports double-de-acquisition of clients.

Change-Id: If83b162262d33de59179b95dbb7175aaafb0f050

HidlService.cpp

diff --git a/HidlService.cpp b/HidlService.cpp
index ddc0f1a..6d020a4 100644
--- a/HidlService.cpp
+++ b/HidlService.cpp
@@ -105,6 +105,12 @@
 }
 
 void HidlService::addClientCallback(const sp<IClientCallback>& callback) {
+    handleClientCallbacks(false /* isCalledOnInterval */);
+
+    if (mHasClients) {
+        sendClientCallbackNotification(callback, true /*hasClients*/);
+    }
+
     mClientCallbacks.push_back(callback);
 }
 
@@ -212,16 +218,20 @@
     LOG(INFO) << "Notifying " << string() << " they have clients: " << hasClients;
 
     for (const auto& cb : mClientCallbacks) {
-        Return<void> ret = cb->onClients(getService(), hasClients);
-        if (!ret.isOk()) {
-            LOG(WARNING) << "onClients callback failed for " << string() << ": " << ret.description();
-        }
+        sendClientCallbackNotification(cb, hasClients);
     }
 
     mNoClientsCounter = 0;
     mHasClients = hasClients;
 }
 
+void HidlService::sendClientCallbackNotification(const sp<IClientCallback>& callback, bool hasClients) {
+    Return<void> ret = callback->onClients(getService(), hasClients);
+    if (!ret.isOk()) {
+        LOG(WARNING) << "onClients callback failed for " << string() << ": " << ret.description();
+    }
+}
+
 
 }  // namespace implementation
 }  // namespace manager

HidlService.h

diff --git a/HidlService.h b/HidlService.h
index 7256b52..426df8e 100644
--- a/HidlService.h
+++ b/HidlService.h
@@ -67,6 +67,7 @@
     bool removeListener(const wp<IBase> &listener);
     void registerPassthroughClient(pid_t pid);
 
+    // also sends onClients(true) if we have clients
     void addClientCallback(const sp<IClientCallback>& callback);
     bool removeClientCallback(const sp<IClientCallback>& callback);
 
@@ -86,8 +87,13 @@
 
 private:
     void sendRegistrationNotifications();
+
+    // Also updates mHasClients (of what the last callback was)
     void sendClientCallbackNotifications(bool hasClients);
 
+    // Only sends notification
+    void sendClientCallbackNotification(const sp<IClientCallback>& callback, bool hasClients);
+
     const std::string                     mInterfaceName; // e.x. "android.hidl.manager@1.0::IServiceManager"
     const std::string                     mInstanceName;  // e.x. "manager"
     sp<IBase>                             mService;

ServiceManager.cpp

diff --git a/ServiceManager.cpp b/ServiceManager.cpp
index a4f99ce..ac142f5 100644
--- a/ServiceManager.cpp
+++ b/ServiceManager.cpp
@@ -565,7 +565,6 @@
     }
 
     registered->addClientCallback(cb);
-    registered->handleClientCallbacks(false /* isCalledOnInterval */);
 
     return true;
 }

test_lazy.cpp

diff --git a/test_lazy.cpp b/test_lazy.cpp
index 542bf53..df42329 100644
--- a/test_lazy.cpp
+++ b/test_lazy.cpp
@@ -170,3 +170,30 @@
     service->handleClientCallbacks(true /*onInterval*/);
     ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
 }
+
+TEST_F(HidlServiceLazyTest, NotificationSentForNewClientCallback) {
+    sp<RecordingClientCallback> cb = new RecordingClientCallback;
+
+    std::unique_ptr<HidlService> service = makeService();
+    service->addClientCallback(cb);
+
+    setReportedClientCount(2);
+    service->handleClientCallbacks(false /*onInterval*/);
+    ASSERT_THAT(cb->stream, ElementsAre(true));
+
+    sp<RecordingClientCallback> laterCb = new RecordingClientCallback;
+    service->addClientCallback(laterCb);
+
+    ASSERT_THAT(cb->stream, ElementsAre(true));
+    ASSERT_THAT(laterCb->stream, ElementsAre(true));
+
+    setReportedClientCount(1);
+
+    service->handleClientCallbacks(true /*onInterval*/);
+    ASSERT_THAT(cb->stream, ElementsAre(true));
+    ASSERT_THAT(laterCb->stream, ElementsAre(true));
+
+    service->handleClientCallbacks(true /*onInterval*/);
+    ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
+    ASSERT_THAT(laterCb->stream, ElementsAre(true, false)); // reported only after two intervals
+}