dynamic services: fix extra sends
Before this change, hwservicemanager sometimes sent out client service
notifications for services which did not currently have known clients.
This is because HidlService always compared the number of clients
reported to the driver w/ '1', representing the service as held by
hwservicemanager. However, in tryUnregister calls and addClientCallback
calls, there are actually two known clients.
So, in this CL, the number of known clients is always piped through from
ServiceManager to HidlService.
Bug: 140310064
Test: hidl_lazy_test
Test: hidl_lazy_test (with hwservicemanager and the test modified to run
ms rather than every 5s. This allows many more iterations to be
tested at a time).
Test: hwservicemanager_test
Change-Id: I1d98aef29bf6ccfb5088f525c250874d3f2254f1
(cherry picked from commit f083ee34c8fbd591aa67d3753e51d0aa2fe4df5b)
Merged-In: I1d98aef29bf6ccfb5088f525c250874d3f2254f1
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());