commit 0413d077f76df5fe464e4f39ab1efa091df1019e
author Mikhail Naganov <mnaganov@google.com> Thu Aug 15 14:12:39 2024 -0700
committer Mikhail Naganov <mnaganov@google.com> Thu Aug 15 14:18:51 2024 -0700
tree 5f23a50b02f5e3f148d7df29f5fb677e5890132d
parent fc8d55a0bf600824595a6afe82b3b618f64ca521

audio: Fix stream cleanup sequence

Move the cleanup of the stream worker thread from '~StreamCommonImpl'
up to concrete stream implementations. This is because when
the worker thread is stopping, it calls 'DriverInterface::shutdown'
method of the stream. At the time when '~StreamCommonImpl' is
running, the concrete stream class has already been destroyed.

The cleanup actually only happens in the case when the client
did not close the stream properly via 'IStreamCommon.close', or
when the stream creation has failed in the middle.

Bug: 355804294
Test: atest VtsHalAudioCoreTargetTest
Change-Id: Ie86f682af202976ed48d24338b2dffcfd20d9a76

audio/aidl/default/Stream.cpp

diff --git a/audio/aidl/default/Stream.cpp b/audio/aidl/default/Stream.cpp
index 389860f..eecc972 100644
--- a/audio/aidl/default/Stream.cpp
+++ b/audio/aidl/default/Stream.cpp
@@ -663,10 +663,14 @@
 }
 
 StreamCommonImpl::~StreamCommonImpl() {
-    if (!isClosed()) {
-        LOG(ERROR) << __func__ << ": stream was not closed prior to destruction, resource leak";
-        stopWorker();
-        // The worker and the context should clean up by themselves via destructors.
+    // It is responsibility of the class that implements 'DriverInterface' to call 'cleanupWorker'
+    // in the destructor. Note that 'cleanupWorker' can not be properly called from this destructor
+    // because any subclasses have already been destroyed and thus the 'DriverInterface'
+    // implementation is not valid. Thus, here it can only be asserted whether the subclass has done
+    // its job.
+    if (!mWorkerStopIssued && !isClosed()) {
+        LOG(FATAL) << __func__ << ": the stream implementation must call 'cleanupWorker' "
+                   << "in order to clean up the worker thread.";
     }
 }
 
@@ -770,10 +774,7 @@
 ndk::ScopedAStatus StreamCommonImpl::close() {
     LOG(DEBUG) << __func__;
     if (!isClosed()) {
-        stopWorker();
-        LOG(DEBUG) << __func__ << ": joining the worker thread...";
-        mWorker->join();
-        LOG(DEBUG) << __func__ << ": worker thread joined";
+        stopAndJoinWorker();
         onClose(mWorker->setClosed());
         return ndk::ScopedAStatus::ok();
     } else {
@@ -791,6 +792,20 @@
     return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_STATE);
 }
 
+void StreamCommonImpl::cleanupWorker() {
+    if (!isClosed()) {
+        LOG(ERROR) << __func__ << ": stream was not closed prior to destruction, resource leak";
+        stopAndJoinWorker();
+    }
+}
+
+void StreamCommonImpl::stopAndJoinWorker() {
+    stopWorker();
+    LOG(DEBUG) << __func__ << ": joining the worker thread...";
+    mWorker->join();
+    LOG(DEBUG) << __func__ << ": worker thread joined";
+}
+
 void StreamCommonImpl::stopWorker() {
     if (auto commandMQ = mContext.getCommandMQ(); commandMQ != nullptr) {
         LOG(DEBUG) << __func__ << ": asking the worker to exit...";
@@ -805,6 +820,7 @@
         }
         LOG(DEBUG) << __func__ << ": done";
     }
+    mWorkerStopIssued = true;
 }
 
 ndk::ScopedAStatus StreamCommonImpl::updateMetadataCommon(const Metadata& metadata) {