Fix TODOs made during migration to libbinder_ndk
Test: atest GtsStatsdHostTestCases
Bug: 149254662
Change-Id: I50a1998e693f9a0c447cd655f8efa11afa28803b
diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp
index f18aaa5..cd10457 100644
--- a/cmds/statsd/src/StatsService.cpp
+++ b/cmds/statsd/src/StatsService.cpp
@@ -930,8 +930,6 @@
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 02291181..b632d04 100644
--- a/cmds/statsd/src/anomaly/AlarmMonitor.cpp
+++ b/cmds/statsd/src/anomaly/AlarmMonitor.cpp
@@ -38,8 +38,6 @@
void AlarmMonitor::setStatsCompanionService(
shared_ptr<IStatsCompanionService> statsCompanionService) {
std::lock_guard<std::mutex> lock(mLock);
- // TODO(b/149254662): determine if tmpForLock is needed now that we have moved
- // from sp to shared_ptr
shared_ptr<IStatsCompanionService> tmpForLock = mStatsCompanionService;
mStatsCompanionService = statsCompanionService;
if (statsCompanionService == nullptr) {
diff --git a/cmds/statsd/src/config/ConfigManager.cpp b/cmds/statsd/src/config/ConfigManager.cpp
index 9bdb588..6d9c644 100644
--- a/cmds/statsd/src/config/ConfigManager.cpp
+++ b/cmds/statsd/src/config/ConfigManager.cpp
@@ -54,20 +54,22 @@
shared_ptr<IPendingIntentRef> mPir;
};
-static void configReceiverDied(void* cookie) {
+void ConfigManager::configReceiverDied(void* cookie) {
auto cookie_ = static_cast<ConfigReceiverDeathCookie*>(cookie);
- sp<ConfigManager> configManager = cookie_->mConfigManager;
- ConfigKey configKey = cookie_->mConfigKey;
- shared_ptr<IPendingIntentRef> pir = cookie_->mPir;
+ sp<ConfigManager>& thiz = cookie_->mConfigManager;
+ ConfigKey& configKey = cookie_->mConfigKey;
+ shared_ptr<IPendingIntentRef>& 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<mutex> 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 @@
shared_ptr<IPendingIntentRef> mPir;
};
-static void activeConfigChangedReceiverDied(void* cookie) {
+void ConfigManager::activeConfigChangedReceiverDied(void* cookie) {
auto cookie_ = static_cast<ActiveConfigChangedReceiverDeathCookie*>(cookie);
- sp<ConfigManager> configManager = cookie_->mConfigManager;
+ sp<ConfigManager>& thiz = cookie_->mConfigManager;
int uid = cookie_->mUid;
- shared_ptr<IPendingIntentRef> pir = cookie_->mPir;
+ shared_ptr<IPendingIntentRef>& 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<mutex> 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::SetActiveConfigsChangedReceiver(const int uid,
const shared_ptr<IPendingIntentRef>& pir) {
- lock_guard<mutex> lock(mMutex);
- mActiveConfigsChangedReceivers[uid] = pir;
+ {
+ lock_guard<mutex> 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 824e588..40146b1 100644
--- a/cmds/statsd/src/config/ConfigManager.h
+++ b/cmds/statsd/src/config/ConfigManager.h
@@ -161,6 +161,19 @@
// 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 1c38542..0115ba2 100644
--- a/cmds/statsd/src/external/StatsPullerManager.cpp
+++ b/cmds/statsd/src/external/StatsPullerManager.cpp
@@ -85,12 +85,9 @@
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<IStatsCompanionService> 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::SetStatsCompanionService(
shared_ptr<IStatsCompanionService> 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<IStatsCompanionService> tmpForLock = mStatsCompanionService;
mStatsCompanionService = statsCompanionService;
diff --git a/cmds/statsd/src/subscriber/SubscriberReporter.cpp b/cmds/statsd/src/subscriber/SubscriberReporter.cpp
index 93af5e9..c915ef3 100644
--- a/cmds/statsd/src/subscriber/SubscriberReporter.cpp
+++ b/cmds/statsd/src/subscriber/SubscriberReporter.cpp
@@ -39,33 +39,47 @@
shared_ptr<IPendingIntentRef> mPir;
};
-static void broadcastSubscriberDied(void* cookie) {
- BroadcastSubscriberDeathCookie* cookie_ = (BroadcastSubscriberDeathCookie*)cookie;
- ConfigKey configKey = cookie_->mConfigKey;
+void SubscriberReporter::broadcastSubscriberDied(void* cookie) {
+ auto cookie_ = static_cast<BroadcastSubscriberDeathCookie*>(cookie);
+ ConfigKey& configKey = cookie_->mConfigKey;
int64_t subscriberId = cookie_->mSubscriberId;
- shared_ptr<IPendingIntentRef> pir = cookie_->mPir;
+ shared_ptr<IPendingIntentRef>& 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<mutex> 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<IPendingIntentRef>& pir) {
VLOG("SubscriberReporter::setBroadcastSubscriber called.");
- lock_guard<mutex> 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<mutex> lock(mLock);
+ mIntentMap[configKey][subscriberId] = pir;
+ }
+ AIBinder_linkToDeath(pir->asBinder().get(), mBroadcastSubscriberDeathRecipient.get(),
new BroadcastSubscriberDeathCookie(configKey, subscriberId, pir));
}
@@ -103,8 +117,6 @@
}
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<string> 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 0f97d39..4fe4281 100644
--- a/cmds/statsd/src/subscriber/SubscriberReporter.h
+++ b/cmds/statsd/src/subscriber/SubscriberReporter.h
@@ -78,7 +78,7 @@
int64_t subscriberId);
private:
- SubscriberReporter() {};
+ SubscriberReporter();
mutable mutex mLock;
@@ -94,6 +94,14 @@
const Subscription& subscription,
const vector<string>& 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