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

Commit 8cddcd9f authored by Yu Shan's avatar Yu Shan
Browse files

Do nothing when unsubscribe a not-subscribed property. 

Previously we will throw INVALID_ARG error if the client try
to call unsubscribe but the client has no subscribed property IDs.
We should not throw error in this case because the client should
always be okay to retry an unsubscribe operation.

Flag: EXEMPT HAL change
Test: atest DefaultVehicleHalTest
Bug: 384100005
Change-Id: Ie01850bac7f619fd3d3dbddfc9ab2a322cfd85cb
parent bf8be291

automotive/vehicle/aidl/android/hardware/automotive/vehicle/IVehicle.aidl

+2 −0
Original line number Diff line number Diff line
@@ -237,6 +237,8 @@ interface IVehicle { 
     * {@link StatusCode#INVALID_ARG}. If a specified propId was not subscribed

     * before, this method must ignore that propId.

     *

     * Unsubscribe a not-subscribed property ID must do nothing.

     *

     * If error is returned, some of the properties failed to unsubscribe.

     * Caller is safe to try again, since unsubscribing an already unsubscribed

     * property is okay.

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

+3 −4
Original line number Diff line number Diff line
@@ -95,15 +95,14 @@ class SubscriptionManager final { 
            bool isContinuousProperty);



    // Unsubscribes from the properties for the client.

    // Returns error if the client was not subscribed before, or one of the given property was not

    // subscribed, or one of the property failed to unsubscribe. Caller is safe to retry since

    // Returns error if one of the property failed to unsubscribe. Caller is safe to retry since

    // unsubscribing to an already unsubscribed property is okay (it would be ignored).

    // Returns ok if all the requested properties for the client are unsubscribed.

    VhalResult<void> unsubscribe(ClientIdType client, const std::vector<int32_t>& propIds);



    // Unsubscribes from all the properties for the client.

    // Returns error if the client was not subscribed before or one of the subscribed properties

    // for the client failed to unsubscribe. Caller is safe to retry.

    // Returns error one of the subscribed properties for the client failed to unsubscribe.

    // Caller is safe to retry.

    // Returns ok if all the properties for the client are unsubscribed.

    VhalResult<void> unsubscribe(ClientIdType client);

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

+4 −3
Original line number Diff line number Diff line
@@ -345,8 +345,8 @@ VhalResult<void> SubscriptionManager::unsubscribe(SubscriptionManager::ClientIdT 
    std::scoped_lock<std::mutex> lockGuard(mLock);



    if (mSubscribedPropsByClient.find(clientId) == mSubscribedPropsByClient.end()) {

        return StatusError(StatusCode::INVALID_ARG)

               << "No property was subscribed for the callback";

        ALOGW("No property was subscribed for the callback, unsubscribe does nothing");

        return {};

    }



    std::vector<PropIdAreaId> propIdAreaIdsToUnsubscribe;
@@ -378,7 +378,8 @@ VhalResult<void> SubscriptionManager::unsubscribe(SubscriptionManager::ClientIdT 
    std::scoped_lock<std::mutex> lockGuard(mLock);



    if (mSubscribedPropsByClient.find(clientId) == mSubscribedPropsByClient.end()) {

        return StatusError(StatusCode::INVALID_ARG) << "No property was subscribed for this client";

        ALOGW("No property was subscribed for this client, unsubscribe does nothing");

        return {};

    }



    auto& subscriptions = mSubscribedPropsByClient[clientId];

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

+2 −3
Original line number Diff line number Diff line
@@ -1842,12 +1842,11 @@ TEST_F(DefaultVehicleHalTest, testSubscribeAreaNoneAccess) { 
    ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::ACCESS_DENIED));

}



TEST_F(DefaultVehicleHalTest, testUnsubscribeFailure) {

TEST_F(DefaultVehicleHalTest, testUnsubscribeNotSubscribedProperty) {

    auto status = getClient()->unsubscribe(getCallbackClient(),

                                           std::vector<int32_t>({GLOBAL_ON_CHANGE_PROP}));



    ASSERT_FALSE(status.isOk()) << "unsubscribe to a not-subscribed property must fail";

    ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::INVALID_ARG));

    ASSERT_TRUE(status.isOk()) << "unsubscribe to a not-subscribed property must do nothing";

}



TEST_F(DefaultVehicleHalTest, testHeartbeatEvent) {

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

+4 −0
Original line number Diff line number Diff line
@@ -351,6 +351,10 @@ TEST_F(SubscriptionManagerTest, testUnsubscribeUnsubscribedPropId) { 
                                       std::vector<int32_t>({0, 1, 2}));

    ASSERT_TRUE(result.ok()) << "unsubscribe an unsubscribed property must do nothing";



    result = getManager()->unsubscribe(getCallbackClient()->asBinder().get(),

                                       std::vector<int32_t>({0, 1, 2}));

    ASSERT_TRUE(result.ok()) << "retry an unsubscribe operation must not throw error";



    std::vector<VehiclePropValue> updatedValues = {

            {

                    .prop = 0,