aaudio: add thread safety annotation
Also improve naming to clarify lock requirements.
And call some locked methods that were not called before.
Although they were protected with a different lock so
it should have no effect.
Bug: 171296283
Test: adb logcat *:F
Test: In another window:
Test: atest AAudioTestCases
Change-Id: I6e863cbdea9250188e3f4b8f8654ef71c8951e74
diff --git a/services/oboeservice/AAudioClientTracker.cpp b/services/oboeservice/AAudioClientTracker.cpp
index 3ec8dea..054a896 100644
--- a/services/oboeservice/AAudioClientTracker.cpp
+++ b/services/oboeservice/AAudioClientTracker.cpp
@@ -41,7 +41,7 @@
: Singleton<AAudioClientTracker>() {
}
-std::string AAudioClientTracker::dump() const {
+std::string AAudioClientTracker::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
[this]()->bool { return mLock.try_lock(); } /* f */,
@@ -207,7 +207,7 @@
}
-std::string AAudioClientTracker::NotificationClient::dump() const {
+std::string AAudioClientTracker::NotificationClient::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
[this]()->bool { return mLock.try_lock(); } /* f */,
diff --git a/services/oboeservice/AAudioClientTracker.h b/services/oboeservice/AAudioClientTracker.h
index facfc3b..2b38621 100644
--- a/services/oboeservice/AAudioClientTracker.h
+++ b/services/oboeservice/AAudioClientTracker.h
@@ -21,6 +21,7 @@
#include <mutex>
#include <set>
+#include <android-base/thread_annotations.h>
#include <utils/Singleton.h>
#include <aaudio/AAudio.h>
@@ -114,10 +115,12 @@
};
// This must be called under mLock
- android::sp<NotificationClient> getNotificationClient_l(pid_t pid);
+ android::sp<NotificationClient> getNotificationClient_l(pid_t pid)
+ REQUIRES(mLock);
mutable std::mutex mLock;
- std::map<pid_t, android::sp<NotificationClient>> mNotificationClients;
+ std::map<pid_t, android::sp<NotificationClient>> mNotificationClients
+ GUARDED_BY(mLock);
android::AAudioService *mAAudioService = nullptr;
};
diff --git a/services/oboeservice/AAudioEndpointManager.cpp b/services/oboeservice/AAudioEndpointManager.cpp
index 483a264..407f6d5 100644
--- a/services/oboeservice/AAudioEndpointManager.cpp
+++ b/services/oboeservice/AAudioEndpointManager.cpp
@@ -43,7 +43,7 @@
, mExclusiveStreams() {
}
-std::string AAudioEndpointManager::dump() const {
+std::string AAudioEndpointManager::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
int index = 0;
diff --git a/services/oboeservice/AAudioEndpointManager.h b/services/oboeservice/AAudioEndpointManager.h
index ae776b1..b07bcef 100644
--- a/services/oboeservice/AAudioEndpointManager.h
+++ b/services/oboeservice/AAudioEndpointManager.h
@@ -20,6 +20,8 @@
#include <map>
#include <mutex>
#include <sys/types.h>
+
+#include <android-base/thread_annotations.h>
#include <utils/Singleton.h>
#include "binding/AAudioServiceMessage.h"
@@ -70,10 +72,12 @@
const aaudio::AAudioStreamRequest &request);
android::sp<AAudioServiceEndpoint> findExclusiveEndpoint_l(
- const AAudioStreamConfiguration& configuration);
+ const AAudioStreamConfiguration& configuration)
+ REQUIRES(mExclusiveLock);
android::sp<AAudioServiceEndpointShared> findSharedEndpoint_l(
- const AAudioStreamConfiguration& configuration);
+ const AAudioStreamConfiguration& configuration)
+ REQUIRES(mSharedLock);
void closeExclusiveEndpoint(android::sp<AAudioServiceEndpoint> serviceEndpoint);
void closeSharedEndpoint(android::sp<AAudioServiceEndpoint> serviceEndpoint);
@@ -83,23 +87,25 @@
// Lock mSharedLock before mExclusiveLock.
// it is OK to only lock mExclusiveLock.
mutable std::mutex mSharedLock;
- std::vector<android::sp<AAudioServiceEndpointShared>> mSharedStreams;
+ std::vector<android::sp<AAudioServiceEndpointShared>> mSharedStreams
+ GUARDED_BY(mSharedLock);
mutable std::mutex mExclusiveLock;
- std::vector<android::sp<AAudioServiceEndpointMMAP>> mExclusiveStreams;
+ std::vector<android::sp<AAudioServiceEndpointMMAP>> mExclusiveStreams
+ GUARDED_BY(mExclusiveLock);
- // Modified under a lock.
- int32_t mExclusiveSearchCount = 0; // number of times we SEARCHED for an exclusive endpoint
- int32_t mExclusiveFoundCount = 0; // number of times we FOUND an exclusive endpoint
- int32_t mExclusiveOpenCount = 0; // number of times we OPENED an exclusive endpoint
- int32_t mExclusiveCloseCount = 0; // number of times we CLOSED an exclusive endpoint
- int32_t mExclusiveStolenCount = 0; // number of times we STOLE an exclusive endpoint
+ // Counts related to an exclusive endpoint.
+ int32_t mExclusiveSearchCount GUARDED_BY(mExclusiveLock) = 0; // # SEARCHED
+ int32_t mExclusiveFoundCount GUARDED_BY(mExclusiveLock) = 0; // # FOUND
+ int32_t mExclusiveOpenCount GUARDED_BY(mExclusiveLock) = 0; // # OPENED
+ int32_t mExclusiveCloseCount GUARDED_BY(mExclusiveLock) = 0; // # CLOSED
+ int32_t mExclusiveStolenCount GUARDED_BY(mExclusiveLock) = 0; // # STOLEN
// Same as above but for SHARED endpoints.
- int32_t mSharedSearchCount = 0;
- int32_t mSharedFoundCount = 0;
- int32_t mSharedOpenCount = 0;
- int32_t mSharedCloseCount = 0;
+ int32_t mSharedSearchCount GUARDED_BY(mSharedLock) = 0;
+ int32_t mSharedFoundCount GUARDED_BY(mSharedLock) = 0;
+ int32_t mSharedOpenCount GUARDED_BY(mSharedLock) = 0;
+ int32_t mSharedCloseCount GUARDED_BY(mSharedLock) = 0;
// For easily disabling the stealing of exclusive streams.
static constexpr bool kStealingEnabled = true;
diff --git a/services/oboeservice/AAudioServiceEndpoint.cpp b/services/oboeservice/AAudioServiceEndpoint.cpp
index b139be1..faea58f 100644
--- a/services/oboeservice/AAudioServiceEndpoint.cpp
+++ b/services/oboeservice/AAudioServiceEndpoint.cpp
@@ -38,7 +38,7 @@
using namespace android; // TODO just import names needed
using namespace aaudio; // TODO just import names needed
-std::string AAudioServiceEndpoint::dump() const {
+std::string AAudioServiceEndpoint::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
diff --git a/services/oboeservice/AAudioServiceEndpoint.h b/services/oboeservice/AAudioServiceEndpoint.h
index 04b906a..72090c2 100644
--- a/services/oboeservice/AAudioServiceEndpoint.h
+++ b/services/oboeservice/AAudioServiceEndpoint.h
@@ -22,6 +22,8 @@
#include <mutex>
#include <vector>
+#include <android-base/thread_annotations.h>
+
#include "client/AudioStreamInternal.h"
#include "client/AudioStreamInternalPlay.h"
#include "core/AAudioStreamParameters.h"
@@ -141,7 +143,8 @@
std::vector<android::sp<AAudioServiceStreamBase>> disconnectRegisteredStreams();
mutable std::mutex mLockStreams;
- std::vector<android::sp<AAudioServiceStreamBase>> mRegisteredStreams;
+ std::vector<android::sp<AAudioServiceStreamBase>> mRegisteredStreams
+ GUARDED_BY(mLockStreams);
SimpleDoubleBuffer<Timestamp> mAtomicEndpointTimestamp;
diff --git a/services/oboeservice/AAudioServiceEndpointPlay.cpp b/services/oboeservice/AAudioServiceEndpointPlay.cpp
index 6ddc30b..4e46033 100644
--- a/services/oboeservice/AAudioServiceEndpointPlay.cpp
+++ b/services/oboeservice/AAudioServiceEndpointPlay.cpp
@@ -98,7 +98,7 @@
{
// Lock the AudioFifo to protect against close.
- std::lock_guard <std::mutex> lock(streamShared->getAudioDataQueueLock());
+ std::lock_guard <std::mutex> lock(streamShared->audioDataQueueLock);
std::shared_ptr<SharedRingBuffer> audioDataQueue
= streamShared->getAudioDataQueue_l();
std::shared_ptr<FifoBuffer> fifo;
diff --git a/services/oboeservice/AAudioServiceEndpointShared.cpp b/services/oboeservice/AAudioServiceEndpointShared.cpp
index caf6139..501e8c0 100644
--- a/services/oboeservice/AAudioServiceEndpointShared.cpp
+++ b/services/oboeservice/AAudioServiceEndpointShared.cpp
@@ -137,24 +137,25 @@
aaudio_result_t aaudio::AAudioServiceEndpointShared::stopSharingThread() {
mCallbackEnabled.store(false);
- aaudio_result_t result = getStreamInternal()->joinThread(NULL);
- return result;
+ return getStreamInternal()->joinThread(NULL);
}
-aaudio_result_t AAudioServiceEndpointShared::startStream(sp<AAudioServiceStreamBase> sharedStream,
- audio_port_handle_t *clientHandle) {
+aaudio_result_t AAudioServiceEndpointShared::startStream(
+ sp<AAudioServiceStreamBase> sharedStream,
+ audio_port_handle_t *clientHandle)
+ NO_THREAD_SAFETY_ANALYSIS {
aaudio_result_t result = AAUDIO_OK;
{
std::lock_guard<std::mutex> lock(mLockStreams);
if (++mRunningStreamCount == 1) { // atomic
- result = getStreamInternal()->requestStart_l();
+ result = getStreamInternal()->systemStart();
if (result != AAUDIO_OK) {
--mRunningStreamCount;
} else {
result = startSharingThread_l();
if (result != AAUDIO_OK) {
- getStreamInternal()->requestStop_l();
+ getStreamInternal()->systemStopFromApp();
--mRunningStreamCount;
}
}
@@ -168,7 +169,7 @@
if (result != AAUDIO_OK) {
if (--mRunningStreamCount == 0) { // atomic
stopSharingThread();
- getStreamInternal()->requestStop_l();
+ getStreamInternal()->systemStopFromApp();
}
}
}
@@ -183,7 +184,7 @@
if (--mRunningStreamCount == 0) { // atomic
stopSharingThread(); // the sharing thread locks mLockStreams
- getStreamInternal()->requestStop_l();
+ getStreamInternal()->systemStopFromApp();
}
return AAUDIO_OK;
}
diff --git a/services/oboeservice/AAudioServiceEndpointShared.h b/services/oboeservice/AAudioServiceEndpointShared.h
index 91a86c1..8357567 100644
--- a/services/oboeservice/AAudioServiceEndpointShared.h
+++ b/services/oboeservice/AAudioServiceEndpointShared.h
@@ -20,6 +20,8 @@
#include <atomic>
#include <mutex>
+#include <android-base/thread_annotations.h>
+
#include "AAudioServiceEndpoint.h"
#include "client/AudioStreamInternal.h"
#include "client/AudioStreamInternalPlay.h"
@@ -63,7 +65,7 @@
protected:
- aaudio_result_t startSharingThread_l();
+ aaudio_result_t startSharingThread_l() REQUIRES(mLockStreams);
aaudio_result_t stopSharingThread();
diff --git a/services/oboeservice/AAudioServiceStreamBase.cpp b/services/oboeservice/AAudioServiceStreamBase.cpp
index 9736091..7edc25c 100644
--- a/services/oboeservice/AAudioServiceStreamBase.cpp
+++ b/services/oboeservice/AAudioServiceStreamBase.cpp
@@ -595,14 +595,3 @@
void AAudioServiceStreamBase::onVolumeChanged(float volume) {
sendServiceEvent(AAUDIO_SERVICE_EVENT_VOLUME, volume);
}
-
-int32_t AAudioServiceStreamBase::incrementServiceReferenceCount_l() {
- return ++mCallingCount;
-}
-
-int32_t AAudioServiceStreamBase::decrementServiceReferenceCount_l() {
- int32_t count = --mCallingCount;
- // Each call to increment should be balanced with one call to decrement.
- assert(count >= 0);
- return count;
-}
diff --git a/services/oboeservice/AAudioServiceStreamBase.h b/services/oboeservice/AAudioServiceStreamBase.h
index f9efc2a..0f752b7 100644
--- a/services/oboeservice/AAudioServiceStreamBase.h
+++ b/services/oboeservice/AAudioServiceStreamBase.h
@@ -20,6 +20,7 @@
#include <assert.h>
#include <mutex>
+#include <android-base/thread_annotations.h>
#include <media/AudioClient.h>
#include <utils/RefBase.h>
@@ -209,25 +210,6 @@
return mSuspended;
}
- /**
- * Atomically increment the number of active references to the stream by AAudioService.
- *
- * This is called under a global lock in AAudioStreamTracker.
- *
- * @return value after the increment
- */
- int32_t incrementServiceReferenceCount_l();
-
- /**
- * Atomically decrement the number of active references to the stream by AAudioService.
- * This should only be called after incrementServiceReferenceCount_l().
- *
- * This is called under a global lock in AAudioStreamTracker.
- *
- * @return value after the decrement
- */
- int32_t decrementServiceReferenceCount_l();
-
bool isCloseNeeded() const {
return mCloseNeeded.load();
}
@@ -250,11 +232,10 @@
aaudio_result_t open(const aaudio::AAudioStreamRequest &request,
aaudio_sharing_mode_t sharingMode);
- // These must be called under mLock
- virtual aaudio_result_t close_l();
- virtual aaudio_result_t pause_l();
- virtual aaudio_result_t stop_l();
- void disconnect_l();
+ virtual aaudio_result_t close_l() REQUIRES(mLock);
+ virtual aaudio_result_t pause_l() REQUIRES(mLock);
+ virtual aaudio_result_t stop_l() REQUIRES(mLock);
+ void disconnect_l() REQUIRES(mLock);
void setState(aaudio_stream_state_t state);
@@ -332,18 +313,17 @@
aaudio_handle_t mHandle = -1;
bool mFlowing = false;
- // This is modified under a global lock in AAudioStreamTracker.
- int32_t mCallingCount = 0;
-
- // This indicates that a stream that is being referenced by a binder call needs to closed.
- std::atomic<bool> mCloseNeeded{false};
+ // This indicates that a stream that is being referenced by a binder call
+ // and needs to closed.
+ std::atomic<bool> mCloseNeeded{false}; // TODO remove
// This indicate that a running stream should not be processed because of an error,
// for example a full message queue. Note that this atomic is unrelated to mCloseNeeded.
std::atomic<bool> mSuspended{false};
+protected:
// Locking order is important.
- // Always acquire mLock before acquiring AAudioServiceEndpoint::mLockStreams
+ // Acquire mLock before acquiring AAudioServiceEndpoint::mLockStreams
std::mutex mLock; // Prevent start/stop/close etcetera from colliding
};
diff --git a/services/oboeservice/AAudioServiceStreamMMAP.h b/services/oboeservice/AAudioServiceStreamMMAP.h
index 5902613..6ba1725 100644
--- a/services/oboeservice/AAudioServiceStreamMMAP.h
+++ b/services/oboeservice/AAudioServiceStreamMMAP.h
@@ -19,6 +19,7 @@
#include <atomic>
+#include <android-base/thread_annotations.h>
#include <android-base/unique_fd.h>
#include <media/audiohal/StreamHalInterface.h>
#include <media/MmapStreamCallback.h>
@@ -34,10 +35,8 @@
#include "TimestampScheduler.h"
#include "utility/MonotonicCounter.h"
-
namespace aaudio {
-
/**
* These corresponds to an EXCLUSIVE mode MMAP client stream.
* It has exclusive use of one AAudioServiceEndpointMMAP to communicate with the underlying
@@ -68,9 +67,9 @@
* This is not guaranteed to be synchronous but it currently is.
* An AAUDIO_SERVICE_EVENT_PAUSED will be sent to the client when complete.
*/
- aaudio_result_t pause_l() override;
+ aaudio_result_t pause_l() REQUIRES(mLock) override;
- aaudio_result_t stop_l() override;
+ aaudio_result_t stop_l() REQUIRES(mLock) override;
aaudio_result_t getAudioDataDescription(AudioEndpointParcelable &parcelable) override;
diff --git a/services/oboeservice/AAudioServiceStreamShared.cpp b/services/oboeservice/AAudioServiceStreamShared.cpp
index 031468e..c665cda 100644
--- a/services/oboeservice/AAudioServiceStreamShared.cpp
+++ b/services/oboeservice/AAudioServiceStreamShared.cpp
@@ -52,14 +52,26 @@
return result.str();
}
-std::string AAudioServiceStreamShared::dump() const {
+std::string AAudioServiceStreamShared::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
+ const bool isLocked = AAudio_tryUntilTrue(
+ [this]()->bool { return audioDataQueueLock.try_lock(); } /* f */,
+ 50 /* times */,
+ 20 /* sleepMs */);
+ if (!isLocked) {
+ result << "AAudioServiceStreamShared may be deadlocked\n";
+ }
+
result << AAudioServiceStreamBase::dump();
result << mAudioDataQueue->dump();
result << std::setw(8) << getXRunCount();
+ if (isLocked) {
+ audioDataQueueLock.unlock();
+ }
+
return result.str();
}
@@ -171,7 +183,7 @@
}
{
- std::lock_guard<std::mutex> lock(mAudioDataQueueLock);
+ std::lock_guard<std::mutex> lock(audioDataQueueLock);
// Create audio data shared memory buffer for client.
mAudioDataQueue = std::make_shared<SharedRingBuffer>();
result = mAudioDataQueue->allocate(calculateBytesPerFrame(), getBufferCapacity());
@@ -202,7 +214,7 @@
aaudio_result_t AAudioServiceStreamShared::getAudioDataDescription(
AudioEndpointParcelable &parcelable)
{
- std::lock_guard<std::mutex> lock(mAudioDataQueueLock);
+ std::lock_guard<std::mutex> lock(audioDataQueueLock);
if (mAudioDataQueue == nullptr) {
ALOGW("%s(): mUpMessageQueue null! - stream not open", __func__);
return AAUDIO_ERROR_NULL;
@@ -260,7 +272,7 @@
int64_t clientFramesWritten = 0;
// Lock the AudioFifo to protect against close.
- std::lock_guard <std::mutex> lock(mAudioDataQueueLock);
+ std::lock_guard <std::mutex> lock(audioDataQueueLock);
if (mAudioDataQueue != nullptr) {
std::shared_ptr<FifoBuffer> fifo = mAudioDataQueue->getFifoBuffer();
diff --git a/services/oboeservice/AAudioServiceStreamShared.h b/services/oboeservice/AAudioServiceStreamShared.h
index 5b1f8da..4fae5b4 100644
--- a/services/oboeservice/AAudioServiceStreamShared.h
+++ b/services/oboeservice/AAudioServiceStreamShared.h
@@ -52,22 +52,15 @@
aaudio_result_t open(const aaudio::AAudioStreamRequest &request) override;
- /**
- * This must be locked when calling getAudioDataQueue_l() and while
- * using the FifoBuffer it contains.
- */
- std::mutex &getAudioDataQueueLock() {
- return mAudioDataQueueLock;
- }
-
void writeDataIfRoom(int64_t mmapFramesRead, const void *buffer, int32_t numFrames);
/**
* This must only be called under getAudioDataQueueLock().
* @return
*/
- std::shared_ptr<SharedRingBuffer> getAudioDataQueue_l() {
- return mAudioDataQueue;
+ std::shared_ptr<SharedRingBuffer> getAudioDataQueue_l()
+ REQUIRES(audioDataQueueLock) {
+ return mAudioDataQueue;
}
/* Keep a record of when a buffer transfer completed.
@@ -89,6 +82,10 @@
const char *getTypeText() const override { return "Shared"; }
+ // This is public so that the thread safety annotation, GUARDED_BY(),
+ // Can work when another object takes the lock.
+ mutable std::mutex audioDataQueueLock;
+
protected:
aaudio_result_t getAudioDataDescription(AudioEndpointParcelable &parcelable) override;
@@ -107,8 +104,7 @@
private:
- std::shared_ptr<SharedRingBuffer> mAudioDataQueue; // protected by mAudioDataQueueLock
- std::mutex mAudioDataQueueLock;
+ std::shared_ptr<SharedRingBuffer> mAudioDataQueue GUARDED_BY(audioDataQueueLock);
std::atomic<int64_t> mTimestampPositionOffset;
std::atomic<int32_t> mXRunCount;
diff --git a/services/oboeservice/AAudioStreamTracker.cpp b/services/oboeservice/AAudioStreamTracker.cpp
index 8e66b94..9bbbc73 100644
--- a/services/oboeservice/AAudioStreamTracker.cpp
+++ b/services/oboeservice/AAudioStreamTracker.cpp
@@ -96,7 +96,7 @@
return handle;
}
-std::string AAudioStreamTracker::dump() const {
+std::string AAudioStreamTracker::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
[this]()->bool { return mHandleLock.try_lock(); } /* f */,
diff --git a/services/oboeservice/AAudioStreamTracker.h b/services/oboeservice/AAudioStreamTracker.h
index d1301a2..43870fc 100644
--- a/services/oboeservice/AAudioStreamTracker.h
+++ b/services/oboeservice/AAudioStreamTracker.h
@@ -17,13 +17,13 @@
#ifndef AAUDIO_AAUDIO_STREAM_TRACKER_H
#define AAUDIO_AAUDIO_STREAM_TRACKER_H
+#include <mutex>
#include <time.h>
-#include <pthread.h>
+#include <android-base/thread_annotations.h>
#include <aaudio/AAudio.h>
#include "binding/AAudioCommon.h"
-
#include "AAudioServiceStreamBase.h"
namespace aaudio {
@@ -75,11 +75,10 @@
static aaudio_handle_t bumpHandle(aaudio_handle_t handle);
// Track stream using a unique handle that wraps. Only use positive half.
- mutable std::mutex mHandleLock;
- // protected by mHandleLock
- aaudio_handle_t mPreviousHandle = 0;
- // protected by mHandleLock
- std::map<aaudio_handle_t, android::sp<aaudio::AAudioServiceStreamBase>> mStreamsByHandle;
+ mutable std::mutex mHandleLock;
+ aaudio_handle_t mPreviousHandle GUARDED_BY(mHandleLock) = 0;
+ std::map<aaudio_handle_t, android::sp<aaudio::AAudioServiceStreamBase>>
+ mStreamsByHandle GUARDED_BY(mHandleLock);
};
diff --git a/services/oboeservice/Android.bp b/services/oboeservice/Android.bp
index 31e590e..80f17f4 100644
--- a/services/oboeservice/Android.bp
+++ b/services/oboeservice/Android.bp
@@ -37,6 +37,7 @@
],
cflags: [
+ "-Wthread-safety",
"-Wno-unused-parameter",
"-Wall",
"-Werror",