diff options
-rw-r--r-- | transport/HidlLazyUtils.cpp | 56 | ||||
-rw-r--r-- | transport/include/hidl/HidlLazyUtils.h | 3 |
2 files changed, 35 insertions, 24 deletions
diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp index be7470f..c5f8c74 100644 --- a/transport/HidlLazyUtils.cpp +++ b/transport/HidlLazyUtils.cpp @@ -36,9 +36,9 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall bool addRegisteredService(const sp<IBase>& service, const std::string& name); - bool tryUnregister(); + bool tryUnregisterLocked(); - void reRegister(); + void reRegisterLocked(); void setActiveServicesCallback(const std::function<bool(bool)>& activeServicesCallback); @@ -58,18 +58,23 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall * Looks up service that is guaranteed to be registered (service from * onClients). */ - Service& assertRegisteredService(const sp<IBase>& service); + Service& assertRegisteredServiceLocked(const sp<IBase>& service); /** * Registers or re-registers services. Returns whether successful. */ - bool registerService(const sp<IBase>& service, const std::string& name); + bool registerServiceLocked(const sp<IBase>& service, const std::string& name); /** * Unregisters all services that we can. If we can't unregister all, re-register other * services. */ - void tryShutdown(); + void tryShutdownLocked(); + + /** + * For below. + */ + std::mutex mMutex; /** * Number of services that have been registered. @@ -103,7 +108,8 @@ class LazyServiceRegistrarImpl { bool ClientCounterCallback::addRegisteredService(const sp<IBase>& service, const std::string& name) { - bool success = registerService(service, name); + std::lock_guard<std::mutex> lock(mMutex); + bool success = registerServiceLocked(service, name); if (success) { mRegisteredServices.push_back({service, name}); @@ -112,7 +118,7 @@ bool ClientCounterCallback::addRegisteredService(const sp<IBase>& service, return success; } -ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredService( +ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredServiceLocked( const sp<IBase>& service) { for (Service& registered : mRegisteredServices) { if (registered.service != service) continue; @@ -123,7 +129,8 @@ ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredService( __builtin_unreachable(); } -bool ClientCounterCallback::registerService(const sp<IBase>& service, const std::string& name) { +bool ClientCounterCallback::registerServiceLocked(const sp<IBase>& service, + const std::string& name) { auto manager = hardware::defaultServiceManager1_2(); const std::string descriptor = getDescriptor(service.get()); @@ -145,13 +152,10 @@ bool ClientCounterCallback::registerService(const sp<IBase>& service, const std: return true; } -/** - * onClients is oneway, so no need to worry about multi-threading. Note that this means multiple - * invocations could occur on different threads however. - */ Return<void> ClientCounterCallback::onClients(const sp<::android::hidl::base::V1_0::IBase>& service, bool clients) { - Service& registered = assertRegisteredService(service); + std::lock_guard<std::mutex> lock(mMutex); + Service& registered = assertRegisteredServiceLocked(service); if (registered.clients == clients) { LOG(FATAL) << "Process already thought " << getDescriptor(service.get()) << "/" << registered.name << " had clients: " << registered.clients @@ -181,13 +185,13 @@ Return<void> ClientCounterCallback::onClients(const sp<::android::hidl::base::V1 // client count change event, try to shutdown the process if its services // have no clients. if (!handledInCallback && numWithClients == 0) { - tryShutdown(); + tryShutdownLocked(); } return Status::ok(); } -bool ClientCounterCallback::tryUnregister() { +bool ClientCounterCallback::tryUnregisterLocked() { auto manager = hardware::defaultServiceManager1_2(); for (Service& entry : mRegisteredServices) { @@ -206,14 +210,14 @@ bool ClientCounterCallback::tryUnregister() { return true; } -void ClientCounterCallback::reRegister() { +void ClientCounterCallback::reRegisterLocked() { for (Service& entry : mRegisteredServices) { // re-register entry if not already registered if (entry.registered) { continue; } - if (!registerService(entry.service, entry.name)) { + if (!registerServiceLocked(entry.service, entry.name)) { // Must restart. Otherwise, clients will never be able to get ahold of this service. LOG(FATAL) << "Bad state: could not re-register " << getDescriptor(entry.service.get()); } @@ -222,22 +226,24 @@ void ClientCounterCallback::reRegister() { } } -void ClientCounterCallback::tryShutdown() { +void ClientCounterCallback::tryShutdownLocked() { LOG(INFO) << "Trying to exit HAL. No clients in use for any service in process."; - if (tryUnregister()) { + if (tryUnregisterLocked()) { LOG(INFO) << "Unregistered all clients and exiting"; exit(EXIT_SUCCESS); } // At this point, we failed to unregister some of the services, leaving the // server in an inconsistent state. Re-register all services that were - // unregistered by tryUnregister(). - reRegister(); + // unregistered by tryUnregisterLocked(). + reRegisterLocked(); } void ClientCounterCallback::setActiveServicesCallback( const std::function<bool(bool)>& activeServicesCallback) { + std::lock_guard<std::mutex> lock(mMutex); + mActiveServicesCallback = activeServicesCallback; } @@ -251,11 +257,15 @@ status_t LazyServiceRegistrarImpl::registerService( } bool LazyServiceRegistrarImpl::tryUnregister() { - return mClientCallback->tryUnregister(); + // see comments in header, this should only be called from the active + // services callback, see also b/191781736 + return mClientCallback->tryUnregisterLocked(); } void LazyServiceRegistrarImpl::reRegister() { - mClientCallback->reRegister(); + // see comments in header, this should only be called from the active + // services callback, see also b/191781736 + mClientCallback->reRegisterLocked(); } void LazyServiceRegistrarImpl::setActiveServicesCallback( diff --git a/transport/include/hidl/HidlLazyUtils.h b/transport/include/hidl/HidlLazyUtils.h index 427611b..376b6ab 100644 --- a/transport/include/hidl/HidlLazyUtils.h +++ b/transport/include/hidl/HidlLazyUtils.h @@ -65,7 +65,8 @@ class LazyServiceRegistrar { /** * Try to unregister all services previously registered with 'registerService'. - * Returns 'true' if successful. + * Returns 'true' if successful. This should only be called within + * the callback registered by setActiveServicesCallback. */ bool tryUnregister(); |