diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp index f18aaa5a9ea0c..cd10457dcd404 100644 --- a/cmds/statsd/src/StatsService.cpp +++ b/cmds/statsd/src/StatsService.cpp @@ -930,8 +930,6 @@ Status StatsService::informOnePackage(const string& app, int32_t uid, int64_t ve ENFORCE_UID(AID_SYSTEM); VLOG("StatsService::informOnePackage was called"); - // TODO(b/149254662): This is gross. We should consider changing statsd - // internals to use std::string. String16 utf16App = String16(app.c_str()); String16 utf16VersionString = String16(versionString.c_str()); String16 utf16Installer = String16(installer.c_str()); diff --git a/cmds/statsd/src/anomaly/AlarmMonitor.cpp b/cmds/statsd/src/anomaly/AlarmMonitor.cpp index 02291181b81b6..b632d040eb43a 100644 --- a/cmds/statsd/src/anomaly/AlarmMonitor.cpp +++ b/cmds/statsd/src/anomaly/AlarmMonitor.cpp @@ -38,8 +38,6 @@ AlarmMonitor::~AlarmMonitor() {} void AlarmMonitor::setStatsCompanionService( shared_ptr statsCompanionService) { std::lock_guard lock(mLock); - // TODO(b/149254662): determine if tmpForLock is needed now that we have moved - // from sp to shared_ptr shared_ptr tmpForLock = mStatsCompanionService; mStatsCompanionService = statsCompanionService; if (statsCompanionService == nullptr) { diff --git a/cmds/statsd/src/config/ConfigManager.cpp b/cmds/statsd/src/config/ConfigManager.cpp index 9bdb5881cc944..6d9c644bb40eb 100644 --- a/cmds/statsd/src/config/ConfigManager.cpp +++ b/cmds/statsd/src/config/ConfigManager.cpp @@ -54,20 +54,22 @@ struct ConfigReceiverDeathCookie { shared_ptr mPir; }; -static void configReceiverDied(void* cookie) { +void ConfigManager::configReceiverDied(void* cookie) { auto cookie_ = static_cast(cookie); - sp configManager = cookie_->mConfigManager; - ConfigKey configKey = cookie_->mConfigKey; - shared_ptr pir = cookie_->mPir; + sp& thiz = cookie_->mConfigManager; + ConfigKey& configKey = cookie_->mConfigKey; + shared_ptr& pir = cookie_->mPir; - // TODO(b/149254662): Fix threading. This currently fails if a new - // pir gets set between the get and the remove. - if (configManager->GetConfigReceiver(configKey) == pir) { - configManager->RemoveConfigReceiver(configKey); + // Erase the mapping from the config key to the config receiver (pir) if the + // mapping still exists. + lock_guard lock(thiz->mMutex); + auto it = thiz->mConfigReceivers.find(configKey); + if (it != thiz->mConfigReceivers.end() && it->second == pir) { + thiz->mConfigReceivers.erase(configKey); } - // The death recipient corresponding to this specific pir can never - // be triggered again, so free up resources. - // TODO(b/149254662): Investigate other options to manage the memory. + + // The death recipient corresponding to this specific pir can never be + // triggered again, so free up resources. delete cookie_; } @@ -83,26 +85,29 @@ struct ActiveConfigChangedReceiverDeathCookie { shared_ptr mPir; }; -static void activeConfigChangedReceiverDied(void* cookie) { +void ConfigManager::activeConfigChangedReceiverDied(void* cookie) { auto cookie_ = static_cast(cookie); - sp configManager = cookie_->mConfigManager; + sp& thiz = cookie_->mConfigManager; int uid = cookie_->mUid; - shared_ptr pir = cookie_->mPir; + shared_ptr& pir = cookie_->mPir; - // TODO(b/149254662): Fix threading. This currently fails if a new - // pir gets set between the get and the remove. - if (configManager->GetActiveConfigsChangedReceiver(uid) == pir) { - configManager->RemoveActiveConfigsChangedReceiver(uid); + // Erase the mapping from the config key to the active config changed + // receiver (pir) if the mapping still exists. + lock_guard lock(thiz->mMutex); + auto it = thiz->mActiveConfigsChangedReceivers.find(uid); + if (it != thiz->mActiveConfigsChangedReceivers.end() && it->second == pir) { + thiz->mActiveConfigsChangedReceivers.erase(uid); } + // The death recipient corresponding to this specific pir can never // be triggered again, so free up resources. delete cookie_; } -ConfigManager::ConfigManager(): +ConfigManager::ConfigManager() : mConfigReceiverDeathRecipient(AIBinder_DeathRecipient_new(configReceiverDied)), mActiveConfigChangedReceiverDeathRecipient( - AIBinder_DeathRecipient_new(activeConfigChangedReceiverDied)) { + AIBinder_DeathRecipient_new(activeConfigChangedReceiverDied)) { } ConfigManager::~ConfigManager() { @@ -189,8 +194,10 @@ void ConfigManager::RemoveConfigReceiver(const ConfigKey& key) { void ConfigManager::SetActiveConfigsChangedReceiver(const int uid, const shared_ptr& pir) { - lock_guard lock(mMutex); - mActiveConfigsChangedReceivers[uid] = pir; + { + lock_guard lock(mMutex); + mActiveConfigsChangedReceivers[uid] = pir; + } AIBinder_linkToDeath(pir->asBinder().get(), mActiveConfigChangedReceiverDeathRecipient.get(), new ActiveConfigChangedReceiverDeathCookie(this, uid, pir)); } diff --git a/cmds/statsd/src/config/ConfigManager.h b/cmds/statsd/src/config/ConfigManager.h index 824e58849b789..40146b1b2becb 100644 --- a/cmds/statsd/src/config/ConfigManager.h +++ b/cmds/statsd/src/config/ConfigManager.h @@ -161,6 +161,19 @@ private: // IPendingIntentRef dies. ::ndk::ScopedAIBinder_DeathRecipient mConfigReceiverDeathRecipient; ::ndk::ScopedAIBinder_DeathRecipient mActiveConfigChangedReceiverDeathRecipient; + + /** + * Death recipient callback that is called when a config receiver dies. + * The cookie is a pointer to a ConfigReceiverDeathCookie. + */ + static void configReceiverDied(void* cookie); + + /** + * Death recipient callback that is called when an active config changed + * receiver dies. The cookie is a pointer to an + * ActiveConfigChangedReceiverDeathCookie. + */ + static void activeConfigChangedReceiverDied(void* cookie); }; } // namespace statsd diff --git a/cmds/statsd/src/external/StatsPullerManager.cpp b/cmds/statsd/src/external/StatsPullerManager.cpp index 1c38542b90177..0115ba2b6bc0e 100644 --- a/cmds/statsd/src/external/StatsPullerManager.cpp +++ b/cmds/statsd/src/external/StatsPullerManager.cpp @@ -85,12 +85,9 @@ void StatsPullerManager::updateAlarmLocked() { return; } - // TODO(b/149254662): Why are we creating a copy here? This is different - // from the other places where we create a copy because we don't reassign - // mStatsCompanionService so a destructor can't implicitly be called... - shared_ptr statsCompanionServiceCopy = mStatsCompanionService; - if (statsCompanionServiceCopy != nullptr) { - statsCompanionServiceCopy->setPullingAlarm(mNextPullTimeNs / 1000000); + // TODO(b/151045771): do not hold a lock while making a binder call + if (mStatsCompanionService != nullptr) { + mStatsCompanionService->setPullingAlarm(mNextPullTimeNs / 1000000); } else { VLOG("StatsCompanionService not available. Alarm not set."); } @@ -99,8 +96,6 @@ void StatsPullerManager::updateAlarmLocked() { void StatsPullerManager::SetStatsCompanionService( shared_ptr statsCompanionService) { - // TODO(b/149254662): Why are we using AutoMutex instead of lock_guard? - // Additionally, do we need the temporary shared_ptr to prevent deadlocks? AutoMutex _l(mLock); shared_ptr tmpForLock = mStatsCompanionService; mStatsCompanionService = statsCompanionService; diff --git a/cmds/statsd/src/subscriber/SubscriberReporter.cpp b/cmds/statsd/src/subscriber/SubscriberReporter.cpp index 93af5e9372283..c915ef3bf0698 100644 --- a/cmds/statsd/src/subscriber/SubscriberReporter.cpp +++ b/cmds/statsd/src/subscriber/SubscriberReporter.cpp @@ -39,33 +39,47 @@ struct BroadcastSubscriberDeathCookie { shared_ptr mPir; }; -static void broadcastSubscriberDied(void* cookie) { - BroadcastSubscriberDeathCookie* cookie_ = (BroadcastSubscriberDeathCookie*)cookie; - ConfigKey configKey = cookie_->mConfigKey; +void SubscriberReporter::broadcastSubscriberDied(void* cookie) { + auto cookie_ = static_cast(cookie); + ConfigKey& configKey = cookie_->mConfigKey; int64_t subscriberId = cookie_->mSubscriberId; - shared_ptr pir = cookie_->mPir; + shared_ptr& pir = cookie_->mPir; - // TODO(b/149254662): Fix threading. This currently fails if a new pir gets - // set between the get and the unset. - if (SubscriberReporter::getInstance().getBroadcastSubscriber(configKey, subscriberId) == pir) { - SubscriberReporter::getInstance().unsetBroadcastSubscriber(configKey, subscriberId); + SubscriberReporter& thiz = getInstance(); + + // Erase the mapping from a (config_key, subscriberId) to a pir if the + // mapping exists. + lock_guard lock(thiz.mLock); + auto subscriberMapIt = thiz.mIntentMap.find(configKey); + if (subscriberMapIt != thiz.mIntentMap.end()) { + auto subscriberMap = subscriberMapIt->second; + auto pirIt = subscriberMap.find(subscriberId); + if (pirIt != subscriberMap.end() && pirIt->second == pir) { + subscriberMap.erase(subscriberId); + if (subscriberMap.empty()) { + thiz.mIntentMap.erase(configKey); + } + } } + // The death recipient corresponding to this specific pir can never be // triggered again, so free up resources. delete cookie_; } -static ::ndk::ScopedAIBinder_DeathRecipient sBroadcastSubscriberDeathRecipient( - AIBinder_DeathRecipient_new(broadcastSubscriberDied)); +SubscriberReporter::SubscriberReporter() : + mBroadcastSubscriberDeathRecipient(AIBinder_DeathRecipient_new(broadcastSubscriberDied)) { +} void SubscriberReporter::setBroadcastSubscriber(const ConfigKey& configKey, int64_t subscriberId, const shared_ptr& pir) { VLOG("SubscriberReporter::setBroadcastSubscriber called."); - lock_guard lock(mLock); - mIntentMap[configKey][subscriberId] = pir; - // TODO(b/149254662): Is it ok to call linkToDeath while holding a lock? - AIBinder_linkToDeath(pir->asBinder().get(), sBroadcastSubscriberDeathRecipient.get(), + { + lock_guard lock(mLock); + mIntentMap[configKey][subscriberId] = pir; + } + AIBinder_linkToDeath(pir->asBinder().get(), mBroadcastSubscriberDeathRecipient.get(), new BroadcastSubscriberDeathCookie(configKey, subscriberId, pir)); } @@ -103,8 +117,6 @@ void SubscriberReporter::alertBroadcastSubscriber(const ConfigKey& configKey, } int64_t subscriberId = subscription.broadcast_subscriber_details().subscriber_id(); - // TODO(b/149254662): Is there a way to convert a RepeatedPtrField into a - // vector without copying? vector cookies; cookies.reserve(subscription.broadcast_subscriber_details().cookie_size()); for (auto& cookie : subscription.broadcast_subscriber_details().cookie()) { diff --git a/cmds/statsd/src/subscriber/SubscriberReporter.h b/cmds/statsd/src/subscriber/SubscriberReporter.h index 0f97d397e3319..4fe428198e712 100644 --- a/cmds/statsd/src/subscriber/SubscriberReporter.h +++ b/cmds/statsd/src/subscriber/SubscriberReporter.h @@ -78,7 +78,7 @@ public: int64_t subscriberId); private: - SubscriberReporter() {}; + SubscriberReporter(); mutable mutex mLock; @@ -94,6 +94,14 @@ private: const Subscription& subscription, const vector& cookies, const MetricDimensionKey& dimKey) const; + + ::ndk::ScopedAIBinder_DeathRecipient mBroadcastSubscriberDeathRecipient; + + /** + * Death recipient callback that is called when a broadcast subscriber dies. + * The cookie is a pointer to a BroadcastSubscriberDeathCookie. + */ + static void broadcastSubscriberDied(void* cookie); }; } // namespace statsd