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

Commit 300630f7 authored by Yu Shan's avatar Yu Shan
Browse files

Fix resource mgmt bug in SubscriptionManager. 

The subscription information map is not correctly cleared during
unsubscription.

Flag: EXEMPT HAL
Test: atest DefaultVehicleHalTest SubscriptionManagerTest
Bug: 38256329
Change-Id: I78968aec54eda291932a34e5ef6c3f3d0e194295
parent 7de54f20

automotive/vehicle/aidl/impl/current/vhal/include/DefaultVehicleHal.h

+1 −1
Original line number Diff line number Diff line
@@ -219,7 +219,7 @@ class DefaultVehicleHal final : public aidlvhal::BnVehicle { 
    // mBinderEvents.

    void onBinderDiedUnlinkedHandler();



    size_t countSubscribeClients();

    size_t countClients();



    // Handles the property change events in batch.

    void handleBatchedPropertyEvents(std::vector<aidlvhal::VehiclePropValue>&& batchedEvents);

automotive/vehicle/aidl/impl/current/vhal/include/SubscriptionManager.h

+1 −0
Original line number Diff line number Diff line
@@ -146,6 +146,7 @@ class SubscriptionManager final { 
  private:

    // Friend class for testing.

    friend class DefaultVehicleHalTest;

    friend class SubscriptionManagerTest;



    IVehicleHardware* mVehicleHardware;

automotive/vehicle/aidl/impl/current/vhal/src/DefaultVehicleHal.cpp

+7 −3
Original line number Diff line number Diff line
@@ -1336,15 +1336,19 @@ binder_status_t DefaultVehicleHal::dump(int fd, const char** args, uint32_t numA 
        dprintf(fd, "Containing %zu property configs\n", configsByPropIdCopy.size());

        dprintf(fd, "Currently have %zu getValues clients\n", mGetValuesClients.size());

        dprintf(fd, "Currently have %zu setValues clients\n", mSetValuesClients.size());

        dprintf(fd, "Currently have %zu subscribe clients\n", countSubscribeClients());

        dprintf(fd, "Currently have %zu subscribe clients\n",

                mSubscriptionManager->countPropertyChangeClients());

        dprintf(fd, "Currently have %zu supported values change subscribe clients\n",

                mSubscriptionManager->countSupportedValueChangeClients());

    }

    return STATUS_OK;

}



size_t DefaultVehicleHal::countSubscribeClients() {

    return mSubscriptionManager->countPropertyChangeClients();

size_t DefaultVehicleHal::countClients() {

    std::scoped_lock<std::mutex> lockGuard(mLock);

    return mGetValuesClients.size() + mSetValuesClients.size() +

           mSubscriptionManager->countPropertyChangeClients() +

           mSubscriptionManager->countSupportedValueChangeClients();

}



}  // namespace vehicle

automotive/vehicle/aidl/impl/current/vhal/src/SubscriptionManager.cpp

+1 −2
Original line number Diff line number Diff line
@@ -414,7 +414,6 @@ VhalResult<void> SubscriptionManager::subscribeSupportedValueChange( 
    std::scoped_lock<std::mutex> lockGuard(mLock);



    ClientIdType clientId = callback->asBinder().get();

    ALOGE("ClientId: %p", clientId);



    // It is possible that some of the [propId, areaId]s are already subscribed, IVehicleHardware

    // will ignore them.
@@ -479,7 +478,7 @@ VhalResult<void> SubscriptionManager::unsubscribeSupportedValueChangeLocked( 
            mSupportedValueChangeClientsByPropIdAreaId.end()) {

            mSupportedValueChangeClientsByPropIdAreaId[propIdAreaId].erase(clientId);

        }

        if (mSupportedValueChangeClientsByPropIdAreaId.empty()) {

        if (mSupportedValueChangeClientsByPropIdAreaId[propIdAreaId].empty()) {

            mSupportedValueChangeClientsByPropIdAreaId.erase(propIdAreaId);

        }

        mSupportedValueChangePropIdAreaIdsByClient[clientId].erase(propIdAreaId);

automotive/vehicle/aidl/impl/current/vhal/test/DefaultVehicleHalTest.cpp

+4 −6
Original line number Diff line number Diff line
@@ -445,11 +445,7 @@ class DefaultVehicleHalTest : public testing::Test { 


    size_t countPendingRequests() { return mVhal->mPendingRequestPool->countPendingRequests(); }



    size_t countClients() {

        std::scoped_lock<std::mutex> lockGuard(mVhal->mLock);

        return mVhal->mGetValuesClients.size() + mVhal->mSetValuesClients.size() +

               mVhal->countSubscribeClients();

    }

    size_t countClients() { return mVhal->countClients(); }



    std::shared_ptr<PendingRequestPool> getPool() { return mVhal->mPendingRequestPool; }
@@ -2575,8 +2571,10 @@ TEST_F(DefaultVehicleHalTest, testUnregisterSupportedValueChangeCallback) { 
    ASSERT_TRUE(status.isOk()) << "Get non-okay status from unregisterSupportedValueChangeCallback"

                               << status.getMessage();



    ASSERT_TRUE(getHardware()->getSubscribedSupportedValueChangePropIdAreaIds().empty())

    EXPECT_TRUE(getHardware()->getSubscribedSupportedValueChangePropIdAreaIds().empty())

            << "All registered [propId, areaId]s must be unregistered";

    EXPECT_EQ(countClients(), static_cast<size_t>(0)) << "subscribe clients must be cleared";

    EXPECT_TRUE(hasNoSubscriptions()) << "subscribe clients must be cleared";

}



TEST_F(DefaultVehicleHalTest, testUnregisterSupportedValueChangeCallback_errorFromHardware) {