Simplify ThreadBase::exit() aka requestExitAndWait
We can remove mExiting and use Thread::exitPending() instead.
The local sp<> on "this" in exit() is not needed, since the caller must
also hold an sp<> in order to be calling us. (Unless it was using a raw
pointer, but that would be dangerous for other reasons.)
Add comment explaining the mLock in exit().
Change-Id: I319e5107533a1a7cdbd13c292685f3e2be60f6c4
diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index 93c91fb..efa3c70 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -998,7 +998,7 @@
mChannelCount(0),
mFrameSize(1), mFormat(AUDIO_FORMAT_INVALID),
mParamStatus(NO_ERROR),
- mStandby(false), mId(id), mExiting(false),
+ mStandby(false), mId(id),
mDevice(device),
mDeathRecipient(new PMDeathRecipient(this))
{
@@ -1017,17 +1017,23 @@
void AudioFlinger::ThreadBase::exit()
{
- // keep a strong ref on ourself so that we won't get
- // destroyed in the middle of requestExitAndWait()
- sp <ThreadBase> strongMe = this;
-
ALOGV("ThreadBase::exit");
{
+ // This lock prevents the following race in thread (uniprocessor for illustration):
+ // if (!exitPending()) {
+ // // context switch from here to exit()
+ // // exit() calls requestExit(), what exitPending() observes
+ // // exit() calls signal(), which is dropped since no waiters
+ // // context switch back from exit() to here
+ // mWaitWorkCV.wait(...);
+ // // now thread is hung
+ // }
AutoMutex lock(mLock);
- mExiting = true;
requestExit();
mWaitWorkCV.signal();
}
+ // When Thread::requestExitAndWait is made virtual and this method is renamed to
+ // "virtual status_t requestExitAndWait()", replace by "return Thread::requestExitAndWait();"
requestExitAndWait();
}
@@ -4540,7 +4546,7 @@
ALOGV("Signal record thread");
mWaitWorkCV.signal();
// do not wait for mStartStopCond if exiting
- if (mExiting) {
+ if (exitPending()) {
mActiveTrack.clear();
status = INVALID_OPERATION;
goto startError;
@@ -4567,7 +4573,7 @@
if (mActiveTrack != 0 && recordTrack == mActiveTrack.get()) {
mActiveTrack->mState = TrackBase::PAUSING;
// do not wait for mStartStopCond if exiting
- if (mExiting) {
+ if (exitPending()) {
return;
}
mStartStopCond.wait(mLock);
@@ -5013,6 +5019,8 @@
mPlaybackThreads.removeItem(output);
}
thread->exit();
+ // The thread entity (active unit of execution) is no longer running here,
+ // but the ThreadBase container still exists.
if (thread->type() != ThreadBase::DUPLICATING) {
AudioStreamOut *out = thread->clearOutput();
@@ -5156,6 +5164,8 @@
mRecordThreads.removeItem(input);
}
thread->exit();
+ // The thread entity (active unit of execution) is no longer running here,
+ // but the ThreadBase container still exists.
AudioStreamIn *in = thread->clearInput();
assert(in != NULL);
diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h
index 97103c4..23cb924 100644
--- a/services/audioflinger/AudioFlinger.h
+++ b/services/audioflinger/AudioFlinger.h
@@ -419,6 +419,8 @@
audio_format_t format() const { return mFormat; }
size_t frameCount() const { return mFrameCount; }
void wakeUp() { mWaitWorkCV.broadcast(); }
+ // Should be "virtual status_t requestExitAndWait()" and override same
+ // method in Thread, but Thread::requestExitAndWait() is not yet virtual.
void exit();
virtual bool checkForNewParameters_l() = 0;
virtual status_t setParameters(const String8& keyValuePairs);
@@ -550,7 +552,6 @@
Vector<ConfigEvent> mConfigEvents;
bool mStandby;
const audio_io_handle_t mId;
- bool mExiting;
Vector< sp<EffectChain> > mEffectChains;
uint32_t mDevice; // output device for PlaybackThread
// input + output devices for RecordThread