Fix race conditions for lazy services.
Add hidlFqName/hidlName to registerClientCallbacks to make it more
explicit, provide better errors, and for general sanity.
Add tryUnregister so that a service can unregister itself before
shutting down.
Bug: 123318663
Test: hidl_test, manual
Change-Id: I3ee26ba96dbc9732e72aaa16281abc47ce6a02c9
diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp
index 9223af6..429ab8b 100644
--- a/transport/HidlLazyUtils.cpp
+++ b/transport/HidlLazyUtils.cpp
@@ -70,10 +70,10 @@
Return<void> ClientCounterCallback::onClients(const sp<::android::hidl::base::V1_0::IBase>& service,
bool clients) {
if (clients) {
- LOG(INFO) << "HAL " << service->descriptor << " connected.";
+ LOG(INFO) << "HAL " << getDescriptor(service.get()) << " connected.";
mNumConnectedServices++;
} else {
- LOG(INFO) << "HAL " << service->descriptor << " disconnected.";
+ LOG(INFO) << "HAL " << getDescriptor(service.get()) << " disconnected.";
mNumConnectedServices--;
}
LOG(INFO) << "HAL has " << mNumConnectedServices << " (of " << mNumRegisteredServices
@@ -89,11 +89,12 @@
status_t LazyServiceRegistrarImpl::registerService(
const sp<::android::hidl::base::V1_0::IBase>& service, const std::string& name) {
static auto manager = hardware::defaultServiceManager1_2();
- LOG(INFO) << "Registering HAL: " << service->descriptor << " with name: " << name;
+ LOG(INFO) << "Registering HAL: " << getDescriptor(service.get()) << " with name: " << name;
status_t res = android::hardware::details::registerAsServiceInternal(service, name);
if (res == android::OK) {
mClientCallback->incServiceCounter();
- bool ret = manager->registerClientCallback(service, mClientCallback);
+ bool ret = manager->registerClientCallback(getDescriptor(service.get()), name, service,
+ mClientCallback);
if (!ret) {
res = android::INVALID_OPERATION;
LOG(ERROR) << "Failed to add client callback";
diff --git a/transport/HidlTransportUtils.cpp b/transport/HidlTransportUtils.cpp
index 4e952eb..8c61281 100644
--- a/transport/HidlTransportUtils.cpp
+++ b/transport/HidlTransportUtils.cpp
@@ -56,6 +56,10 @@
}
std::string getDescriptor(IBase* interface) {
+ if (interface == nullptr) {
+ return "";
+ }
+
std::string myDescriptor{};
auto ret = interface->interfaceDescriptor([&](const hidl_string &types) {
myDescriptor = types.c_str();
diff --git a/transport/manager/1.2/IClientCallback.hal b/transport/manager/1.2/IClientCallback.hal
index 1189e44..8ebb044 100644
--- a/transport/manager/1.2/IClientCallback.hal
+++ b/transport/manager/1.2/IClientCallback.hal
@@ -21,6 +21,10 @@
* This is called when there is a transition between having >= 1 clients and having 0 clients
* (or vice versa).
*
+ * Upon receiving hasClients false, if the process decides to exit, it is recommended to try to
+ * unregister using @1.2::IServiceManager's tryUnregister before quiting in case another client
+ * associates.
+ *
* @param registered binder 'server' registered with IServiceManager's registerClientCallback
* @param hasClients whether there are currently clients
* true - when there are >= 1 clients. This must be called as soon as IServiceManager::get
diff --git a/transport/manager/1.2/IServiceManager.hal b/transport/manager/1.2/IServiceManager.hal
index e1629d6..d79403d 100644
--- a/transport/manager/1.2/IServiceManager.hal
+++ b/transport/manager/1.2/IServiceManager.hal
@@ -27,7 +27,9 @@
* If the service has clients at the time of registration, the callback is called with
* hasClients true. After that, it is called based on the changes in clientele.
*
- * @param server non-null service waiting to have no clients
+ * @param fqName Fully-qualified interface name (used to register)
+ * @param name Instance name (of the registered service)
+ * @param server non-null service waiting to have no clients (previously registered)
* @param cb non-null callback to call when there are no clients
* @return success
* true on success
@@ -35,7 +37,11 @@
* - the server or cb parameters are null
* - this is called by a process other than the server process
*/
- registerClientCallback(interface server, IClientCallback cb) generates (bool success);
+ registerClientCallback(string fqName,
+ string name,
+ interface server,
+ IClientCallback cb)
+ generates (bool success);
/**
* Removes a callback previously registered with registerClientCallback.
@@ -68,9 +74,32 @@
*
* See also @1.0::IServiceManager's listByInterface function.
*
- * @param fqName Fully-qualified interface name.
+ * @param fqName Fully-qualified interface name.
*
* @return instanceNames List of instance names running the particular service.
*/
listManifestByInterface(string fqName) generates (vec<string> instanceNames);
+
+ /**
+ * Unregisters a service if there are no clients for it. This must only unregister the
+ * service if it is called from the same process that registered the service.
+ *
+ * This unregisters all instances of the service, even if they are under a different instance
+ * name.
+ *
+ * Recommended usage is when creating a lazy process, try unregistering when IClientCallback's
+ * onClients(*, false) is called. If this unregister is successful, then it is safe to exit. If
+ * it is unsuccessful, then you can assume a client re-associated with the server. If a process
+ * has multiple servers, and only some succeed in unregistration, then the unregistered services
+ * can be re-registered.
+ *
+ * See also addWithChain and @1.0::IServiceManager's add.
+ *
+ * @param fqName Fully-qualified interface name.
+ * @param name Instance name.
+ * @param service Handle to registering service.
+ *
+ * @return success Whether the service was successfully unregistered.
+ */
+ tryUnregister(string fqName, string name, interface service) generates (bool success);
};