From fdc123b6858b529d3a7661af241a94306816bfa9 Mon Sep 17 00:00:00 2001 From: David Chen Date: Fri, 9 Feb 2018 17:21:48 -0800 Subject: Adds locks to ConfigManager in statsd. The ConfigManager can be triggered by Binder threads, so we should lock any reads or writes to the config managers. We don't need to lock within StatsService, so those methods simply call the methods in ConfigManager. Test: Test that statsd builds and unit tests pass. Change-Id: I96904c4140a95dc60a419281c8c54f606a443fbc --- cmds/statsd/src/config/ConfigManager.cpp | 118 ++++++++++++++++++++++--------- cmds/statsd/src/config/ConfigManager.h | 5 +- 2 files changed, 88 insertions(+), 35 deletions(-) diff --git a/cmds/statsd/src/config/ConfigManager.cpp b/cmds/statsd/src/config/ConfigManager.cpp index fbb0fdde40a0..d0f55abe7033 100644 --- a/cmds/statsd/src/config/ConfigManager.cpp +++ b/cmds/statsd/src/config/ConfigManager.cpp @@ -59,45 +59,71 @@ void ConfigManager::StartupForTest() { } void ConfigManager::AddListener(const sp& listener) { + lock_guard lock(mMutex); mListeners.push_back(listener); } void ConfigManager::UpdateConfig(const ConfigKey& key, const StatsdConfig& config) { - // Add to set - mConfigs.insert(key); + vector> broadcastList; + { + lock_guard lock(mMutex); - // Save to disk - update_saved_configs(key, config); + // Add to set + mConfigs.insert(key); + + // Save to disk + update_saved_configs_locked(key, config); + + for (sp listener : mListeners) { + broadcastList.push_back(listener); + } + } // Tell everyone - for (auto& listener : mListeners) { + for (sp listener:broadcastList) { listener->OnConfigUpdated(key, config); } } void ConfigManager::SetConfigReceiver(const ConfigKey& key, const sp& intentSender) { + lock_guard lock(mMutex); mConfigReceivers[key] = intentSender; } void ConfigManager::RemoveConfigReceiver(const ConfigKey& key) { + lock_guard lock(mMutex); mConfigReceivers.erase(key); } void ConfigManager::RemoveConfig(const ConfigKey& key) { - auto it = mConfigs.find(key); - if (it != mConfigs.end()) { - // Remove from map - mConfigs.erase(it); + vector> broadcastList; + { + lock_guard lock(mMutex); + + auto it = mConfigs.find(key); + if (it != mConfigs.end()) { + // Remove from map + mConfigs.erase(it); + + for (sp listener : mListeners) { + broadcastList.push_back(listener); + } + } - // Tell everyone - for (auto& listener : mListeners) { - listener->OnConfigRemoved(key); + auto itReceiver = mConfigReceivers.find(key); + if (itReceiver != mConfigReceivers.end()) { + // Remove from map + mConfigReceivers.erase(itReceiver); } + + // Remove from disk. There can still be a lingering file on disk so we check + // whether or not the config was on memory. + remove_saved_configs(key); } - // Remove from disk. There can still be a lingering file on disk so we check - // whether or not the config was on memory. - remove_saved_configs(key); + for (sp listener:broadcastList) { + listener->OnConfigRemoved(key); + } } void ConfigManager::remove_saved_configs(const ConfigKey& key) { @@ -107,23 +133,32 @@ void ConfigManager::remove_saved_configs(const ConfigKey& key) { void ConfigManager::RemoveConfigs(int uid) { vector removed; + vector> broadcastList; + { + lock_guard lock(mMutex); + + + for (auto it = mConfigs.begin(); it != mConfigs.end();) { + // Remove from map + if (it->GetUid() == uid) { + remove_saved_configs(*it); + removed.push_back(*it); + mConfigReceivers.erase(*it); + it = mConfigs.erase(it); + } else { + it++; + } + } - for (auto it = mConfigs.begin(); it != mConfigs.end();) { - // Remove from map - if (it->GetUid() == uid) { - remove_saved_configs(*it); - removed.push_back(*it); - mConfigReceivers.erase(*it); - it = mConfigs.erase(it); - } else { - it++; + for (sp listener : mListeners) { + broadcastList.push_back(listener); } } // Remove separately so if they do anything in the callback they can't mess up our iteration. for (auto& key : removed) { // Tell everyone - for (auto& listener : mListeners) { + for (sp listener:broadcastList) { listener->OnConfigRemoved(key); } } @@ -131,27 +166,38 @@ void ConfigManager::RemoveConfigs(int uid) { void ConfigManager::RemoveAllConfigs() { vector removed; + vector> broadcastList; + { + lock_guard lock(mMutex); - for (auto it = mConfigs.begin(); it != mConfigs.end();) { - // Remove from map - removed.push_back(*it); - auto receiverIt = mConfigReceivers.find(*it); - if (receiverIt != mConfigReceivers.end()) { - mConfigReceivers.erase(*it); + + for (auto it = mConfigs.begin(); it != mConfigs.end();) { + // Remove from map + removed.push_back(*it); + auto receiverIt = mConfigReceivers.find(*it); + if (receiverIt != mConfigReceivers.end()) { + mConfigReceivers.erase(*it); + } + it = mConfigs.erase(it); + } + + for (sp listener : mListeners) { + broadcastList.push_back(listener); } - it = mConfigs.erase(it); } // Remove separately so if they do anything in the callback they can't mess up our iteration. for (auto& key : removed) { // Tell everyone - for (auto& listener : mListeners) { + for (sp listener:broadcastList) { listener->OnConfigRemoved(key); } } } vector ConfigManager::GetAllConfigKeys() const { + lock_guard lock(mMutex); + vector ret; for (auto it = mConfigs.cbegin(); it != mConfigs.cend(); ++it) { ret.push_back(*it); @@ -160,6 +206,8 @@ vector ConfigManager::GetAllConfigKeys() const { } const sp ConfigManager::GetConfigReceiver(const ConfigKey& key) const { + lock_guard lock(mMutex); + auto it = mConfigReceivers.find(key); if (it == mConfigReceivers.end()) { return nullptr; @@ -169,6 +217,8 @@ const sp ConfigManager::GetConfigReceiver(const ConfigKey& key } void ConfigManager::Dump(FILE* out) { + lock_guard lock(mMutex); + fprintf(out, "CONFIGURATIONS (%d)\n", (int)mConfigs.size()); fprintf(out, " uid name\n"); for (const auto& key : mConfigs) { @@ -180,7 +230,7 @@ void ConfigManager::Dump(FILE* out) { } } -void ConfigManager::update_saved_configs(const ConfigKey& key, const StatsdConfig& config) { +void ConfigManager::update_saved_configs_locked(const ConfigKey& key, const StatsdConfig& config) { // If there is a pre-existing config with same key we should first delete it. remove_saved_configs(key); diff --git a/cmds/statsd/src/config/ConfigManager.h b/cmds/statsd/src/config/ConfigManager.h index a2b2a0ce43d5..a0c1c1cb16f8 100644 --- a/cmds/statsd/src/config/ConfigManager.h +++ b/cmds/statsd/src/config/ConfigManager.h @@ -21,6 +21,7 @@ #include "config/ConfigListener.h" #include +#include #include #include @@ -109,10 +110,12 @@ public: void Dump(FILE* out); private: + mutable std::mutex mMutex; + /** * Save the configs to disk. */ - void update_saved_configs(const ConfigKey& key, const StatsdConfig& config); + void update_saved_configs_locked(const ConfigKey& key, const StatsdConfig& config); /** * Remove saved configs from disk. -- cgit v1.2.3-59-g8ed1b