dynamic services: fix extra sends am: f083ee34c8 am: 04ec3b3c7a
Change-Id: Ib5bd632eb989939b04addefa23d85f8e933d80a2
diff --git a/HidlService.cpp b/HidlService.cpp
index 6d8c4ff..6ee6024 100644
--- a/HidlService.cpp
+++ b/HidlService.cpp
@@ -104,10 +104,10 @@
return mPassthroughClients;
}
-void HidlService::addClientCallback(const sp<IClientCallback>& callback) {
+void HidlService::addClientCallback(const sp<IClientCallback>& callback, size_t knownClientCount) {
if (mHasClients) {
// we have this kernel feature, so make sure we're in an updated state
- forceHandleClientCallbacks(false /*onInterval*/);
+ forceHandleClientCallbacks(false /*onInterval*/, knownClientCount);
}
if (mHasClients) {
@@ -133,21 +133,21 @@
return found;
}
-ssize_t HidlService::handleClientCallbacks(bool isCalledOnInterval) {
+bool HidlService::handleClientCallbacks(bool isCalledOnInterval, size_t knownClientCount) {
if (!mClientCallbacks.empty()) {
- return forceHandleClientCallbacks(isCalledOnInterval);
+ return forceHandleClientCallbacks(isCalledOnInterval, knownClientCount);
}
- return -1;
+ return false;
}
-ssize_t HidlService::forceHandleClientCallbacks(bool isCalledOnInterval) {
+bool HidlService::forceHandleClientCallbacks(bool isCalledOnInterval, size_t knownClientCount) {
ssize_t count = getNodeStrongRefCount();
// binder driver doesn't support this feature
- if (count == -1) return count;
+ if (count < 0) return false;
- bool hasClients = count > 1; // this process holds a strong count
+ bool hasClients = (size_t)count > knownClientCount;
if (mGuaranteeClient) {
// we have no record of this client
@@ -173,7 +173,7 @@
}
}
- return count;
+ return mHasClients;
}
void HidlService::guaranteeClient() {
diff --git a/HidlService.h b/HidlService.h
index f94f82b..76d1c38 100644
--- a/HidlService.h
+++ b/HidlService.h
@@ -67,16 +67,23 @@
void registerPassthroughClient(pid_t pid);
// also sends onClients(true) if we have clients
- void addClientCallback(const sp<IClientCallback>& callback);
+ // knownClientCount, see forceHandleClientCallbacks
+ void addClientCallback(const sp<IClientCallback>& callback, size_t knownClientCount);
bool removeClientCallback(const sp<IClientCallback>& callback);
- // return is number of clients (-1 means this is not implemented or we didn't check)
- // count includes one held by hwservicemanager
- ssize_t handleClientCallbacks(bool isCalledOnInterval);
+ // return is if we are guaranteed to have a client
+ // knownClientCount, see forceHandleClientCallbacks
+ bool handleClientCallbacks(bool isCalledOnInterval, size_t knownClientCount);
// Updates client callbacks (even if mClientCallbacks is emtpy)
// see handleClientCallbacks
- ssize_t forceHandleClientCallbacks(bool isCalledOnInterval);
+ //
+ // knownClientCount - this is the number of clients that is currently
+ // expected to be in use by known actors. This number of clients must be
+ // exceeded in order to consider the service to have clients.
+ //
+ // returns whether or not this service has clients
+ bool forceHandleClientCallbacks(bool isCalledOnInterval, size_t knownClientCount);
// when giving out a handle to a client, but the kernel might not know this yet
void guaranteeClient();
diff --git a/ServiceManager.cpp b/ServiceManager.cpp
index 991dc36..165a8d9 100644
--- a/ServiceManager.cpp
+++ b/ServiceManager.cpp
@@ -291,7 +291,7 @@
// time. This will run before anything else can modify the HidlService which is owned by this
// object, so it will be in the same state that it was when this function returns.
hardware::addPostCommandTask([hidlService] {
- hidlService->handleClientCallbacks(false /* isCalledOnInterval */);
+ hidlService->handleClientCallbacks(false /* isCalledOnInterval */, 1 /*knownClientCount*/);
});
return service;
@@ -602,7 +602,10 @@
return false;
}
- registered->addClientCallback(cb);
+ // knownClientCount
+ // - one from binder transaction (base here)
+ // - one from hwservicemanager
+ registered->addClientCallback(cb, 2 /*knownClientCount*/);
return true;
}
@@ -625,7 +628,8 @@
void ServiceManager::handleClientCallbacks() {
forEachServiceEntry([&] (HidlService *service) {
- service->handleClientCallbacks(true /* isCalledOnInterval */);
+ // hwservicemanager will hold one reference, so knownClientCount is 1.
+ service->handleClientCallbacks(true /* isCalledOnInterval */, 1 /*knownClientCount*/);
return true; // continue
});
}
@@ -688,14 +692,12 @@
return false;
}
- int clients = registered->forceHandleClientCallbacks(false /* isCalledOnInterval */);
+ // knownClientCount
+ // - one from binder transaction (base here)
+ // - one from hwservicemanager
+ bool clients = registered->forceHandleClientCallbacks(false /*isCalledOnInterval*/, 2 /*knownClientCount*/);
- // clients < 0: feature not implemented or other error. Assume clients.
- // Otherwise:
- // - kernel driver will hold onto one refcount (during this transaction)
- // - hwservicemanager has a refcount (guaranteed by this transaction)
- // So, if clients > 2, then at least one other service on the system must hold a refcount.
- if (clients < 0 || clients > 2) {
+ if (clients) {
// client callbacks are either disabled or there are other clients
LOG(INFO) << "Tried to unregister for " << fqName << "/" << name
<< " but there are clients: " << clients;
diff --git a/TEST_MAPPING b/TEST_MAPPING
index 54334db..0de3a89 100644
--- a/TEST_MAPPING
+++ b/TEST_MAPPING
@@ -2,6 +2,9 @@
"presubmit": [
{
"name": "hwservicemanager_test"
+ },
+ {
+ "name": "hidl_lazy_test"
}
]
}
diff --git a/test_lazy.cpp b/test_lazy.cpp
index 21108ba..81ae694 100644
--- a/test_lazy.cpp
+++ b/test_lazy.cpp
@@ -83,12 +83,27 @@
sp<RecordingClientCallback> cb = new RecordingClientCallback;
std::unique_ptr<HidlService> service = makeService();
- service->addClientCallback(cb);
+ service->addClientCallback(cb, 1 /*knownClients*/);
setReportedClientCount(1);
for (size_t i = 0; i < 100; i++) {
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
+ }
+
+ ASSERT_THAT(cb->stream, ElementsAre());
+}
+
+TEST_F(HidlServiceLazyTest, NoChangeWithKnownClients) {
+ sp<RecordingClientCallback> cb = new RecordingClientCallback;
+
+ std::unique_ptr<HidlService> service = makeService();
+ service->addClientCallback(cb, 2 /*knownClients*/);
+
+ setReportedClientCount(2);
+
+ for (size_t i = 0; i < 100; i++) {
+ service->handleClientCallbacks(true /*onInterval*/, 2 /*knownClients*/);
}
ASSERT_THAT(cb->stream, ElementsAre());
@@ -98,20 +113,20 @@
sp<RecordingClientCallback> cb = new RecordingClientCallback;
std::unique_ptr<HidlService> service = makeService();
- service->addClientCallback(cb);
+ service->addClientCallback(cb, 1 /*knownClients*/);
// some other process has the service
setReportedClientCount(2);
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
// just hwservicemanager has the service
setReportedClientCount(1);
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
}
@@ -120,18 +135,18 @@
sp<RecordingClientCallback> cb = new RecordingClientCallback;
std::unique_ptr<HidlService> service = makeService();
- service->addClientCallback(cb);
+ service->addClientCallback(cb, 1 /*knownClients*/);
service->guaranteeClient();
setReportedClientCount(1);
- service->handleClientCallbacks(false /*onInterval*/);
+ service->handleClientCallbacks(false /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
}
@@ -139,23 +154,23 @@
sp<RecordingClientCallback> cb = new RecordingClientCallback;
std::unique_ptr<HidlService> service = makeService();
- service->addClientCallback(cb);
+ service->addClientCallback(cb, 1 /*knownClients*/);
// Clients can appear and dissappear as many times as necessary, but they are only considered
// dropped when the fixed interval stops.
for (size_t i = 0; i < 100; i++) {
setReportedClientCount(2);
- service->handleClientCallbacks(false /*onInterval*/);
+ service->handleClientCallbacks(false /*onInterval*/, 1 /*knownClients*/);
setReportedClientCount(1);
- service->handleClientCallbacks(false /*onInterval*/);
+ service->handleClientCallbacks(false /*onInterval*/, 1 /*knownClients*/);
}
ASSERT_THAT(cb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
}
@@ -163,22 +178,22 @@
sp<RecordingClientCallback> cb = new RecordingClientCallback;
std::unique_ptr<HidlService> service = makeService();
- service->addClientCallback(cb);
+ service->addClientCallback(cb, 1 /*knownClients*/);
setReportedClientCount(2);
- service->handleClientCallbacks(false /*onInterval*/);
+ service->handleClientCallbacks(false /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
setReportedClientCount(1);
service->guaranteeClient();
- service->handleClientCallbacks(false /*onInterval*/);
+ service->handleClientCallbacks(false /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
}
@@ -186,25 +201,25 @@
sp<RecordingClientCallback> cb = new RecordingClientCallback;
std::unique_ptr<HidlService> service = makeService();
- service->addClientCallback(cb);
+ service->addClientCallback(cb, 1 /*knownClients*/);
setReportedClientCount(2);
- service->handleClientCallbacks(false /*onInterval*/);
+ service->handleClientCallbacks(false /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
sp<RecordingClientCallback> laterCb = new RecordingClientCallback;
- service->addClientCallback(laterCb);
+ service->addClientCallback(laterCb, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
ASSERT_THAT(laterCb->stream, ElementsAre(true));
setReportedClientCount(1);
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true));
ASSERT_THAT(laterCb->stream, ElementsAre(true));
- service->handleClientCallbacks(true /*onInterval*/);
+ service->handleClientCallbacks(true /*onInterval*/, 1 /*knownClients*/);
ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
ASSERT_THAT(laterCb->stream, ElementsAre(true, false)); // reported only after two intervals
}
@@ -213,7 +228,7 @@
std::unique_ptr<HidlService> service = makeService();
setReportedClientCount(2);
- service->handleClientCallbacks(false /*onInterval*/);
+ service->handleClientCallbacks(false /*onInterval*/, 1 /*knownClients*/);
// kernel API should not be called
EXPECT_EQ(0u, getNumTimesReported());
