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

1 files changed, 18 insertions, 8 deletions

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 @@ AudioFlinger::ThreadBase::ThreadBase(const sp<AudioFlinger>& audioFlinger, audio
         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 @@ AudioFlinger::ThreadBase::~ThreadBase()
 
 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 @@ status_t AudioFlinger::RecordThread::start(RecordThread::RecordTrack* recordTrac
         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 @@ void AudioFlinger::RecordThread::stop(RecordThread::RecordTrack* recordTrack) {
         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 @@ status_t AudioFlinger::closeOutput(audio_io_handle_t output)
         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 @@ status_t AudioFlinger::closeInput(audio_io_handle_t input)
         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);