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
diff --git a/automotive/vehicle/aidl/android/hardware/automotive/vehicle/IVehicle.aidl b/automotive/vehicle/aidl/android/hardware/automotive/vehicle/IVehicle.aidl
index 96c4953..cdf9066 100644
--- a/automotive/vehicle/aidl/android/hardware/automotive/vehicle/IVehicle.aidl
+++ b/automotive/vehicle/aidl/android/hardware/automotive/vehicle/IVehicle.aidl
@@ -237,6 +237,8 @@
* {@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.
diff --git a/automotive/vehicle/aidl/impl/current/vhal/include/SubscriptionManager.h b/automotive/vehicle/aidl/impl/current/vhal/include/SubscriptionManager.h
index 2f16fca..cac1901 100644
--- a/automotive/vehicle/aidl/impl/current/vhal/include/SubscriptionManager.h
+++ b/automotive/vehicle/aidl/impl/current/vhal/include/SubscriptionManager.h
@@ -95,15 +95,14 @@
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);
diff --git a/automotive/vehicle/aidl/impl/current/vhal/src/SubscriptionManager.cpp b/automotive/vehicle/aidl/impl/current/vhal/src/SubscriptionManager.cpp
index 14ee707..2d09e02 100644
--- a/automotive/vehicle/aidl/impl/current/vhal/src/SubscriptionManager.cpp
+++ b/automotive/vehicle/aidl/impl/current/vhal/src/SubscriptionManager.cpp
@@ -345,8 +345,8 @@
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 @@
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];
diff --git a/automotive/vehicle/aidl/impl/current/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/current/vhal/test/DefaultVehicleHalTest.cpp
index 992b0cc..4df5ba3 100644
--- a/automotive/vehicle/aidl/impl/current/vhal/test/DefaultVehicleHalTest.cpp
+++ b/automotive/vehicle/aidl/impl/current/vhal/test/DefaultVehicleHalTest.cpp
@@ -1842,12 +1842,11 @@
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) {
diff --git a/automotive/vehicle/aidl/impl/current/vhal/test/SubscriptionManagerTest.cpp b/automotive/vehicle/aidl/impl/current/vhal/test/SubscriptionManagerTest.cpp
index 5d58de5..2ac3a73 100644
--- a/automotive/vehicle/aidl/impl/current/vhal/test/SubscriptionManagerTest.cpp
+++ b/automotive/vehicle/aidl/impl/current/vhal/test/SubscriptionManagerTest.cpp
@@ -351,6 +351,10 @@
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,