oboe: fix possible race on close

Protect native stream pointer with a shared_lock.

The race may have never ended badly.
But it was theoretically possible. So this prevents it.

Note that this requires that Oboe be built with C++17.
But apps can still use C++14.

Fixes #1180

CMakeLists.txt

@@ -72,7 +72,7 @@ target_include_directories(oboe
 #     Enable -Ofast
 target_compile_options(oboe
         PRIVATE
-        -std=c++14
+        -std=c++17
         -Wall
         -Wextra-semi
         -Wshadow

apps/OboeTester/app/src/main/cpp/NativeAudioContext.cpp

@@ -14,11 +14,18 @@
  * limitations under the License.
  */
 
+// Set to 1 for debugging race condition #1180 with mAAudioStream.
+// See also AudioStreamAAudio.cpp in Oboe.
+#define DEBUG_CLOSE_RACE 0
+
 #include <fstream>
 #include <iostream>
+#if DEBUG_CLOSE_RACE
+#include <thread>
+#endif // DEBUG_CLOSE_RACE
 #include <vector>
-#include <common/AudioClock.h>
 
+#include <common/AudioClock.h>
 #include "util/WaveFileWriter.h"
 
 #include "NativeAudioContext.h"
@@ -251,6 +258,20 @@ oboe::Result ActivityContext::start() {
         dataThread = new std::thread(threadCallback, this);
     }
 
+#ifdef DEBUG_CLOSE_RACE
+    // Also put a sleep for 400 msec in AudioStreamAAudio::updateFramesRead().
+    if (outputStream != nullptr) {
+        std::thread raceDebugger([outputStream]() {
+            while (outputStream->getState() != StreamState::Closed) {
+                int64_t framesRead = outputStream->getFramesRead();
+                LOGD("raceDebugger, framesRead = %d, state = %d",
+                     (int) framesRead, (int) outputStream->getState());
+            }
+        });
+        raceDebugger.detach();
+    }
+#endif // DEBUG_CLOSE_RACE
+
     return result;
 }
 

apps/OboeTester/app/src/main/cpp/NativeAudioContext.h

@@ -298,7 +298,7 @@ class ActivityContext {
     int32_t                      mSampleRate = 0; // TODO per stream
 
     std::atomic<bool>            threadEnabled{false};
-    std::thread                 *dataThread = nullptr;
+    std::thread                 *dataThread = nullptr; // FIXME never gets deleted
 
 private:
     int64_t mInputOpenedAt = 0;

include/oboe/AudioStream.h

@@ -130,7 +130,7 @@ class AudioStream : public AudioStreamBase {
      *
      * @return state or a negative error.
      */
-    virtual StreamState getState() const = 0;
+    virtual StreamState getState() = 0;
 
     /**
      * Wait until the stream's current state no longer matches the input state.
@@ -191,7 +191,7 @@ class AudioStream : public AudioStreamBase {
      * @return a result which is either Result::OK with the xRun count as the value, or a
      * Result::Error* code
      */
-    virtual ResultWithValue<int32_t> getXRunCount() const {
+    virtual ResultWithValue<int32_t> getXRunCount() {
         return ResultWithValue<int32_t>(Result::ErrorUnimplemented);
     }
 
@@ -205,7 +205,9 @@ class AudioStream : public AudioStreamBase {
      *
      * @return burst size
      */
-    virtual int32_t getFramesPerBurst() = 0;
+    int32_t getFramesPerBurst() const {
+        return mFramesPerBurst;
+    }
 
     /**
      * Get the number of bytes in each audio frame. This is calculated using the channel count
@@ -537,6 +539,12 @@ class AudioStream : public AudioStreamBase {
 
     oboe::Result         mErrorCallbackResult = oboe::Result::OK;
 
+    /**
+     * Number of frames which will be copied to/from the audio device in a single read/write
+     * operation
+     */
+    int32_t              mFramesPerBurst = kUnspecified;
+
 private:
 
     // Log the scheduler if it changes.

include/oboe/AudioStreamBase.h

@@ -196,11 +196,6 @@ class AudioStreamBase {
     int32_t                         mBufferCapacityInFrames = kUnspecified;
     /** Stream buffer size specified as a number of audio frames */
     int32_t                         mBufferSizeInFrames = kUnspecified;
-    /**
-     * Number of frames which will be copied to/from the audio device in a single read/write
-     * operation
-     */
-    int32_t                         mFramesPerBurst = kUnspecified;
 
     /** Stream sharing mode */
     SharingMode                     mSharingMode = SharingMode::Shared;

src/aaudio/AudioStreamAAudio.cpp

@@ -282,6 +282,7 @@ Result AudioStreamAAudio::open() {
             mLibLoader->stream_getPerformanceMode(mAAudioStream));
     mBufferCapacityInFrames = mLibLoader->stream_getBufferCapacity(mAAudioStream);
     mBufferSizeInFrames = mLibLoader->stream_getBufferSize(mAAudioStream);
+    mFramesPerBurst = mLibLoader->stream_getFramesPerBurst(mAAudioStream);
 
     // These were added in P so we have to check for the function pointer.
     if (mLibLoader->stream_getUsage != nullptr) {
@@ -318,8 +319,13 @@ Result AudioStreamAAudio::close() {
 
     AudioStream::close();
 
-    // This will delete the AAudio stream object so we need to null out the pointer.
-    AAudioStream *stream = mAAudioStream.exchange(nullptr);
+    AAudioStream *stream = nullptr;
+    {
+        // Wait for any methods using mAAudioStream to finish.
+        std::unique_lock<std::shared_mutex> lock2(mAAudioStreamLock);
+        // Closing will delete *mAAudioStream so we need to null out the pointer atomically.
+        stream = mAAudioStream.exchange(nullptr);
+    }
     if (stream != nullptr) {
         if (OboeGlobals::areWorkaroundsEnabled()) {
             // Make sure we are really stopped. Do it under mLock
@@ -541,29 +547,27 @@ Result AudioStreamAAudio::waitForStateChange(StreamState currentState,
 }
 
 ResultWithValue<int32_t> AudioStreamAAudio::setBufferSizeInFrames(int32_t requestedFrames) {
+    int32_t adjustedFrames = requestedFrames;
+    if (adjustedFrames > mBufferCapacityInFrames) {
+        adjustedFrames = mBufferCapacityInFrames;
+    }
+    // This calls getBufferSize() so avoid recursive lock.
+    adjustedFrames = QuirksManager::getInstance().clipBufferSize(*this, adjustedFrames);
 
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
-
     if (stream != nullptr) {
-        int32_t adjustedFrames = requestedFrames;
-        if (adjustedFrames > mBufferCapacityInFrames) {
-            adjustedFrames = mBufferCapacityInFrames;
-        }
-        adjustedFrames = QuirksManager::getInstance().clipBufferSize(*this, adjustedFrames);
-
         int32_t newBufferSize = mLibLoader->stream_setBufferSize(mAAudioStream, adjustedFrames);
-
         // Cache the result if it's valid
         if (newBufferSize > 0) mBufferSizeInFrames = newBufferSize;
-
         return ResultWithValue<int32_t>::createBasedOnSign(newBufferSize);
-
     } else {
         return ResultWithValue<int32_t>(Result::ErrorClosed);
     }
 }
 
-StreamState AudioStreamAAudio::getState() const {
+StreamState AudioStreamAAudio::getState() {
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
     if (stream != nullptr) {
         aaudio_stream_state_t aaudioState = mLibLoader->stream_getState(stream);
@@ -580,36 +584,40 @@ StreamState AudioStreamAAudio::getState() const {
 }
 
 int32_t AudioStreamAAudio::getBufferSizeInFrames() {
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
     if (stream != nullptr) {
         mBufferSizeInFrames = mLibLoader->stream_getBufferSize(stream);
     }
     return mBufferSizeInFrames;
 }
 
-int32_t AudioStreamAAudio::getFramesPerBurst() {
-    AAudioStream *stream = mAAudioStream.load();
-    if (stream != nullptr) {
-        mFramesPerBurst = mLibLoader->stream_getFramesPerBurst(stream);
-    }
-    return mFramesPerBurst;
-}
-
 void AudioStreamAAudio::updateFramesRead() {
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
+// Set to 1 for debugging race condition #1180 with mAAudioStream.
+// See also DEBUG_CLOSE_RACE in OboeTester.
+#define DEBUG_CLOSE_RACE 0
+#if DEBUG_CLOSE_RACE
+    // This is used when testing race conditions with close().
+    // See DEBUG_CLOSE_RACE in OboeTester
+    AudioClock::sleepForNanos(400 * kNanosPerMillisecond);
+#endif // DEBUG_CLOSE_RACE
     if (stream != nullptr) {
         mFramesRead = mLibLoader->stream_getFramesRead(stream);
     }
 }
 
 void AudioStreamAAudio::updateFramesWritten() {
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
     if (stream != nullptr) {
         mFramesWritten = mLibLoader->stream_getFramesWritten(stream);
     }
 }
 
-ResultWithValue<int32_t> AudioStreamAAudio::getXRunCount() const {
+ResultWithValue<int32_t> AudioStreamAAudio::getXRunCount() {
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
     if (stream != nullptr) {
         return ResultWithValue<int32_t>::createBasedOnSign(mLibLoader->stream_getXRunCount(stream));
@@ -621,11 +629,12 @@ ResultWithValue<int32_t> AudioStreamAAudio::getXRunCount() const {
 Result AudioStreamAAudio::getTimestamp(clockid_t clockId,
                                    int64_t *framePosition,
                                    int64_t *timeNanoseconds) {
+    if (getState() != StreamState::Started) {
+        return Result::ErrorInvalidState;
+    }
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
     if (stream != nullptr) {
-        if (getState() != StreamState::Started) {
-            return Result::ErrorInvalidState;
-        }
         return static_cast<Result>(mLibLoader->stream_getTimestamp(stream, clockId,
                                                framePosition, timeNanoseconds));
     } else {
@@ -634,11 +643,6 @@ Result AudioStreamAAudio::getTimestamp(clockid_t clockId,
 }
 
 ResultWithValue<double> AudioStreamAAudio::calculateLatencyMillis() {
-    AAudioStream *stream = mAAudioStream.load();
-    if (stream == nullptr) {
-        return ResultWithValue<double>(Result::ErrorClosed);
-    }
-
     // Get the time that a known audio frame was presented.
     int64_t hardwareFrameIndex;
     int64_t hardwareFrameHardwareTime;
@@ -676,6 +680,7 @@ ResultWithValue<double> AudioStreamAAudio::calculateLatencyMillis() {
 }
 
 bool AudioStreamAAudio::isMMapUsed() {
+    std::shared_lock<std::shared_mutex> lock(mAAudioStreamLock);
     AAudioStream *stream = mAAudioStream.load();
     if (stream != nullptr) {
         return AAudioExtensions::getInstance().isMMapUsed(stream);

src/aaudio/AudioStreamAAudio.h

@@ -18,6 +18,7 @@
 #define OBOE_STREAM_AAUDIO_H_
 
 #include <atomic>
+#include <shared_mutex>
 #include <mutex>
 #include <thread>
 
@@ -67,8 +68,7 @@ class AudioStreamAAudio : public AudioStream {
 
     ResultWithValue<int32_t> setBufferSizeInFrames(int32_t requestedFrames) override;
     int32_t getBufferSizeInFrames() override;
-    int32_t getFramesPerBurst() override;
-    ResultWithValue<int32_t> getXRunCount() const override;
+    ResultWithValue<int32_t> getXRunCount()  override;
     bool isXRunCountSupported() const override { return true; }
 
     ResultWithValue<double> calculateLatencyMillis() override;
@@ -81,7 +81,7 @@ class AudioStreamAAudio : public AudioStream {
                                        int64_t *framePosition,
                                        int64_t *timeNanoseconds) override;
 
-    StreamState getState() const override;
+    StreamState getState() override;
 
     AudioApi getAudioApi() const override {
         return AudioApi::AAudio;
@@ -120,6 +120,7 @@ class AudioStreamAAudio : public AudioStream {
 
     // pointer to the underlying 'C' AAudio stream, valid if open, null if closed
     std::atomic<AAudioStream *> mAAudioStream{nullptr};
+    std::shared_mutex           mAAudioStreamLock; // to protect mAAudioStream while closing
 
     static AAudioLoader *mLibLoader;
 

src/common/FilterAudioStream.h

@@ -56,6 +56,7 @@ class FilterAudioStream : public AudioStream, AudioStreamCallback {
         mBufferCapacityInFrames = mChildStream->getBufferCapacityInFrames();
         mPerformanceMode = mChildStream->getPerformanceMode();
         mInputPreset = mChildStream->getInputPreset();
+        mFramesPerBurst = mChildStream->getFramesPerBurst();
     }
 
     virtual ~FilterAudioStream() = default;
@@ -113,7 +114,7 @@ class FilterAudioStream : public AudioStream, AudioStreamCallback {
             int32_t numFrames,
             int64_t timeoutNanoseconds) override;
 
-    StreamState getState() const override {
+    StreamState getState() override {
         return mChildStream->getState();
     }
 
@@ -128,10 +129,6 @@ class FilterAudioStream : public AudioStream, AudioStreamCallback {
         return mChildStream->isXRunCountSupported();
     }
 
-    int32_t getFramesPerBurst() override {
-        return mChildStream->getFramesPerBurst();
-    }
-
     AudioApi getAudioApi() const override {
         return mChildStream->getAudioApi();
     }
@@ -159,7 +156,7 @@ class FilterAudioStream : public AudioStream, AudioStreamCallback {
         return mBufferSizeInFrames;
     }
 
-    ResultWithValue<int32_t> getXRunCount() const override {
+    ResultWithValue<int32_t> getXRunCount() override {
         return mChildStream->getXRunCount();
     }
 

src/opensles/AudioStreamBuffered.h

@@ -49,7 +49,7 @@ class AudioStreamBuffered : public AudioStream {
 
     int32_t getBufferCapacityInFrames() const override;
 
-    ResultWithValue<int32_t> getXRunCount() const override {
+    ResultWithValue<int32_t> getXRunCount() override {
         return ResultWithValue<int32_t>(mXRunCount);
     }
 

src/opensles/AudioStreamOpenSLES.cpp

@@ -354,10 +354,6 @@ SLresult AudioStreamOpenSLES::registerBufferQueueCallback() {
     return result;
 }
 
-int32_t AudioStreamOpenSLES::getFramesPerBurst() {
-    return mFramesPerBurst;
-}
-
 int64_t AudioStreamOpenSLES::getFramesProcessedByServer() {
     updateServiceFrameCounter();
     int64_t millis64 = mPositionMillis.get();

src/opensles/AudioStreamOpenSLES.h

@@ -56,10 +56,7 @@ class AudioStreamOpenSLES : public AudioStreamBuffered {
      *
      * @return state or a negative error.
      */
-    StreamState getState() const override { return mState.load(); }
-
-    int32_t getFramesPerBurst() override;
-
+    StreamState getState() override { return mState.load(); }
 
     AudioApi getAudioApi() const override {
         return AudioApi::OpenSLES;