From 3eb9cede0b3f488736d3c62f10b70df0a5f1d4dc Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Mon, 20 Apr 2020 22:04:38 -0700 Subject: [PATCH] Fix PullUidProvider unregistering on config update Previously, MetricsManagers would unregister themselves as a PullUidProvider for a given ConfigKey in the destructor. This caused all pulls to fail after a config update because the new MetricsManager would register itself before the old MetricsManager was destructed and unregistered. This resulted in the old MetricsManager removing the new config since they shared the same config key. The fix is for the PullerManager to check that the PullUidProviders are equal in the unregister function before actually erasing it. Test: bit statsd_test:* (wrote a failing test that now passes). Test: statsd_localdrive to manually update a config, ensured pulls still worked. Bug: 154544328 Change-Id: Id7af3b3b407e24bee74fc34bd1c2b9e0575e9c9e --- .../src/external/StatsPullerManager.cpp | 8 +++++-- cmds/statsd/src/external/StatsPullerManager.h | 7 ++++-- cmds/statsd/src/metrics/MetricsManager.cpp | 2 +- cmds/statsd/tests/MetricsManager_test.cpp | 2 +- cmds/statsd/tests/StatsLogProcessor_test.cpp | 23 +++++++++++++++++++ .../tests/metrics/metrics_test_helper.h | 3 ++- 6 files changed, 38 insertions(+), 7 deletions(-) diff --git a/cmds/statsd/src/external/StatsPullerManager.cpp b/cmds/statsd/src/external/StatsPullerManager.cpp index ebe9610143361..cfd5d14b0d3be 100644 --- a/cmds/statsd/src/external/StatsPullerManager.cpp +++ b/cmds/statsd/src/external/StatsPullerManager.cpp @@ -252,9 +252,13 @@ void StatsPullerManager::RegisterPullUidProvider(const ConfigKey& configKey, mPullUidProviders[configKey] = provider; } -void StatsPullerManager::UnregisterPullUidProvider(const ConfigKey& configKey) { +void StatsPullerManager::UnregisterPullUidProvider(const ConfigKey& configKey, + wp provider) { std::lock_guard _l(mLock); - mPullUidProviders.erase(configKey); + const auto& it = mPullUidProviders.find(configKey); + if (it != mPullUidProviders.end() && it->second == provider) { + mPullUidProviders.erase(it); + } } void StatsPullerManager::OnAlarmFired(int64_t elapsedTimeNs) { diff --git a/cmds/statsd/src/external/StatsPullerManager.h b/cmds/statsd/src/external/StatsPullerManager.h index ab0cceeb112e9..5e18aaa6ed610 100644 --- a/cmds/statsd/src/external/StatsPullerManager.h +++ b/cmds/statsd/src/external/StatsPullerManager.h @@ -78,11 +78,12 @@ public: wp receiver); // Registers a pull uid provider for the config key. When pulling atoms, it will be used to - // determine which atoms to pull from. + // determine which uids to pull from. virtual void RegisterPullUidProvider(const ConfigKey& configKey, wp provider); // Unregister a pull uid provider. - virtual void UnregisterPullUidProvider(const ConfigKey& configKey); + virtual void UnregisterPullUidProvider(const ConfigKey& configKey, + wp provider); // Verify if we know how to pull for this matcher bool PullerForMatcherExists(int tagId) const; @@ -180,6 +181,8 @@ private: FRIEND_TEST(ValueMetricE2eTest, TestPulledEvents); FRIEND_TEST(ValueMetricE2eTest, TestPulledEvents_LateAlarm); FRIEND_TEST(ValueMetricE2eTest, TestPulledEvents_WithActivation); + + FRIEND_TEST(StatsLogProcessorTest, TestPullUidProviderSetOnConfigUpdate); }; } // namespace statsd diff --git a/cmds/statsd/src/metrics/MetricsManager.cpp b/cmds/statsd/src/metrics/MetricsManager.cpp index bcca1fd9fdc98..e03de0b991cd9 100644 --- a/cmds/statsd/src/metrics/MetricsManager.cpp +++ b/cmds/statsd/src/metrics/MetricsManager.cpp @@ -189,7 +189,7 @@ MetricsManager::~MetricsManager() { StateManager::getInstance().unregisterListener(atomId, it); } } - mPullerManager->UnregisterPullUidProvider(mConfigKey); + mPullerManager->UnregisterPullUidProvider(mConfigKey, this); VLOG("~MetricsManager()"); } diff --git a/cmds/statsd/tests/MetricsManager_test.cpp b/cmds/statsd/tests/MetricsManager_test.cpp index 1075fe4f9ce1f..a44541dddf538 100644 --- a/cmds/statsd/tests/MetricsManager_test.cpp +++ b/cmds/statsd/tests/MetricsManager_test.cpp @@ -528,7 +528,7 @@ TEST(MetricsManagerTest, TestLogSources) { })); sp pullerManager = new StrictMock(); EXPECT_CALL(*pullerManager, RegisterPullUidProvider(kConfigKey, _)).Times(1); - EXPECT_CALL(*pullerManager, UnregisterPullUidProvider(kConfigKey)).Times(1); + EXPECT_CALL(*pullerManager, UnregisterPullUidProvider(kConfigKey, _)).Times(1); sp anomalyAlarmMonitor; sp periodicAlarmMonitor; diff --git a/cmds/statsd/tests/StatsLogProcessor_test.cpp b/cmds/statsd/tests/StatsLogProcessor_test.cpp index d29394b1c5a6d..b809286da5f4e 100644 --- a/cmds/statsd/tests/StatsLogProcessor_test.cpp +++ b/cmds/statsd/tests/StatsLogProcessor_test.cpp @@ -301,6 +301,29 @@ TEST(StatsLogProcessorTest, TestOnDumpReportEraseData) { EXPECT_TRUE(noData); } +TEST(StatsLogProcessorTest, TestPullUidProviderSetOnConfigUpdate) { + // Setup simple config key corresponding to empty config. + sp m = new UidMap(); + sp pullerManager = new StatsPullerManager(); + sp anomalyAlarmMonitor; + sp subscriberAlarmMonitor; + StatsLogProcessor p( + m, pullerManager, anomalyAlarmMonitor, subscriberAlarmMonitor, 0, + [](const ConfigKey& key) { return true; }, + [](const int&, const vector&) { return true; }); + ConfigKey key(3, 4); + StatsdConfig config = MakeConfig(false); + p.OnConfigUpdated(0, key, config); + EXPECT_NE(pullerManager->mPullUidProviders.find(key), pullerManager->mPullUidProviders.end()); + + config.add_default_pull_packages("AID_STATSD"); + p.OnConfigUpdated(5, key, config); + EXPECT_NE(pullerManager->mPullUidProviders.find(key), pullerManager->mPullUidProviders.end()); + + p.OnConfigRemoved(key); + EXPECT_EQ(pullerManager->mPullUidProviders.find(key), pullerManager->mPullUidProviders.end()); +} + TEST(StatsLogProcessorTest, TestActiveConfigMetricDiskWriteRead) { int uid = 1111; diff --git a/cmds/statsd/tests/metrics/metrics_test_helper.h b/cmds/statsd/tests/metrics/metrics_test_helper.h index 69f7e3f497923..be410b10d43b2 100644 --- a/cmds/statsd/tests/metrics/metrics_test_helper.h +++ b/cmds/statsd/tests/metrics/metrics_test_helper.h @@ -44,7 +44,8 @@ public: vector>* data, bool useUids)); MOCK_METHOD2(RegisterPullUidProvider, void(const ConfigKey& configKey, wp provider)); - MOCK_METHOD1(UnregisterPullUidProvider, void(const ConfigKey& configKey)); + MOCK_METHOD2(UnregisterPullUidProvider, + void(const ConfigKey& configKey, wp provider)); }; class MockUidMap : public UidMap {