From e28dd7900d353d17ed9da4afb6fa57b18d69d6fc Mon Sep 17 00:00:00 2001 From: Yu-Han Yang Date: Sun, 15 Oct 2023 17:47:23 +0000 Subject: [PATCH] Fix a deadlock in emulator HAL implementation Before this change, a start() call will wait for previous threads to finish. However, in ListenerMultiplexer.java in the framework, start(), stop(), and deliverListener() calls are contending for the same lock. Therefore, if a waiting start() is holding the lock, while the almost-finishing thread is also going to hold that lock for calling deliverListener(), a deadlock will happen. This CL moves the waiting logic into the new thread of the start() call, so that start() will return immediately. The new thread will wait for the old thread to finish, and then start the actual work. Bug: 299563185 Test: atest CtsLocationGnssTestCases Change-Id: Ic2993a6d82c24688fa98d26d336c85518c683cf6 --- .../aidl/default/GnssMeasurementInterface.cpp | 25 +++++++++++++------ gnss/aidl/default/GnssMeasurementInterface.h | 2 +- .../GnssNavigationMessageInterface.cpp | 23 +++++++++++------ .../default/GnssNavigationMessageInterface.h | 2 +- 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/gnss/aidl/default/GnssMeasurementInterface.cpp b/gnss/aidl/default/GnssMeasurementInterface.cpp index aab9e038d0..f7e4d4a542 100644 --- a/gnss/aidl/default/GnssMeasurementInterface.cpp +++ b/gnss/aidl/default/GnssMeasurementInterface.cpp @@ -35,7 +35,9 @@ using DeviceFileReader = ::android::hardware::gnss::common::DeviceFileReader; std::shared_ptr GnssMeasurementInterface::sCallback = nullptr; GnssMeasurementInterface::GnssMeasurementInterface() - : mIntervalMs(1000), mLocationIntervalMs(1000), mFutures(std::vector>()) {} + : mIntervalMs(1000), mLocationIntervalMs(1000) { + mThreads.reserve(2); +} GnssMeasurementInterface::~GnssMeasurementInterface() { waitForStoppingThreads(); @@ -100,12 +102,12 @@ void GnssMeasurementInterface::start(const bool enableCorrVecOutputs, ALOGD("restarting since measurement has started"); stop(); } - // Wait for stopping previous thread. - waitForStoppingThreads(); mIsActive = true; - mThreadBlocker.reset(); - mThread = std::thread([this, enableCorrVecOutputs, enableFullTracking]() { + mThreads.emplace_back(std::thread([this, enableCorrVecOutputs, enableFullTracking]() { + waitForStoppingThreads(); + mThreadBlocker.reset(); + int intervalMs; do { if (!mIsActive) { @@ -134,15 +136,22 @@ void GnssMeasurementInterface::start(const bool enableCorrVecOutputs, intervalMs = (mLocationEnabled) ? std::min(mLocationIntervalMs, mIntervalMs) : mIntervalMs; } while (mIsActive && mThreadBlocker.wait_for(std::chrono::milliseconds(intervalMs))); - }); + })); } void GnssMeasurementInterface::stop() { ALOGD("stop"); mIsActive = false; mThreadBlocker.notify(); - if (mThread.joinable()) { - mFutures.push_back(std::async(std::launch::async, [this] { mThread.join(); })); + for (auto iter = mThreads.begin(); iter != mThreads.end(); ++iter) { + if (iter->joinable()) { + mFutures.push_back(std::async(std::launch::async, [this, iter] { + iter->join(); + mThreads.erase(iter); + })); + } else { + mThreads.erase(iter); + } } } diff --git a/gnss/aidl/default/GnssMeasurementInterface.h b/gnss/aidl/default/GnssMeasurementInterface.h index 926a4e759b..ea07c9a696 100644 --- a/gnss/aidl/default/GnssMeasurementInterface.h +++ b/gnss/aidl/default/GnssMeasurementInterface.h @@ -52,7 +52,7 @@ struct GnssMeasurementInterface : public BnGnssMeasurementInterface { std::atomic mLocationIntervalMs; std::atomic mIsActive; std::atomic mLocationEnabled; - std::thread mThread; + std::vector mThreads; std::vector> mFutures; ::android::hardware::gnss::common::ThreadBlocker mThreadBlocker; diff --git a/gnss/aidl/default/GnssNavigationMessageInterface.cpp b/gnss/aidl/default/GnssNavigationMessageInterface.cpp index 75b962452a..c262dc6b88 100644 --- a/gnss/aidl/default/GnssNavigationMessageInterface.cpp +++ b/gnss/aidl/default/GnssNavigationMessageInterface.cpp @@ -29,7 +29,9 @@ using GnssNavigationMessageType = GnssNavigationMessage::GnssNavigationMessageTy std::shared_ptr GnssNavigationMessageInterface::sCallback = nullptr; -GnssNavigationMessageInterface::GnssNavigationMessageInterface() : mMinIntervalMillis(1000) {} +GnssNavigationMessageInterface::GnssNavigationMessageInterface() : mMinIntervalMillis(1000) { + mThreads.reserve(2); +} GnssNavigationMessageInterface::~GnssNavigationMessageInterface() { waitForStoppingThreads(); @@ -61,11 +63,11 @@ void GnssNavigationMessageInterface::start() { ALOGD("restarting since nav msg has started"); stop(); } - // Wait for stopping previous thread. - waitForStoppingThreads(); mIsActive = true; - mThread = std::thread([this]() { + mThreads.emplace_back(std::thread([this]() { + waitForStoppingThreads(); + mThreadBlocker.reset(); do { if (!mIsActive) { break; @@ -81,15 +83,22 @@ void GnssNavigationMessageInterface::start() { this->reportMessage(message); } while (mIsActive && mThreadBlocker.wait_for(std::chrono::milliseconds(mMinIntervalMillis))); - }); + })); } void GnssNavigationMessageInterface::stop() { ALOGD("stop"); mIsActive = false; mThreadBlocker.notify(); - if (mThread.joinable()) { - mFutures.push_back(std::async(std::launch::async, [this] { mThread.join(); })); + for (auto iter = mThreads.begin(); iter != mThreads.end(); ++iter) { + if (iter->joinable()) { + mFutures.push_back(std::async(std::launch::async, [this, iter] { + iter->join(); + mThreads.erase(iter); + })); + } else { + mThreads.erase(iter); + } } } diff --git a/gnss/aidl/default/GnssNavigationMessageInterface.h b/gnss/aidl/default/GnssNavigationMessageInterface.h index b3353480f4..e9a7536357 100644 --- a/gnss/aidl/default/GnssNavigationMessageInterface.h +++ b/gnss/aidl/default/GnssNavigationMessageInterface.h @@ -40,7 +40,7 @@ struct GnssNavigationMessageInterface : public BnGnssNavigationMessageInterface std::atomic mMinIntervalMillis; std::atomic mIsActive; - std::thread mThread; + std::vector mThreads; std::vector> mFutures; ::android::hardware::gnss::common::ThreadBlocker mThreadBlocker;