Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e28dd790 authored by Yu-Han Yang's avatar Yu-Han Yang
Browse files

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
parent b1b06175

gnss/aidl/default/GnssMeasurementInterface.cpp

+17 −8
Original line number Diff line number Diff line
@@ -35,7 +35,9 @@ using DeviceFileReader = ::android::hardware::gnss::common::DeviceFileReader; 
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 @@ void GnssMeasurementInterface::start(const bool enableCorrVecOutputs, 
        ALOGD("restarting since measurement has started");

        stop();

    }

    // Wait for stopping previous thread.

    waitForStoppingThreads();



    mIsActive = true;

    mThreads.emplace_back(std::thread([this, enableCorrVecOutputs, enableFullTracking]() {

        waitForStoppingThreads();

        mThreadBlocker.reset();

    mThread = std::thread([this, enableCorrVecOutputs, enableFullTracking]() {



        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);

        }

    }

}

gnss/aidl/default/GnssMeasurementInterface.h

+1 −1
Original line number Diff line number Diff line
@@ -52,7 +52,7 @@ struct GnssMeasurementInterface : public BnGnssMeasurementInterface { 
    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;

gnss/aidl/default/GnssNavigationMessageInterface.cpp

+16 −7
Original line number Diff line number Diff line
@@ -29,7 +29,9 @@ using GnssNavigationMessageType = GnssNavigationMessage::GnssNavigationMessageTy 


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 @@ 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);

        }

    }

}

gnss/aidl/default/GnssNavigationMessageInterface.h

+1 −1
Original line number Diff line number Diff line
@@ -40,7 +40,7 @@ struct GnssNavigationMessageInterface : public BnGnssNavigationMessageInterface 


    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;