MediaMetrics: Add thread-safety checking
Test: atest mediametrics_tests
Bug: 70398235
Bug: 149850236
Change-Id: I288ef92e6444785e02043c2629355c229c14e85c
diff --git a/media/libmediametrics/Android.bp b/media/libmediametrics/Android.bp
index 0cd5194..03068c7 100644
--- a/media/libmediametrics/Android.bp
+++ b/media/libmediametrics/Android.bp
@@ -22,9 +22,11 @@
export_include_dirs: ["include"],
cflags: [
- "-Werror",
- "-Wno-error=deprecated-declarations",
"-Wall",
+ "-Werror",
+ "-Wextra",
+ "-Wthread-safety",
+ "-Wunreachable-code",
],
sanitize: {
diff --git a/services/mediametrics/AnalyticsActions.h b/services/mediametrics/AnalyticsActions.h
index 5568c91..0151134 100644
--- a/services/mediametrics/AnalyticsActions.h
+++ b/services/mediametrics/AnalyticsActions.h
@@ -16,6 +16,7 @@
#pragma once
+#include <android-base/thread_annotations.h>
#include <media/MediaMetricsItem.h>
#include <mutex>
@@ -144,7 +145,7 @@
}
mutable std::mutex mLock;
- std::map<Trigger, Action> mFilters; // GUARDED_BY mLock
+ std::map<Trigger, Action> mFilters GUARDED_BY(mLock);
};
} // namespace android::mediametrics
diff --git a/services/mediametrics/Android.bp b/services/mediametrics/Android.bp
index 58f3ea8..fb4022e 100644
--- a/services/mediametrics/Android.bp
+++ b/services/mediametrics/Android.bp
@@ -27,6 +27,7 @@
"-Wall",
"-Werror",
"-Wextra",
+ "-Wthread-safety",
],
}
diff --git a/services/mediametrics/MediaMetricsService.cpp b/services/mediametrics/MediaMetricsService.cpp
index b9703ba..4b84bea 100644
--- a/services/mediametrics/MediaMetricsService.cpp
+++ b/services/mediametrics/MediaMetricsService.cpp
@@ -282,8 +282,8 @@
} else {
result.appendFormat("Dump of the %s process:\n", kServiceName);
const char *prefixptr = prefix.size() > 0 ? prefix.c_str() : nullptr;
- dumpHeaders_l(result, sinceNs, prefixptr);
- dumpQueue_l(result, sinceNs, prefixptr);
+ dumpHeaders(result, sinceNs, prefixptr);
+ dumpQueue(result, sinceNs, prefixptr);
// TODO: maybe consider a better way of dumping audio analytics info.
const int32_t linesToDump = all ? INT32_MAX : 1000;
@@ -312,7 +312,7 @@
}
// dump headers
-void MediaMetricsService::dumpHeaders_l(String8 &result, int64_t sinceNs, const char* prefix)
+void MediaMetricsService::dumpHeaders(String8 &result, int64_t sinceNs, const char* prefix)
{
if (mediametrics::Item::isEnabled()) {
result.append("Metrics gathering: enabled\n");
@@ -337,7 +337,7 @@
}
// TODO: should prefix be a set<string>?
-void MediaMetricsService::dumpQueue_l(String8 &result, int64_t sinceNs, const char* prefix)
+void MediaMetricsService::dumpQueue(String8 &result, int64_t sinceNs, const char* prefix)
{
if (mItems.empty()) {
result.append("empty\n");
@@ -364,7 +364,7 @@
// if item != NULL, it's the item we just inserted
// true == more items eligible to be recovered
-bool MediaMetricsService::expirations_l(const std::shared_ptr<const mediametrics::Item>& item)
+bool MediaMetricsService::expirations(const std::shared_ptr<const mediametrics::Item>& item)
{
bool more = false;
@@ -419,7 +419,7 @@
do {
sleep(1);
std::lock_guard _l(mLock);
- more = expirations_l(nullptr);
+ more = expirations(nullptr);
} while (more);
}
@@ -429,7 +429,7 @@
// we assume the items are roughly in time order.
mItems.emplace_back(item);
++mItemsFinalized;
- if (expirations_l(item)
+ if (expirations(item)
&& (!mExpireFuture.valid()
|| mExpireFuture.wait_for(std::chrono::seconds(0)) == std::future_status::ready)) {
mExpireFuture = std::async(std::launch::async, [this] { processExpirations(); });
diff --git a/services/mediametrics/MediaMetricsService.h b/services/mediametrics/MediaMetricsService.h
index a93f7fb..faba197 100644
--- a/services/mediametrics/MediaMetricsService.h
+++ b/services/mediametrics/MediaMetricsService.h
@@ -23,6 +23,7 @@
#include <unordered_map>
// IMediaMetricsService must include Vector, String16, Errors
+#include <android-base/thread_annotations.h>
#include <media/IMediaMetricsService.h>
#include <mediautils/ServiceUtilities.h>
#include <utils/String8.h>
@@ -81,12 +82,11 @@
bool isRateLimited(mediametrics::Item *) const;
void saveItem(const std::shared_ptr<const mediametrics::Item>& item);
- // The following methods are GUARDED_BY(mLock)
- bool expirations_l(const std::shared_ptr<const mediametrics::Item>& item);
+ bool expirations(const std::shared_ptr<const mediametrics::Item>& item) REQUIRES(mLock);
// support for generating output
- void dumpQueue_l(String8 &result, int64_t sinceNs, const char* prefix);
- void dumpHeaders_l(String8 &result, int64_t sinceNs, const char* prefix);
+ void dumpQueue(String8 &result, int64_t sinceNs, const char* prefix) REQUIRES(mLock);
+ void dumpHeaders(String8 &result, int64_t sinceNs, const char* prefix) REQUIRES(mLock);
// The following variables accessed without mLock
@@ -102,22 +102,22 @@
mediautils::UidInfo mUidInfo; // mUidInfo can be accessed without lock (locked internally)
- mediametrics::AudioAnalytics mAudioAnalytics;
+ mediametrics::AudioAnalytics mAudioAnalytics; // mAudioAnalytics is locked internally.
std::mutex mLock;
// statistics about our analytics
- int64_t mItemsFinalized = 0; // GUARDED_BY(mLock)
- int64_t mItemsDiscarded = 0; // GUARDED_BY(mLock)
- int64_t mItemsDiscardedExpire = 0; // GUARDED_BY(mLock)
- int64_t mItemsDiscardedCount = 0; // GUARDED_BY(mLock)
+ int64_t mItemsFinalized GUARDED_BY(mLock) = 0;
+ int64_t mItemsDiscarded GUARDED_BY(mLock) = 0;
+ int64_t mItemsDiscardedExpire GUARDED_BY(mLock) = 0;
+ int64_t mItemsDiscardedCount GUARDED_BY(mLock) = 0;
// If we have a worker thread to garbage collect
- std::future<void> mExpireFuture; // GUARDED_BY(mLock)
+ std::future<void> mExpireFuture GUARDED_BY(mLock);
// Our item queue, generally (oldest at front)
// TODO: Make separate class, use segmented queue, write lock only end.
// Note: Another analytics module might have ownership of an item longer than the log.
- std::deque<std::shared_ptr<const mediametrics::Item>> mItems; // GUARDED_BY(mLock)
+ std::deque<std::shared_ptr<const mediametrics::Item>> mItems GUARDED_BY(mLock);
};
} // namespace android
diff --git a/services/mediametrics/TimeMachine.h b/services/mediametrics/TimeMachine.h
index 29adeae..b138233 100644
--- a/services/mediametrics/TimeMachine.h
+++ b/services/mediametrics/TimeMachine.h
@@ -23,6 +23,7 @@
#include <variant>
#include <vector>
+#include <android-base/thread_annotations.h>
#include <media/MediaMetricsItem.h>
#include <utils/Timers.h>
@@ -92,7 +93,8 @@
}
template <typename T>
- status_t getValue(const std::string &property, T* value, int64_t time = 0) const {
+ status_t getValue(const std::string &property, T* value, int64_t time = 0) const
+ REQUIRES(mPseudoKeyHistoryLock) {
if (time == 0) time = systemTime(SYSTEM_TIME_REALTIME);
const auto tsptr = mPropertyMap.find(property);
if (tsptr == mPropertyMap.end()) return BAD_VALUE;
@@ -108,20 +110,22 @@
}
template <typename T>
- status_t getValue(const std::string &property, T defaultValue, int64_t time = 0) const {
+ status_t getValue(const std::string &property, T defaultValue, int64_t time = 0) const
+ REQUIRES(mPseudoKeyHistoryLock){
T value;
return getValue(property, &value, time) != NO_ERROR ? defaultValue : value;
}
void putProp(
- const std::string &name, const mediametrics::Item::Prop &prop, int64_t time = 0) {
+ const std::string &name, const mediametrics::Item::Prop &prop, int64_t time = 0)
+ REQUIRES(mPseudoKeyHistoryLock) {
//alternatively: prop.visit([&](auto value) { putValue(name, value, time); });
putValue(name, prop.get(), time);
}
template <typename T>
- void putValue(const std::string &property,
- T&& e, int64_t time = 0) {
+ void putValue(const std::string &property, T&& e, int64_t time = 0)
+ REQUIRES(mPseudoKeyHistoryLock) {
if (time == 0) time = systemTime(SYSTEM_TIME_REALTIME);
mLastModificationTime = time;
if (mPropertyMap.size() >= kKeyMaxProperties &&
@@ -144,7 +148,8 @@
}
}
- std::pair<std::string, int32_t> dump(int32_t lines, int64_t time) const {
+ std::pair<std::string, int32_t> dump(int32_t lines, int64_t time) const
+ REQUIRES(mPseudoKeyHistoryLock) {
std::stringstream ss;
int32_t ll = lines;
for (auto& tsPair : mPropertyMap) {
@@ -158,7 +163,9 @@
return { ss.str(), lines - ll };
}
- int64_t getLastModificationTime() const { return mLastModificationTime; }
+ int64_t getLastModificationTime() const REQUIRES(mPseudoKeyHistoryLock) {
+ return mLastModificationTime;
+ }
private:
static std::string dump(
@@ -267,7 +274,7 @@
if (it == mHistory.end()) {
if (!isTrusted) return PERMISSION_DENIED;
- (void)gc_l(garbage);
+ (void)gc(garbage);
// no keylock needed here as we are sole owner
// until placed on mHistory.
@@ -435,7 +442,8 @@
private:
// Obtains the lock for a KeyHistory.
- std::mutex &getLockForKey(const std::string &key) const {
+ std::mutex &getLockForKey(const std::string &key) const
+ RETURN_CAPABILITY(mPseudoKeyHistoryLock) {
return mKeyLocks[std::hash<std::string>{}(key) % std::size(mKeyLocks)];
}
@@ -459,7 +467,6 @@
return it->second;
}
- // GUARDED_BY mLock
/**
* Garbage collects if the TimeMachine size exceeds the high water mark.
*
@@ -471,7 +478,7 @@
*
* \return true if garbage collection was done.
*/
- bool gc_l(std::vector<std::any>& garbage) {
+ bool gc(std::vector<std::any>& garbage) REQUIRES(mLock) {
// TODO: something better than this for garbage collection.
if (mHistory.size() < mKeyHighWaterMark) return false;
@@ -540,12 +547,17 @@
*/
mutable std::mutex mLock; // Lock for mHistory
- History mHistory; // GUARDED_BY mLock
+ History mHistory GUARDED_BY(mLock);
// KEY_LOCKS is the number of mutexes for keys.
// It need not be a power of 2, but faster that way.
static inline constexpr size_t KEY_LOCKS = 256;
mutable std::mutex mKeyLocks[KEY_LOCKS]; // Hash-striped lock for KeyHistory based on key.
+
+ // Used for thread-safety analysis, we create a fake mutex object to represent
+ // the hash stripe lock mechanism, which is then tracked by the compiler.
+ class CAPABILITY("mutex") PseudoLock {};
+ static inline PseudoLock mPseudoKeyHistoryLock;
};
} // namespace android::mediametrics
diff --git a/services/mediametrics/TransactionLog.h b/services/mediametrics/TransactionLog.h
index 4f09bb0..d64acf3 100644
--- a/services/mediametrics/TransactionLog.h
+++ b/services/mediametrics/TransactionLog.h
@@ -21,6 +21,7 @@
#include <sstream>
#include <string>
+#include <android-base/thread_annotations.h>
#include <media/MediaMetricsItem.h>
namespace android::mediametrics {
@@ -92,7 +93,7 @@
std::vector<std::any> garbage; // objects destroyed after lock.
std::lock_guard lock(mLock);
- (void)gc_l(garbage);
+ (void)gc(garbage);
mLog.emplace(time, item);
mItemMap[key].emplace(time, item);
return NO_ERROR; // no errors for now.
@@ -104,7 +105,7 @@
std::vector<std::shared_ptr<const mediametrics::Item>> get(
int64_t startTime = 0, int64_t endTime = INT64_MAX) const {
std::lock_guard lock(mLock);
- return getItemsInRange_l(mLog, startTime, endTime);
+ return getItemsInRange(mLog, startTime, endTime);
}
/**
@@ -116,7 +117,7 @@
std::lock_guard lock(mLock);
auto mapIt = mItemMap.find(key);
if (mapIt == mItemMap.end()) return {};
- return getItemsInRange_l(mapIt->second, startTime, endTime);
+ return getItemsInRange(mapIt->second, startTime, endTime);
}
/**
@@ -204,7 +205,6 @@
return { ss.str(), lines - ll };
}
- // GUARDED_BY mLock
/**
* Garbage collects if the TimeMachine size exceeds the high water mark.
*
@@ -213,7 +213,7 @@
*
* \return true if garbage collection was done.
*/
- bool gc_l(std::vector<std::any>& garbage) {
+ bool gc(std::vector<std::any>& garbage) REQUIRES(mLock) {
if (mLog.size() < mHighWaterMark) return false;
ALOGD("%s: garbage collection", __func__);
@@ -268,7 +268,7 @@
return true;
}
- static std::vector<std::shared_ptr<const mediametrics::Item>> getItemsInRange_l(
+ static std::vector<std::shared_ptr<const mediametrics::Item>> getItemsInRange(
const MapTimeItem& map,
int64_t startTime = 0, int64_t endTime = INT64_MAX) {
auto it = map.lower_bound(startTime);
@@ -289,11 +289,8 @@
mutable std::mutex mLock;
- // GUARDED_BY mLock
- MapTimeItem mLog;
-
- // GUARDED_BY mLock
- std::map<std::string /* item_key */, MapTimeItem> mItemMap;
+ MapTimeItem mLog GUARDED_BY(mLock);
+ std::map<std::string /* item_key */, MapTimeItem> mItemMap GUARDED_BY(mLock);
};
} // namespace android::mediametrics