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
diff --git a/gnss/aidl/default/GnssMeasurementInterface.cpp b/gnss/aidl/default/GnssMeasurementInterface.cpp
index aab9e03..f7e4d4a 100644
--- a/gnss/aidl/default/GnssMeasurementInterface.cpp
+++ b/gnss/aidl/default/GnssMeasurementInterface.cpp
@@ -35,7 +35,9 @@
std::shared_ptr<IGnssMeasurementCallback> GnssMeasurementInterface::sCallback = nullptr;
GnssMeasurementInterface::GnssMeasurementInterface()
- : mIntervalMs(1000), mLocationIntervalMs(1000), mFutures(std::vector<std::future<void>>()) {}
+ : mIntervalMs(1000), mLocationIntervalMs(1000) {
+ mThreads.reserve(2);
+}
GnssMeasurementInterface::~GnssMeasurementInterface() {
waitForStoppingThreads();
@@ -100,12 +102,12 @@
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 @@
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 926a4e7..ea07c9a 100644
--- a/gnss/aidl/default/GnssMeasurementInterface.h
+++ b/gnss/aidl/default/GnssMeasurementInterface.h
@@ -52,7 +52,7 @@
std::atomic<long> mLocationIntervalMs;
std::atomic<bool> mIsActive;
std::atomic<bool> mLocationEnabled;
- std::thread mThread;
+ std::vector<std::thread> mThreads;
std::vector<std::future<void>> mFutures;
::android::hardware::gnss::common::ThreadBlocker mThreadBlocker;
diff --git a/gnss/aidl/default/GnssNavigationMessageInterface.cpp b/gnss/aidl/default/GnssNavigationMessageInterface.cpp
index 75b9624..c262dc6 100644
--- a/gnss/aidl/default/GnssNavigationMessageInterface.cpp
+++ b/gnss/aidl/default/GnssNavigationMessageInterface.cpp
@@ -29,7 +29,9 @@
std::shared_ptr<IGnssNavigationMessageCallback> GnssNavigationMessageInterface::sCallback = nullptr;
-GnssNavigationMessageInterface::GnssNavigationMessageInterface() : mMinIntervalMillis(1000) {}
+GnssNavigationMessageInterface::GnssNavigationMessageInterface() : mMinIntervalMillis(1000) {
+ mThreads.reserve(2);
+}
GnssNavigationMessageInterface::~GnssNavigationMessageInterface() {
waitForStoppingThreads();
@@ -61,11 +63,11 @@
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 @@
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 b335348..e9a7536 100644
--- a/gnss/aidl/default/GnssNavigationMessageInterface.h
+++ b/gnss/aidl/default/GnssNavigationMessageInterface.h
@@ -40,7 +40,7 @@
std::atomic<long> mMinIntervalMillis;
std::atomic<bool> mIsActive;
- std::thread mThread;
+ std::vector<std::thread> mThreads;
std::vector<std::future<void>> mFutures;
::android::hardware::gnss::common::ThreadBlocker mThreadBlocker;