Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3eb9cede authored by Tej Singh's avatar Tej Singh
Browse files

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
parent 87adee87

cmds/statsd/src/external/StatsPullerManager.cpp

+6 −2
Original line number Diff line number Diff line
@@ -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<PullUidProvider> provider) {

    std::lock_guard<std::mutex> _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) {

cmds/statsd/src/external/StatsPullerManager.h

+5 −2
Original line number Diff line number Diff line
@@ -78,11 +78,12 @@ public: 
                                    wp<PullDataReceiver> 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<PullUidProvider> provider);



    // Unregister a pull uid provider.

    virtual void UnregisterPullUidProvider(const ConfigKey& configKey);

    virtual void UnregisterPullUidProvider(const ConfigKey& configKey,

                                           wp<PullUidProvider> 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

cmds/statsd/src/metrics/MetricsManager.cpp

+1 −1
Original line number Diff line number Diff line
@@ -189,7 +189,7 @@ MetricsManager::~MetricsManager() { 
            StateManager::getInstance().unregisterListener(atomId, it);

        }

    }

    mPullerManager->UnregisterPullUidProvider(mConfigKey);

    mPullerManager->UnregisterPullUidProvider(mConfigKey, this);



    VLOG("~MetricsManager()");

}

cmds/statsd/tests/MetricsManager_test.cpp

+1 −1
Original line number Diff line number Diff line
@@ -528,7 +528,7 @@ TEST(MetricsManagerTest, TestLogSources) { 
            }));

    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();

    EXPECT_CALL(*pullerManager, RegisterPullUidProvider(kConfigKey, _)).Times(1);

    EXPECT_CALL(*pullerManager, UnregisterPullUidProvider(kConfigKey)).Times(1);

    EXPECT_CALL(*pullerManager, UnregisterPullUidProvider(kConfigKey, _)).Times(1);



    sp<AlarmMonitor> anomalyAlarmMonitor;

    sp<AlarmMonitor> periodicAlarmMonitor;

cmds/statsd/tests/StatsLogProcessor_test.cpp

+23 −0
Original line number Diff line number Diff line
@@ -301,6 +301,29 @@ TEST(StatsLogProcessorTest, TestOnDumpReportEraseData) { 
    EXPECT_TRUE(noData);

}



TEST(StatsLogProcessorTest, TestPullUidProviderSetOnConfigUpdate) {

    // Setup simple config key corresponding to empty config.

    sp<UidMap> m = new UidMap();

    sp<StatsPullerManager> pullerManager = new StatsPullerManager();

    sp<AlarmMonitor> anomalyAlarmMonitor;

    sp<AlarmMonitor> subscriberAlarmMonitor;

    StatsLogProcessor p(

            m, pullerManager, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,

            [](const ConfigKey& key) { return true; },

            [](const int&, const vector<int64_t>&) { 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;