From 518d77d208f97e7f0834969fa53a1b49d4eb87fb Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Mon, 13 Jan 2020 17:56:58 -0800 Subject: [PATCH] Register pullers in separate thread This cl modifies getStatsService to use the blocking getService, but also makes the binder call in a separate thread to not block the client thread. This is needed because it is possible for pullers to be registered before statsd starts, and calling checkService before statsd is up will fail. We also would never receive the binderDied to register it, because we would never have a binder object to linkToDeath on. Bug: 147682855 Test: atest LibStatsPullTests Change-Id: I68c04bc24c7fe066eca88cab4f6a76885581c1ee --- libstats/pull/stats_pull_atom_callback.cpp | 78 ++++++++++++++-------- 1 file changed, 51 insertions(+), 27 deletions(-) diff --git a/libstats/pull/stats_pull_atom_callback.cpp b/libstats/pull/stats_pull_atom_callback.cpp index 99233c316..e8fc2ea32 100644 --- a/libstats/pull/stats_pull_atom_callback.cpp +++ b/libstats/pull/stats_pull_atom_callback.cpp @@ -25,7 +25,8 @@ #include #include #include -#include "include/stats_pull_atom_callback.h" + +#include struct pulled_stats_event_list { std::vector data; @@ -94,7 +95,7 @@ static std::mutex pullAtomMutex; static android::sp sStatsd = nullptr; static std::map> mPullers; -static android::sp getStatsServiceLocked(); +static android::sp getStatsService(); class StatsDeathRecipient : public android::IBinder::DeathRecipient { public: @@ -103,15 +104,21 @@ class StatsDeathRecipient : public android::IBinder::DeathRecipient { // android::IBinder::DeathRecipient override: void binderDied(const android::wp& /* who */) override { - std::lock_guard lock(pullAtomMutex); - if (sStatsd) { + { + std::lock_guard lock(pullAtomMutex); sStatsd = nullptr; } - android::sp statsService = getStatsServiceLocked(); + android::sp statsService = getStatsService(); if (statsService == nullptr) { return; } - for (auto it : mPullers) { + + std::map> pullersCopy; + { + std::lock_guard lock(pullAtomMutex); + pullersCopy = mPullers; + } + for (auto it : pullersCopy) { statsService->registerNativePullAtomCallback(it.first, it.second->getCoolDownNs(), it.second->getTimeoutNs(), it.second->getAdditiveFields(), it.second); @@ -121,11 +128,12 @@ class StatsDeathRecipient : public android::IBinder::DeathRecipient { static android::sp statsDeathRecipient = new StatsDeathRecipient(); -static android::sp getStatsServiceLocked() { +static android::sp getStatsService() { + std::lock_guard lock(pullAtomMutex); if (!sStatsd) { // Fetch statsd. const android::sp binder = - android::defaultServiceManager()->checkService(android::String16("stats")); + android::defaultServiceManager()->getService(android::String16("stats")); if (!binder) { return nullptr; } @@ -135,6 +143,28 @@ static android::sp getStatsServiceLocked() { return sStatsd; } +void registerStatsPullAtomCallbackBlocking(int32_t atomTag, + android::sp cb) { + const android::sp statsService = getStatsService(); + if (statsService == nullptr) { + // Statsd not available + return; + } + + statsService->registerNativePullAtomCallback(atomTag, cb->getCoolDownNs(), cb->getTimeoutNs(), + cb->getAdditiveFields(), cb); +} + +void unregisterStatsPullAtomCallbackBlocking(int32_t atomTag) { + const android::sp statsService = getStatsService(); + if (statsService == nullptr) { + // Statsd not available + return; + } + + statsService->unregisterNativePullAtomCallback(atomTag); +} + void register_stats_pull_atom_callback(int32_t atom_tag, stats_pull_atom_callback_t callback, pull_atom_metadata* metadata, void* cookie) { int64_t coolDownNs = metadata == nullptr ? DEFAULT_COOL_DOWN_NS : metadata->cool_down_ns; @@ -146,32 +176,26 @@ void register_stats_pull_atom_callback(int32_t atom_tag, stats_pull_atom_callbac metadata->additive_fields + metadata->additive_fields_size); } - std::lock_guard lg(pullAtomMutex); - - // Always add to the map. If statsd is dead, we will add them when it comes back. android::sp callbackBinder = new StatsPullAtomCallbackInternal( callback, cookie, coolDownNs, timeoutNs, additiveFields); - mPullers[atom_tag] = callbackBinder; - const android::sp statsService = getStatsServiceLocked(); - if (statsService == nullptr) { - // Statsd not available - return; + { + std::lock_guard lg(pullAtomMutex); + // Always add to the map. If statsd is dead, we will add them when it comes back. + mPullers[atom_tag] = callbackBinder; } - statsService->registerNativePullAtomCallback(atom_tag, coolDownNs, timeoutNs, additiveFields, - callbackBinder); + std::thread registerThread(registerStatsPullAtomCallbackBlocking, atom_tag, callbackBinder); + registerThread.detach(); } void unregister_stats_pull_atom_callback(int32_t atom_tag) { - std::lock_guard lg(pullAtomMutex); - // Always remove the puller from our map. - // If statsd is down, we will not register it when it comes back. - mPullers.erase(atom_tag); - const android::sp statsService = getStatsServiceLocked(); - if (statsService == nullptr) { - // Statsd not available - return; + { + std::lock_guard lg(pullAtomMutex); + // Always remove the puller from our map. + // If statsd is down, we will not register it when it comes back. + mPullers.erase(atom_tag); } - statsService->unregisterNativePullAtomCallback(atom_tag); + std::thread unregisterThread(unregisterStatsPullAtomCallbackBlocking, atom_tag); + unregisterThread.detach(); }