aaudio: fix STOP hang by unlocking around join()
Unlock and lock around the join() because the callback
thread that we are joining may be blocked by the lock.
Check for DISCONNECTED state so the error callback is
also joined correctly.
Remove unnecessary waitForStateChange that could cause
a timeout delay on DISCONNECT.
Bug: 134963902
Test: test_stop_hang (see bug report)
Change-Id: I5080fa655da35b3221234a2bcafb6ccd71d3ca27
diff --git a/media/libaaudio/src/client/AudioStreamInternal.cpp b/media/libaaudio/src/client/AudioStreamInternal.cpp
index c7e8088..52eadd4 100644
--- a/media/libaaudio/src/client/AudioStreamInternal.cpp
+++ b/media/libaaudio/src/client/AudioStreamInternal.cpp
@@ -241,22 +241,18 @@
return result;
}
+// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::close() {
aaudio_result_t result = AAUDIO_OK;
ALOGV("%s(): mServiceStreamHandle = 0x%08X", __func__, mServiceStreamHandle);
if (mServiceStreamHandle != AAUDIO_HANDLE_INVALID) {
// Don't close a stream while it is running.
aaudio_stream_state_t currentState = getState();
- if (isActive()) {
+ // Don't close a stream while it is running. Stop it first.
+ // If DISCONNECTED then we should still try to stop in case the
+ // error callback is still running.
+ if (isActive() || currentState == AAUDIO_STREAM_STATE_DISCONNECTED) {
requestStop();
- aaudio_stream_state_t nextState;
- int64_t timeoutNanoseconds = MIN_TIMEOUT_NANOS;
- result = waitForStateChange(currentState, &nextState,
- timeoutNanoseconds);
- if (result != AAUDIO_OK) {
- ALOGW("%s() waitForStateChange() returned %d %s",
- __func__, result, AAudio_convertResultToText(result));
- }
}
setState(AAUDIO_STREAM_STATE_CLOSING);
aaudio_handle_t serviceStreamHandle = mServiceStreamHandle;
@@ -357,21 +353,31 @@
return calculateReasonableTimeout(getFramesPerBurst());
}
+// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::stopCallback()
{
- if (isDataCallbackActive()) {
+ if (isDataCallbackSet()
+ && (isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {
mCallbackEnabled.store(false);
- return joinThread(NULL);
+ return joinThread(NULL); // may temporarily unlock mStreamLock
} else {
return AAUDIO_OK;
}
}
+// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::requestStop() {
aaudio_result_t result = stopCallback();
if (result != AAUDIO_OK) {
return result;
}
+ // The stream may have been unlocked temporarily to let a callback finish
+ // and the callback may have stopped the stream.
+ // Check to make sure the stream still needs to be stopped.
+ // See also AudioStream::safeStop().
+ if (!(isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {
+ return AAUDIO_OK;
+ }
if (mServiceStreamHandle == AAUDIO_HANDLE_INVALID) {
ALOGW("%s() mServiceStreamHandle invalid = 0x%08X",
@@ -728,6 +734,7 @@
return mFramesPerBurst;
}
+// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::joinThread(void** returnArg) {
return AudioStream::joinThread(returnArg, calculateReasonableTimeout(getFramesPerBurst()));
}
diff --git a/media/libaaudio/src/client/AudioStreamInternalPlay.cpp b/media/libaaudio/src/client/AudioStreamInternalPlay.cpp
index 164ad2b..b8ef247 100644
--- a/media/libaaudio/src/client/AudioStreamInternalPlay.cpp
+++ b/media/libaaudio/src/client/AudioStreamInternalPlay.cpp
@@ -58,6 +58,7 @@
return result;
}
+// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternalPlay::requestPause()
{
aaudio_result_t result = stopCallback();
diff --git a/media/libaaudio/src/core/AudioStream.cpp b/media/libaaudio/src/core/AudioStream.cpp
index 25669be..9b77223 100644
--- a/media/libaaudio/src/core/AudioStream.cpp
+++ b/media/libaaudio/src/core/AudioStream.cpp
@@ -211,6 +211,7 @@
return result;
}
+// This must be called under mStreamLock.
aaudio_result_t AudioStream::safeStop() {
switch (getState()) {
@@ -247,6 +248,7 @@
}
aaudio_result_t AudioStream::safeClose() {
+ // This get temporarily unlocked in the close when joining callback threads.
std::lock_guard<std::mutex> lock(mStreamLock);
if (collidesWithCallback()) {
ALOGE("%s cannot be called from a callback!", __func__);
@@ -363,6 +365,7 @@
}
}
+// This must be called under mStreamLock.
aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanoseconds __unused)
{
if (!mHasThread) {
@@ -374,6 +377,8 @@
// then we don't need to join(). The thread is already about to exit.
if (pthread_self() != mThread) {
// Called from an app thread. Not the callback.
+ // Unlock because the callback may be trying to stop the stream but is blocked.
+ mStreamLock.unlock();
#if 0
// TODO implement equivalent of pthread_timedjoin_np()
struct timespec abstime;
@@ -381,6 +386,7 @@
#else
int err = pthread_join(mThread, returnArg);
#endif
+ mStreamLock.lock();
if (err) {
ALOGE("%s() pthread_join() returns err = %d", __func__, err);
result = AAudioConvert_androidToAAudioResult(-err);