From 0413d077f76df5fe464e4f39ab1efa091df1019e Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Thu, 15 Aug 2024 14:12:39 -0700 Subject: [PATCH] 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 | 32 ++++++++++++++----- audio/aidl/default/alsa/StreamAlsa.cpp | 4 +++ .../default/bluetooth/StreamBluetooth.cpp | 4 +++ audio/aidl/default/include/core-impl/Stream.h | 8 +++++ .../default/include/core-impl/StreamAlsa.h | 2 ++ .../include/core-impl/StreamBluetooth.h | 2 ++ .../include/core-impl/StreamRemoteSubmix.h | 2 ++ .../default/include/core-impl/StreamStub.h | 2 ++ .../default/include/core-impl/StreamUsb.h | 1 + .../default/r_submix/StreamRemoteSubmix.cpp | 4 +++ audio/aidl/default/stub/StreamStub.cpp | 4 +++ 11 files changed, 57 insertions(+), 8 deletions(-) diff --git a/audio/aidl/default/Stream.cpp b/audio/aidl/default/Stream.cpp index 389860fa2e..eecc9724af 100644 --- a/audio/aidl/default/Stream.cpp +++ b/audio/aidl/default/Stream.cpp @@ -663,10 +663,14 @@ bool StreamOutWorkerLogic::write(size_t clientSize, StreamDescriptor::Reply* rep } 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::removeEffect( 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 @@ ndk::ScopedAStatus StreamCommonImpl::prepareToClose() { 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 @@ void StreamCommonImpl::stopWorker() { } LOG(DEBUG) << __func__ << ": done"; } + mWorkerStopIssued = true; } ndk::ScopedAStatus StreamCommonImpl::updateMetadataCommon(const Metadata& metadata) { diff --git a/audio/aidl/default/alsa/StreamAlsa.cpp b/audio/aidl/default/alsa/StreamAlsa.cpp index e57d538cf9..f548903de2 100644 --- a/audio/aidl/default/alsa/StreamAlsa.cpp +++ b/audio/aidl/default/alsa/StreamAlsa.cpp @@ -37,6 +37,10 @@ StreamAlsa::StreamAlsa(StreamContext* context, const Metadata& metadata, int rea mConfig(alsa::getPcmConfig(getContext(), mIsInput)), mReadWriteRetries(readWriteRetries) {} +StreamAlsa::~StreamAlsa() { + cleanupWorker(); +} + ::android::status_t StreamAlsa::init() { return mConfig.has_value() ? ::android::OK : ::android::NO_INIT; } diff --git a/audio/aidl/default/bluetooth/StreamBluetooth.cpp b/audio/aidl/default/bluetooth/StreamBluetooth.cpp index efab4708e3..6e1a811450 100644 --- a/audio/aidl/default/bluetooth/StreamBluetooth.cpp +++ b/audio/aidl/default/bluetooth/StreamBluetooth.cpp @@ -66,6 +66,10 @@ StreamBluetooth::StreamBluetooth(StreamContext* context, const Metadata& metadat 1000), mBtDeviceProxy(btDeviceProxy) {} +StreamBluetooth::~StreamBluetooth() { + cleanupWorker(); +} + ::android::status_t StreamBluetooth::init() { std::lock_guard guard(mLock); if (mBtDeviceProxy == nullptr) { diff --git a/audio/aidl/default/include/core-impl/Stream.h b/audio/aidl/default/include/core-impl/Stream.h index 93ace96b5c..100b4c8e17 100644 --- a/audio/aidl/default/include/core-impl/Stream.h +++ b/audio/aidl/default/include/core-impl/Stream.h @@ -457,6 +457,11 @@ class StreamCommonImpl : virtual public StreamCommonInterface, virtual public Dr } virtual void onClose(StreamDescriptor::State statePriorToClosing) = 0; + // Any stream class implementing 'DriverInterface::shutdown' must call 'cleanupWorker' in + // the destructor in order to stop and join the worker thread in the case when the client + // has not called 'IStreamCommon::close' method. + void cleanupWorker(); + void stopAndJoinWorker(); void stopWorker(); const StreamContext& mContext; @@ -464,6 +469,9 @@ class StreamCommonImpl : virtual public StreamCommonInterface, virtual public Dr std::unique_ptr mWorker; ChildInterface mCommon; ConnectedDevices mConnectedDevices; + + private: + std::atomic mWorkerStopIssued = false; }; // Note: 'StreamIn/Out' can not be used on their own. Instead, they must be used for defining diff --git a/audio/aidl/default/include/core-impl/StreamAlsa.h b/audio/aidl/default/include/core-impl/StreamAlsa.h index 2c3b284448..035694614d 100644 --- a/audio/aidl/default/include/core-impl/StreamAlsa.h +++ b/audio/aidl/default/include/core-impl/StreamAlsa.h @@ -32,6 +32,8 @@ namespace aidl::android::hardware::audio::core { class StreamAlsa : public StreamCommonImpl { public: StreamAlsa(StreamContext* context, const Metadata& metadata, int readWriteRetries); + ~StreamAlsa(); + // Methods of 'DriverInterface'. ::android::status_t init() override; ::android::status_t drain(StreamDescriptor::DrainMode) override; diff --git a/audio/aidl/default/include/core-impl/StreamBluetooth.h b/audio/aidl/default/include/core-impl/StreamBluetooth.h index 7f4239cfb4..357a546153 100644 --- a/audio/aidl/default/include/core-impl/StreamBluetooth.h +++ b/audio/aidl/default/include/core-impl/StreamBluetooth.h @@ -41,6 +41,8 @@ class StreamBluetooth : public StreamCommonImpl { const std::shared_ptr<::android::bluetooth::audio::aidl::BluetoothAudioPortAidl>& btDeviceProxy, const ::aidl::android::hardware::bluetooth::audio::PcmConfiguration& pcmConfig); + ~StreamBluetooth(); + // Methods of 'DriverInterface'. ::android::status_t init() override; ::android::status_t drain(StreamDescriptor::DrainMode) override; diff --git a/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h b/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h index 0d50c9696a..6ea796808c 100644 --- a/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h +++ b/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h @@ -29,7 +29,9 @@ class StreamRemoteSubmix : public StreamCommonImpl { StreamRemoteSubmix( StreamContext* context, const Metadata& metadata, const ::aidl::android::media::audio::common::AudioDeviceAddress& deviceAddress); + ~StreamRemoteSubmix(); + // Methods of 'DriverInterface'. ::android::status_t init() override; ::android::status_t drain(StreamDescriptor::DrainMode) override; ::android::status_t flush() override; diff --git a/audio/aidl/default/include/core-impl/StreamStub.h b/audio/aidl/default/include/core-impl/StreamStub.h index 3857e0e75e..f4ee566110 100644 --- a/audio/aidl/default/include/core-impl/StreamStub.h +++ b/audio/aidl/default/include/core-impl/StreamStub.h @@ -23,6 +23,8 @@ namespace aidl::android::hardware::audio::core { class StreamStub : public StreamCommonImpl { public: StreamStub(StreamContext* context, const Metadata& metadata); + ~StreamStub(); + // Methods of 'DriverInterface'. ::android::status_t init() override; ::android::status_t drain(StreamDescriptor::DrainMode) override; diff --git a/audio/aidl/default/include/core-impl/StreamUsb.h b/audio/aidl/default/include/core-impl/StreamUsb.h index 608f27d410..694fccf355 100644 --- a/audio/aidl/default/include/core-impl/StreamUsb.h +++ b/audio/aidl/default/include/core-impl/StreamUsb.h @@ -29,6 +29,7 @@ namespace aidl::android::hardware::audio::core { class StreamUsb : public StreamAlsa { public: StreamUsb(StreamContext* context, const Metadata& metadata); + // Methods of 'DriverInterface'. ::android::status_t transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) override; diff --git a/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp b/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp index a266b54ae5..db105b6ea9 100644 --- a/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp +++ b/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp @@ -43,6 +43,10 @@ StreamRemoteSubmix::StreamRemoteSubmix(StreamContext* context, const Metadata& m mStreamConfig.sampleRate = context->getSampleRate(); } +StreamRemoteSubmix::~StreamRemoteSubmix() { + cleanupWorker(); +} + ::android::status_t StreamRemoteSubmix::init() { mCurrentRoute = SubmixRoute::findOrCreateRoute(mDeviceAddress, mStreamConfig); if (mCurrentRoute == nullptr) { diff --git a/audio/aidl/default/stub/StreamStub.cpp b/audio/aidl/default/stub/StreamStub.cpp index 2422fe4b3e..3b6a85aa68 100644 --- a/audio/aidl/default/stub/StreamStub.cpp +++ b/audio/aidl/default/stub/StreamStub.cpp @@ -39,6 +39,10 @@ StreamStub::StreamStub(StreamContext* context, const Metadata& metadata) mIsAsynchronous(!!getContext().getAsyncCallback()), mIsInput(isInput(metadata)) {} +StreamStub::~StreamStub() { + cleanupWorker(); +} + ::android::status_t StreamStub::init() { mIsInitialized = true; return ::android::OK;