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

2 files changed, 20 insertions, 9 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);
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 @@ private:
                     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 @@ private:
                     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