From 14829be2691102eeff5b395317b45aef79560c4f Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Fri, 3 Dec 2021 19:07:41 -0800 Subject: [PATCH] Implement linkToDeath, use binder as client ID. Implement linkToDeath for binders. Delete allocated resources for a binder when it died or unlinked. This CL also uses 'const AIBinder*' as client id type instead of the callback because the Binder object corresponds to the remote proxy and is guaranteed to be unique per client. Bug: 204943359 Test: atest DefaultVehicleHalTest Change-Id: If2e0c58e86a041a78b8ca69597aef4733ce1826c --- .../aidl/impl/vhal/include/ConnectedClient.h | 28 ++--- .../impl/vhal/include/DefaultVehicleHal.h | 58 ++++++++- .../impl/vhal/include/SubscriptionManager.h | 30 +++-- .../aidl/impl/vhal/src/DefaultVehicleHal.cpp | 103 ++++++++++++++-- .../impl/vhal/src/SubscriptionManager.cpp | 42 ++++--- .../impl/vhal/test/DefaultVehicleHalTest.cpp | 113 ++++++++++++++++-- .../vhal/test/SubscriptionManagerTest.cpp | 30 +++-- 7 files changed, 320 insertions(+), 84 deletions(-) diff --git a/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h b/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h index 4f0b74a853..833707a580 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h +++ b/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h @@ -42,10 +42,10 @@ namespace vehicle { // This class is thread-safe. class ConnectedClient { public: - ConnectedClient( - std::shared_ptr requestPool, - std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> - callback); + using CallbackType = + std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback>; + + ConnectedClient(std::shared_ptr requestPool, CallbackType callback); virtual ~ConnectedClient() = default; @@ -68,8 +68,7 @@ class ConnectedClient { virtual std::shared_ptr getTimeoutCallback() = 0; const std::shared_ptr mRequestPool; - const std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> - mCallback; + const CallbackType mCallback; }; // A class to represent a client that calls {@code IVehicle.setValues} or {@code @@ -77,10 +76,7 @@ class ConnectedClient { template class GetSetValuesClient final : public ConnectedClient { public: - GetSetValuesClient( - std::shared_ptr requestPool, - std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> - callback); + GetSetValuesClient(std::shared_ptr requestPool, CallbackType callback); // Sends the results to this client. void sendResults(const std::vector& results); @@ -105,10 +101,7 @@ class GetSetValuesClient final : public ConnectedClient { // A class to represent a client that calls {@code IVehicle.subscribe}. class SubscriptionClient final : public ConnectedClient { public: - SubscriptionClient( - std::shared_ptr requestPool, - std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> - callback); + SubscriptionClient(std::shared_ptr requestPool, CallbackType callback); // Gets the callback to be called when the request for this client has finished. std::shared_ptr getResultCallback(); @@ -116,8 +109,7 @@ class SubscriptionClient final : public ConnectedClient { // Marshals the updated values into largeParcelable and sents it through {@code onPropertyEvent} // callback. static void sendUpdatedValues( - std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> - callback, + CallbackType callback, std::vector<::aidl::android::hardware::automotive::vehicle::VehiclePropValue>&& updatedValues); @@ -132,9 +124,7 @@ class SubscriptionClient final : public ConnectedClient { std::shared_ptr mPropertyChangeCallback; static void onGetValueResults( - const void* clientId, - std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> - callback, + const void* clientId, CallbackType callback, std::shared_ptr requestPool, std::vector<::aidl::android::hardware::automotive::vehicle::GetValueResult> results); }; diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h index b9975bcfc8..62b2627691 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h +++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h @@ -53,7 +53,7 @@ class DefaultVehicleHal final : public ::aidl::android::hardware::automotive::ve explicit DefaultVehicleHal(std::unique_ptr hardware); - ~DefaultVehicleHal() = default; + ~DefaultVehicleHal(); ::ndk::ScopedAStatus getAllPropConfigs( ::aidl::android::hardware::automotive::vehicle::VehiclePropConfigs* returnConfigs) @@ -101,7 +101,7 @@ class DefaultVehicleHal final : public ::aidl::android::hardware::automotive::ve private: std::mutex mLock; - std::unordered_map mIds GUARDED_BY(mLock); + std::unordered_map mIds GUARDED_BY(mLock); }; // A thread safe class to store all subscribe clients. This class is safe to pass to async @@ -112,16 +112,42 @@ class DefaultVehicleHal final : public ::aidl::android::hardware::automotive::ve std::shared_ptr getClient(const CallbackType& callback); + void removeClient(const AIBinder* clientId); + size_t countClients(); private: std::mutex mLock; - std::unordered_map> mClients + std::unordered_map> mClients GUARDED_BY(mLock); // PendingRequestPool is thread-safe. std::shared_ptr mPendingRequestPool; }; + // A wrapper for linkToDeath to enable stubbing for test. + class ILinkToDeath { + public: + virtual ~ILinkToDeath() = default; + + virtual binder_status_t linkToDeath(AIBinder* binder, AIBinder_DeathRecipient* recipient, + void* cookie) = 0; + }; + + // A real implementation for ILinkToDeath. + class AIBinderLinkToDeathImpl final : public ILinkToDeath { + public: + binder_status_t linkToDeath(AIBinder* binder, AIBinder_DeathRecipient* recipient, + void* cookie) override; + }; + + // OnBinderDiedContext is a type used as a cookie passed deathRecipient. The deathRecipient's + // onBinderDied function takes only a cookie as input and we have to store all the contexts + // as the cookie. + struct OnBinderDiedContext { + DefaultVehicleHal* vhal; + const AIBinder* clientId; + }; + // The default timeout of get or set value requests is 30s. // TODO(b/214605968): define TIMEOUT_IN_NANO in IVehicle and allow getValues/setValues/subscribe // to specify custom timeouts. @@ -142,15 +168,22 @@ class DefaultVehicleHal final : public ::aidl::android::hardware::automotive::ve std::shared_ptr mSubscriptionManager; std::mutex mLock; - std::unordered_map> mGetValuesClients + std::unordered_map> mOnBinderDiedContexts GUARDED_BY(mLock); - std::unordered_map> mSetValuesClients + std::unordered_map> mGetValuesClients + GUARDED_BY(mLock); + std::unordered_map> mSetValuesClients GUARDED_BY(mLock); // SubscriptionClients is thread-safe. std::shared_ptr mSubscriptionClients; + // mLinkToDeathImpl is only going to be changed in test. + std::unique_ptr mLinkToDeathImpl; + // RecurrentTimer is thread-safe. RecurrentTimer mRecurrentTimer; + ::ndk::ScopedAIBinder_DeathRecipient mDeathRecipient; + ::android::base::Result checkProperty( const ::aidl::android::hardware::automotive::vehicle::VehiclePropValue& propValue); @@ -176,9 +209,15 @@ class DefaultVehicleHal final : public ::aidl::android::hardware::automotive::ve const ::aidl::android::hardware::automotive::vehicle::VehiclePropConfig*> getConfig(int32_t propId) const; + void onBinderDiedWithContext(const AIBinder* clientId); + + void onBinderUnlinkedWithContext(const AIBinder* clientId); + + void monitorBinderLifeCycle(const CallbackType& callback); + template static std::shared_ptr getOrCreateClient( - std::unordered_map>* clients, + std::unordered_map>* clients, const CallbackType& callback, std::shared_ptr pendingRequestPool); static void getValueFromHardwareCallCallback( @@ -195,9 +234,16 @@ class DefaultVehicleHal final : public ::aidl::android::hardware::automotive::ve static void checkHealth(std::weak_ptr hardware, std::weak_ptr subscriptionManager); + static void onBinderDied(void* cookie); + + static void onBinderUnlinked(void* cookie); + // Test-only // Set the default timeout for pending requests. void setTimeout(int64_t timeoutInNano); + + // Test-only + void setLinkToDeathImpl(std::unique_ptr impl); }; } // namespace vehicle diff --git a/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h b/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h index 28809c61d1..e739c8c3f2 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h +++ b/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h @@ -38,6 +38,7 @@ namespace vehicle { // A thread-safe subscription manager that manages all VHAL subscriptions. class SubscriptionManager final { public: + using ClientIdType = const AIBinder*; using CallbackType = std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback>; using GetValueFunc = std::function unsubscribe(const CallbackType& callback, + // Returns ok if all the requested properties for the client are unsubscribed. + ::android::base::Result unsubscribe(ClientIdType client, const std::vector& propIds); - // Unsubscribes to all the properties for the callback. - // Returns error if the callback was not subscribed before. If error is returned, no property + // Unsubscribes from all the properties for the client. + // Returns error if the client was not subscribed before. If error is returned, no property // would be unsubscribed. - // Returns ok if all the properties for the callback are unsubscribed. - ::android::base::Result unsubscribe(const CallbackType& callback); + // Returns ok if all the properties for the client are unsubscribed. + ::android::base::Result unsubscribe(ClientIdType client); // For a list of updated properties, returns a map that maps clients subscribing to // the updated properties to a list of updated values. This would only return on-change property // clients that should be informed for the given updated values. std::unordered_map< - std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback>, + CallbackType, std::vector> getSubscribedClients( const std::vector<::aidl::android::hardware::automotive::vehicle::VehiclePropValue>& @@ -86,6 +87,9 @@ class SubscriptionManager final { static bool checkSampleRate(float sampleRate); private: + // Friend class for testing. + friend class DefaultVehicleHalTest; + struct PropIdAreaId { int32_t propId; int32_t areaId; @@ -131,9 +135,10 @@ class SubscriptionManager final { }; mutable std::mutex mLock; - std::unordered_map, PropIdAreaIdHash> + std::unordered_map, + PropIdAreaIdHash> mClientsByPropIdArea GUARDED_BY(mLock); - std::unordered_map, + std::unordered_map, PropIdAreaIdHash>> mSubscriptionsByClient GUARDED_BY(mLock); // RecurrentTimer is thread-safe. @@ -141,6 +146,9 @@ class SubscriptionManager final { const GetValueFunc mGetValue; static ::android::base::Result getInterval(float sampleRate); + + // Checks whether the manager is empty. For testing purpose. + bool isEmpty(); }; } // namespace vehicle diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp index c4cbc68c8c..3e088c5ee8 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp @@ -42,7 +42,6 @@ using ::aidl::android::hardware::automotive::vehicle::GetValueRequest; using ::aidl::android::hardware::automotive::vehicle::GetValueRequests; using ::aidl::android::hardware::automotive::vehicle::GetValueResult; using ::aidl::android::hardware::automotive::vehicle::GetValueResults; -using ::aidl::android::hardware::automotive::vehicle::IVehicleCallback; using ::aidl::android::hardware::automotive::vehicle::SetValueRequest; using ::aidl::android::hardware::automotive::vehicle::SetValueRequests; using ::aidl::android::hardware::automotive::vehicle::SetValueResult; @@ -62,6 +61,8 @@ using ::android::base::Error; using ::android::base::expected; using ::android::base::Result; using ::android::base::StringPrintf; + +using ::ndk::ScopedAIBinder_DeathRecipient; using ::ndk::ScopedAStatus; std::string toString(const std::unordered_set& values) { @@ -86,10 +87,15 @@ std::shared_ptr DefaultVehicleHal::SubscriptionClients::getC int64_t DefaultVehicleHal::SubscribeIdByClient::getId(const CallbackType& callback) { std::scoped_lock lockGuard(mLock); // This would be initialized to 0 if callback does not exist in the map. - int64_t subscribeId = (mIds[callback])++; + int64_t subscribeId = (mIds[callback->asBinder().get()])++; return subscribeId; } +void DefaultVehicleHal::SubscriptionClients::removeClient(const AIBinder* clientId) { + std::scoped_lock lockGuard(mLock); + mClients.erase(clientId); +} + size_t DefaultVehicleHal::SubscriptionClients::countClients() { std::scoped_lock lockGuard(mLock); return mClients.size(); @@ -139,6 +145,17 @@ DefaultVehicleHal::DefaultVehicleHal(std::unique_ptr hardware) std::make_shared>([hardwareCopy, subscriptionManagerCopy]() { checkHealth(hardwareCopy, subscriptionManagerCopy); })); + + mLinkToDeathImpl = std::make_unique(); + mDeathRecipient = ScopedAIBinder_DeathRecipient( + AIBinder_DeathRecipient_new(&DefaultVehicleHal::onBinderDied)); + AIBinder_DeathRecipient_setOnUnlinked(mDeathRecipient.get(), + &DefaultVehicleHal::onBinderUnlinked); +} + +DefaultVehicleHal::~DefaultVehicleHal() { + // Delete the deathRecipient so that onBinderDied would not be called to reference 'this'. + mDeathRecipient = ScopedAIBinder_DeathRecipient(); } void DefaultVehicleHal::onPropertyChangeEvent( @@ -161,26 +178,73 @@ void DefaultVehicleHal::onPropertyChangeEvent( template std::shared_ptr DefaultVehicleHal::getOrCreateClient( - std::unordered_map>* clients, const CallbackType& callback, - std::shared_ptr pendingRequestPool) { - if (clients->find(callback) == clients->end()) { - // TODO(b/204943359): Remove client from clients when linkToDeath is implemented. - (*clients)[callback] = std::make_shared(pendingRequestPool, callback); + std::unordered_map>* clients, + const CallbackType& callback, std::shared_ptr pendingRequestPool) { + const AIBinder* clientId = callback->asBinder().get(); + if (clients->find(clientId) == clients->end()) { + (*clients)[clientId] = std::make_shared(pendingRequestPool, callback); } - return (*clients)[callback]; + return (*clients)[clientId]; +} + +void DefaultVehicleHal::monitorBinderLifeCycle(const CallbackType& callback) { + AIBinder* clientId = callback->asBinder().get(); + { + std::scoped_lock lockGuard(mLock); + if (mOnBinderDiedContexts.find(clientId) != mOnBinderDiedContexts.end()) { + // Already registered. + return; + } + } + + std::unique_ptr context = std::make_unique( + OnBinderDiedContext{.vhal = this, .clientId = clientId}); + binder_status_t status = mLinkToDeathImpl->linkToDeath(clientId, mDeathRecipient.get(), + static_cast(context.get())); + if (status == STATUS_OK) { + std::scoped_lock lockGuard(mLock); + // Insert into a map to keep the context object alive. + mOnBinderDiedContexts[clientId] = std::move(context); + } else { + ALOGE("failed to call linkToDeath on client binder, status: %d", static_cast(status)); + } +} + +void DefaultVehicleHal::onBinderDied(void* cookie) { + OnBinderDiedContext* context = reinterpret_cast(cookie); + context->vhal->onBinderDiedWithContext(context->clientId); +} + +void DefaultVehicleHal::onBinderDiedWithContext(const AIBinder* clientId) { + std::scoped_lock lockGuard(mLock); + mSetValuesClients.erase(clientId); + mGetValuesClients.erase(clientId); + mSubscriptionClients->removeClient(clientId); + mSubscriptionManager->unsubscribe(clientId); +} + +void DefaultVehicleHal::onBinderUnlinked(void* cookie) { + // Delete the context associated with this cookie. + OnBinderDiedContext* context = reinterpret_cast(cookie); + context->vhal->onBinderUnlinkedWithContext(context->clientId); +} + +void DefaultVehicleHal::onBinderUnlinkedWithContext(const AIBinder* clientId) { + std::scoped_lock lockGuard(mLock); + mOnBinderDiedContexts.erase(clientId); } template std::shared_ptr DefaultVehicleHal::getOrCreateClient( - std::unordered_map>* clients, + std::unordered_map>* clients, const CallbackType& callback, std::shared_ptr pendingRequestPool); template std::shared_ptr DefaultVehicleHal::getOrCreateClient( - std::unordered_map>* clients, + std::unordered_map>* clients, const CallbackType& callback, std::shared_ptr pendingRequestPool); template std::shared_ptr DefaultVehicleHal::getOrCreateClient( - std::unordered_map>* clients, + std::unordered_map>* clients, const CallbackType& callback, std::shared_ptr pendingRequestPool); void DefaultVehicleHal::getValueFromHardwareCallCallback( @@ -268,6 +332,8 @@ Result DefaultVehicleHal::checkProperty(const VehiclePropValue& propValue) ScopedAStatus DefaultVehicleHal::getValues(const CallbackType& callback, const GetValueRequests& requests) { + monitorBinderLifeCycle(callback); + expected, ScopedAStatus> deserializedResults = fromStableLargeParcelable(requests); if (!deserializedResults.ok()) { @@ -344,6 +410,8 @@ ScopedAStatus DefaultVehicleHal::getValues(const CallbackType& callback, ScopedAStatus DefaultVehicleHal::setValues(const CallbackType& callback, const SetValueRequests& requests) { + monitorBinderLifeCycle(callback); + expected, ScopedAStatus> deserializedResults = fromStableLargeParcelable(requests); if (!deserializedResults.ok()) { @@ -526,6 +594,8 @@ Result DefaultVehicleHal::checkSubscribeOptions( ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback, const std::vector& options, [[maybe_unused]] int32_t maxSharedMemoryFileCount) { + monitorBinderLifeCycle(callback); + // TODO(b/205189110): Use shared memory file count. if (auto result = checkSubscribeOptions(options); !result.ok()) { ALOGE("subscribe: invalid subscribe options: %s", getErrorMsg(result).c_str()); @@ -571,7 +641,7 @@ ScopedAStatus DefaultVehicleHal::subscribe(const CallbackType& callback, ScopedAStatus DefaultVehicleHal::unsubscribe(const CallbackType& callback, const std::vector& propIds) { - return toScopedAStatus(mSubscriptionManager->unsubscribe(callback, propIds), + return toScopedAStatus(mSubscriptionManager->unsubscribe(callback->asBinder().get(), propIds), StatusCode::INVALID_ARG); } @@ -639,6 +709,15 @@ void DefaultVehicleHal::checkHealth(std::weak_ptr hardware, return; } +binder_status_t DefaultVehicleHal::AIBinderLinkToDeathImpl::linkToDeath( + AIBinder* binder, AIBinder_DeathRecipient* recipient, void* cookie) { + return AIBinder_linkToDeath(binder, recipient, cookie); +} + +void DefaultVehicleHal::setLinkToDeathImpl(std::unique_ptr impl) { + mLinkToDeathImpl = std::move(impl); +} + } // namespace vehicle } // namespace automotive } // namespace hardware diff --git a/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp b/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp index ff996fec3d..21bfba6e3a 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp @@ -26,7 +26,7 @@ namespace vehicle { namespace { -constexpr float ONE_SECOND_IN_NANO = 1000000000.; +constexpr float ONE_SECOND_IN_NANO = 1'000'000'000.; } // namespace @@ -100,6 +100,7 @@ Result SubscriptionManager::subscribe(const std::shared_ptrasBinder().get(); for (const auto& option : options) { int32_t propId = option.propId; const std::vector& areaIds = option.areaIds; @@ -118,7 +119,7 @@ Result SubscriptionManager::subscribe(const std::shared_ptr( mTimer, [this, callback, propValueRequest] { @@ -126,24 +127,24 @@ Result SubscriptionManager::subscribe(const std::shared_ptr(); } - mClientsByPropIdArea[propIdAreaId].insert(callback); + mClientsByPropIdArea[propIdAreaId][clientId] = callback; } } return {}; } -Result SubscriptionManager::unsubscribe(const std::shared_ptr& callback, +Result SubscriptionManager::unsubscribe(SubscriptionManager::ClientIdType clientId, const std::vector& propIds) { std::scoped_lock lockGuard(mLock); - if (mSubscriptionsByClient.find(callback) == mSubscriptionsByClient.end()) { + if (mSubscriptionsByClient.find(clientId) == mSubscriptionsByClient.end()) { return Error() << "No property was subscribed for the callback"; } std::unordered_set subscribedPropIds; - for (auto const& [propIdAreaId, _] : mSubscriptionsByClient[callback]) { + for (auto const& [propIdAreaId, _] : mSubscriptionsByClient[clientId]) { subscribedPropIds.insert(propIdAreaId.propId); } @@ -153,13 +154,13 @@ Result SubscriptionManager::unsubscribe(const std::shared_ptrfirst.propId; if (std::find(propIds.begin(), propIds.end(), propId) != propIds.end()) { auto& clients = mClientsByPropIdArea[it->first]; - clients.erase(callback); + clients.erase(clientId); if (clients.empty()) { mClientsByPropIdArea.erase(it->first); } @@ -169,27 +170,27 @@ Result SubscriptionManager::unsubscribe(const std::shared_ptr SubscriptionManager::unsubscribe(const std::shared_ptr& callback) { +Result SubscriptionManager::unsubscribe(SubscriptionManager::ClientIdType clientId) { std::scoped_lock lockGuard(mLock); - if (mSubscriptionsByClient.find(callback) == mSubscriptionsByClient.end()) { - return Error() << "No property was subscribed for the callback"; + if (mSubscriptionsByClient.find(clientId) == mSubscriptionsByClient.end()) { + return Error() << "No property was subscribed for this client"; } - auto& subscriptions = mSubscriptionsByClient[callback]; + auto& subscriptions = mSubscriptionsByClient[clientId]; for (auto const& [propIdAreaId, _] : subscriptions) { auto& clients = mClientsByPropIdArea[propIdAreaId]; - clients.erase(callback); + clients.erase(clientId); if (clients.empty()) { mClientsByPropIdArea.erase(propIdAreaId); } } - mSubscriptionsByClient.erase(callback); + mSubscriptionsByClient.erase(clientId); return {}; } @@ -207,8 +208,8 @@ SubscriptionManager::getSubscribedClients(const std::vector& u if (mClientsByPropIdArea.find(propIdAreaId) == mClientsByPropIdArea.end()) { continue; } - for (const auto& client : mClientsByPropIdArea[propIdAreaId]) { - if (!mSubscriptionsByClient[client][propIdAreaId]->isOnChange()) { + for (const auto& [clientId, client] : mClientsByPropIdArea[propIdAreaId]) { + if (!mSubscriptionsByClient[clientId][propIdAreaId]->isOnChange()) { continue; } clients[client].push_back(&value); @@ -217,6 +218,11 @@ SubscriptionManager::getSubscribedClients(const std::vector& u return clients; } +bool SubscriptionManager::isEmpty() { + std::scoped_lock lockGuard(mLock); + return mSubscriptionsByClient.empty() && mClientsByPropIdArea.empty(); +} + SubscriptionManager::RecurrentSubscription::RecurrentSubscription( std::shared_ptr timer, std::function&& action, int64_t interval) : mAction(std::make_shared>(action)), mTimer(timer) { diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp index 54fc17d29c..ff355c33e1 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp @@ -73,6 +73,7 @@ using ::android::base::Result; using ::ndk::ScopedAStatus; using ::ndk::ScopedFileDescriptor; +using ::ndk::SpAIBinder; using ::testing::Eq; using ::testing::UnorderedElementsAre; @@ -324,7 +325,12 @@ class DefaultVehicleHalTest : public ::testing::Test { mVhal = ndk::SharedRefBase::make(std::move(hardware)); mVhalClient = IVehicle::fromBinder(mVhal->asBinder()); mCallback = ndk::SharedRefBase::make(); - mCallbackClient = IVehicleCallback::fromBinder(mCallback->asBinder()); + // Keep the local binder alive. + mBinder = mCallback->asBinder(); + mCallbackClient = IVehicleCallback::fromBinder(mBinder); + + // Set the linkToDeath to a fake implementation that always returns OK. + setTestLinkToDeathImpl(); } void TearDown() override { @@ -342,10 +348,36 @@ class DefaultVehicleHalTest : public ::testing::Test { void setTimeout(int64_t timeoutInNano) { mVhal->setTimeout(timeoutInNano); } + void setTestLinkToDeathImpl() { + mVhal->setLinkToDeathImpl(std::make_unique()); + } + size_t countPendingRequests() { return mVhal->mPendingRequestPool->countPendingRequests(); } + size_t countClients() { + std::scoped_lock lockGuard(mVhal->mLock); + return mVhal->mGetValuesClients.size() + mVhal->mSetValuesClients.size() + + mVhal->mSubscriptionClients->countClients(); + } + std::shared_ptr getPool() { return mVhal->mPendingRequestPool; } + void onBinderDied(void* cookie) { return mVhal->onBinderDied(cookie); } + + void onBinderUnlinked(void* cookie) { return mVhal->onBinderUnlinked(cookie); } + + void* getOnBinderDiedContexts(AIBinder* clientId) { + std::scoped_lock lockGuard(mVhal->mLock); + return mVhal->mOnBinderDiedContexts[clientId].get(); + } + + bool countOnBinderDiedContexts() { + std::scoped_lock lockGuard(mVhal->mLock); + return mVhal->mOnBinderDiedContexts.size(); + } + + bool hasNoSubscriptions() { return mVhal->mSubscriptionManager->isEmpty(); } + static Result getValuesTestCases(size_t size, GetValueRequests& requests, std::vector& expectedResults, std::vector& expectedHardwareRequests) { @@ -416,18 +448,20 @@ class DefaultVehicleHalTest : public ::testing::Test { return {}; } - size_t countClients() { - std::scoped_lock lockGuard(mVhal->mLock); - return mVhal->mGetValuesClients.size() + mVhal->mSetValuesClients.size() + - mVhal->mSubscriptionClients->countClients(); - } - private: std::shared_ptr mVhal; std::shared_ptr mVhalClient; MockVehicleHardware* mHardwarePtr; std::shared_ptr mCallback; std::shared_ptr mCallbackClient; + SpAIBinder mBinder; + + class TestLinkToDeathImpl final : public DefaultVehicleHal::ILinkToDeath { + public: + binder_status_t linkToDeath(AIBinder*, AIBinder_DeathRecipient*, void*) override { + return STATUS_OK; + } + }; }; TEST_F(DefaultVehicleHalTest, testGetAllPropConfigsSmall) { @@ -1445,6 +1479,71 @@ TEST_F(DefaultVehicleHalTest, testHeartbeatEvent) { << "expect to get the latest timestamp with the heartbeat event"; } +TEST_F(DefaultVehicleHalTest, testOnBinderDiedUnlinked) { + // First subscribe to a continuous property so that we register a death recipient for our + // client. + VehiclePropValue testValue{ + .prop = GLOBAL_CONTINUOUS_PROP, + .value.int32Values = {0}, + }; + // Set responses for all the hardware getValues requests. + getHardware()->setGetValueResponder( + [](std::shared_ptr callback, + const std::vector& requests) { + std::vector results; + for (auto& request : requests) { + VehiclePropValue prop = request.prop; + prop.value.int32Values = {0}; + results.push_back({ + .requestId = request.requestId, + .status = StatusCode::OK, + .prop = prop, + }); + } + (*callback)(results); + return StatusCode::OK; + }); + std::vector options = { + { + .propId = GLOBAL_CONTINUOUS_PROP, + .sampleRate = 20.0, + }, + }; + auto status = getClient()->subscribe(getCallbackClient(), options, 0); + ASSERT_TRUE(status.isOk()) << "subscribe failed: " << status.getMessage(); + // Sleep for 100ms so that the subscriptionClient gets created because we would at least try to + // get value once. + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + + // Issue another getValue request on the same client. + GetValueRequests requests; + std::vector expectedResults; + std::vector expectedHardwareRequests; + ASSERT_TRUE(getValuesTestCases(1, requests, expectedResults, expectedHardwareRequests).ok()); + getHardware()->addGetValueResponses(expectedResults); + status = getClient()->getValues(getCallbackClient(), requests); + ASSERT_TRUE(status.isOk()) << "getValues failed: " << status.getMessage(); + + ASSERT_EQ(countOnBinderDiedContexts(), static_cast(1)) + << "expect one OnBinderDied context when one client is registered"; + + // Get the death recipient cookie for our callback that would be used in onBinderDied and + // onBinderUnlinked. + AIBinder* clientId = getCallbackClient()->asBinder().get(); + void* context = getOnBinderDiedContexts(clientId); + + onBinderDied(context); + + ASSERT_EQ(countClients(), static_cast(0)) + << "expect all clients to be removed when binder died"; + ASSERT_TRUE(hasNoSubscriptions()) << "expect no subscriptions when binder died"; + + onBinderUnlinked(context); + + ASSERT_EQ(countOnBinderDiedContexts(), static_cast(0)) + << "expect OnBinderDied context to be deleted when binder is unlinked"; +} + } // namespace vehicle } // namespace automotive } // namespace hardware diff --git a/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp index c14f4fdcc0..f81b1a2998 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp @@ -45,6 +45,7 @@ using ::aidl::android::hardware::automotive::vehicle::VehiclePropErrors; using ::aidl::android::hardware::automotive::vehicle::VehiclePropValue; using ::aidl::android::hardware::automotive::vehicle::VehiclePropValues; using ::ndk::ScopedAStatus; +using ::ndk::SpAIBinder; using ::testing::ElementsAre; using ::testing::WhenSorted; @@ -95,7 +96,9 @@ class SubscriptionManagerTest : public ::testing::Test { 0); }); mCallback = ::ndk::SharedRefBase::make(); - mCallbackClient = IVehicleCallback::fromBinder(mCallback->asBinder()); + // Keep the local binder alive. + mBinder = mCallback->asBinder(); + mCallbackClient = IVehicleCallback::fromBinder(mBinder); } SubscriptionManager* getManager() { return mManager.get(); } @@ -112,6 +115,7 @@ class SubscriptionManagerTest : public ::testing::Test { std::unique_ptr mManager; std::shared_ptr mCallback; std::shared_ptr mCallbackClient; + SpAIBinder mBinder; }; TEST_F(SubscriptionManagerTest, testSubscribeGlobalContinuous) { @@ -229,7 +233,7 @@ TEST_F(SubscriptionManagerTest, testUnsubscribeGlobalContinuous) { auto result = getManager()->subscribe(getCallbackClient(), options, true); ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); - result = getManager()->unsubscribe(getCallbackClient()); + result = getManager()->unsubscribe(getCallbackClient()->asBinder().get()); ASSERT_TRUE(result.ok()) << "failed to unsubscribe: " << result.error().message(); clearEvents(); @@ -257,7 +261,8 @@ TEST_F(SubscriptionManagerTest, testUnsubscribeMultipleAreas) { auto result = getManager()->subscribe(getCallbackClient(), options, true); ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); - result = getManager()->unsubscribe(getCallbackClient(), std::vector({0})); + result = getManager()->unsubscribe(getCallbackClient()->asBinder().get(), + std::vector({0})); ASSERT_TRUE(result.ok()) << "failed to unsubscribe: " << result.error().message(); clearEvents(); @@ -289,7 +294,7 @@ TEST_F(SubscriptionManagerTest, testUnsubscribeByCallback) { auto result = getManager()->subscribe(getCallbackClient(), options, true); ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); - result = getManager()->unsubscribe(getCallbackClient()); + result = getManager()->unsubscribe(getCallbackClient()->asBinder().get()); ASSERT_TRUE(result.ok()) << "failed to unsubscribe: " << result.error().message(); clearEvents(); @@ -315,12 +320,14 @@ TEST_F(SubscriptionManagerTest, testUnsubscribeFailure) { ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); // Property ID: 2 was not subscribed. - result = getManager()->unsubscribe(getCallbackClient(), std::vector({0, 1, 2})); + result = getManager()->unsubscribe(getCallbackClient()->asBinder().get(), + std::vector({0, 1, 2})); ASSERT_FALSE(result.ok()) << "unsubscribe an unsubscribed property must fail"; // Since property 0 and property 1 was not unsubscribed successfully, we should be able to // unsubscribe them again. - result = getManager()->unsubscribe(getCallbackClient(), std::vector({0, 1})); + result = getManager()->unsubscribe(getCallbackClient()->asBinder().get(), + std::vector({0, 1})); ASSERT_TRUE(result.ok()) << "a failed unsubscription must not unsubscribe any properties" << result.error().message(); } @@ -343,10 +350,10 @@ TEST_F(SubscriptionManagerTest, testSubscribeOnchange) { }, }; - std::shared_ptr client1 = IVehicleCallback::fromBinder( - ::ndk::SharedRefBase::make()->asBinder()); - std::shared_ptr client2 = IVehicleCallback::fromBinder( - ::ndk::SharedRefBase::make()->asBinder()); + SpAIBinder binder1 = ::ndk::SharedRefBase::make()->asBinder(); + std::shared_ptr client1 = IVehicleCallback::fromBinder(binder1); + SpAIBinder binder2 = ::ndk::SharedRefBase::make()->asBinder(); + std::shared_ptr client2 = IVehicleCallback::fromBinder(binder2); auto result = getManager()->subscribe(client1, options1, false); ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); result = getManager()->subscribe(client2, options2, false); @@ -447,7 +454,8 @@ TEST_F(SubscriptionManagerTest, testUnsubscribeOnchange) { auto result = getManager()->subscribe(getCallbackClient(), options, false); ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); - result = getManager()->unsubscribe(getCallbackClient(), std::vector({0})); + result = getManager()->unsubscribe(getCallbackClient()->asBinder().get(), + std::vector({0})); ASSERT_TRUE(result.ok()) << "failed to unsubscribe: " << result.error().message(); std::vector updatedValues = {