Make refreshTimestamp atomic.
Make refreshTimestamp atomic in VehiclePropertyStore so that we
are sure the property value will not be updated after we read
the value out, but before we put the value with the new timestamp back.
Otherwise there is a slight chance that a property update will be
overwritten by the timestamp refresh operation.
Test: atest FakeVehicleHardwareTest
Bug: 308552957
Change-Id: I789257b4c75d8a4d19edeec31d01dc23776233ec
diff --git a/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp b/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp
index cc4fae1..acee9b3 100644
--- a/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp
+++ b/automotive/vehicle/aidl/impl/fake_impl/hardware/src/FakeVehicleHardware.cpp
@@ -2078,25 +2078,10 @@
enableVariableUpdateRate) {
eventMode = VehiclePropertyStore::EventMode::ON_VALUE_CHANGE;
}
- auto action = std::make_shared<RecurrentTimer::Callback>([this, propId, areaId,
- eventMode] {
- // Refresh the property value. In real implementation, this should poll the latest
- // value from vehicle bus. Here, we are just refreshing the existing value with a
- // new timestamp.
- auto result = getValue(VehiclePropValue{
- .areaId = areaId,
- .prop = propId,
- .value = {},
- });
- if (!result.ok()) {
- // Failed to read current value, skip refreshing.
- return;
- }
-
- mServerSidePropStore->writeValue(std::move(result.value()),
- /*updateStatus=*/true, eventMode,
- /*useCurrentTimestamp=*/true);
- });
+ auto action =
+ std::make_shared<RecurrentTimer::Callback>([this, propId, areaId, eventMode] {
+ mServerSidePropStore->refreshTimestamp(propId, areaId, eventMode);
+ });
mRecurrentTimer->registerTimerCallback(intervalInNanos, action);
mRecurrentActions[propIdAreaId] = action;
return StatusCode::OK;
diff --git a/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h b/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h
index 2812c3b..ef36532 100644
--- a/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h
+++ b/automotive/vehicle/aidl/impl/utils/common/include/VehiclePropertyStore.h
@@ -106,6 +106,11 @@
EventMode mode = EventMode::ON_VALUE_CHANGE,
bool useCurrentTimestamp = false) EXCLUDES(mLock);
+ // Refresh the timestamp for the stored property value for [propId, areaId]. If eventMode is
+ // always, generates the property update event, otherwise, only update the stored timestamp
+ // without generating event. This operation is atomic with other writeValue operations.
+ void refreshTimestamp(int32_t propId, int32_t areaId, EventMode eventMode) EXCLUDES(mLock);
+
// Remove a given property value from the property store. The 'propValue' would be used to
// generate the key for the value to remove.
void removeValue(
diff --git a/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp b/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp
index b879850..7d9d8b7 100644
--- a/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp
+++ b/automotive/vehicle/aidl/impl/utils/common/src/VehiclePropertyStore.cpp
@@ -176,6 +176,42 @@
return {};
}
+void VehiclePropertyStore::refreshTimestamp(int32_t propId, int32_t areaId, EventMode eventMode) {
+ VehiclePropValue updatedValue;
+ OnValueChangeCallback onValueChangeCallback = nullptr;
+ {
+ std::scoped_lock<std::mutex> g(mLock);
+
+ VehiclePropertyStore::Record* record = getRecordLocked(propId);
+ if (record == nullptr) {
+ return;
+ }
+
+ VehiclePropValue propValue = {
+ .areaId = areaId,
+ .prop = propId,
+ .value = {},
+ };
+
+ VehiclePropertyStore::RecordId recId = getRecordIdLocked(propValue, *record);
+ if (auto it = record->values.find(recId); it != record->values.end()) {
+ it->second->timestamp = elapsedRealtimeNano();
+ updatedValue = *(it->second);
+ } else {
+ return;
+ }
+ if (!mOnValueChangeCallback) {
+ return;
+ }
+ onValueChangeCallback = mOnValueChangeCallback;
+ }
+
+ // Invoke the callback outside the lock to prevent dead-lock.
+ if (eventMode == EventMode::ALWAYS) {
+ onValueChangeCallback(updatedValue);
+ }
+}
+
void VehiclePropertyStore::removeValue(const VehiclePropValue& propValue) {
std::scoped_lock<std::mutex> g(mLock);