Remove crash on flush request timeout
Flush requests can get dropped when the system is too busy and CHRE
should avoid crashing the system in this case so the system can recover
on its own.
Unfortunately, this is slightly dangerous as a stale request can be
completed later and cause CHRE to treat a new request as complete, but
there isn't a way to solve this until CHRE can tie its requests to an
identifier.
Bug: 135558440
Test: Load onto device with short timeout and verify when
a timeout happens, an event is sent to nanoapps.
Change-Id: I51fc3d08a8841ce4058feaa150c93e9432e566ec
diff --git a/core/include/chre/core/sensor_request_manager.h b/core/include/chre/core/sensor_request_manager.h
index 2f363f8..2248c0e 100644
--- a/core/include/chre/core/sensor_request_manager.h
+++ b/core/include/chre/core/sensor_request_manager.h
@@ -374,6 +374,11 @@
void clearPendingFlushRequest();
/**
+ * Cancels the pending timeout timer associated with a flush request.
+ */
+ void cancelPendingFlushRequestTimer();
+
+ /**
* @return true if a flush through makeFlushRequest is pending.
*/
inline bool isFlushRequestPending() const {
diff --git a/core/sensor_request_manager.cc b/core/sensor_request_manager.cc
index 42750cb..09d7589 100644
--- a/core/sensor_request_manager.cc
+++ b/core/sensor_request_manager.cc
@@ -19,6 +19,7 @@
#include "chre_api/chre/version.h"
#include "chre/core/event_loop_manager.h"
#include "chre/platform/fatal_error.h"
+#include "chre/util/nested_data_ptr.h"
#include "chre/util/system/debug_dump.h"
namespace chre {
@@ -51,13 +52,6 @@
return success;
}
-void flushTimerCallback(uint16_t /* eventType */, void * /* data */) {
- // TODO: Fatal error here since some platforms may not be able to handle
- // timeouts gracefully. Modify this implementation to drop flush
- // requests and handle stale responses in the future appropriately.
- FATAL_ERROR("Flush request timed out");
-}
-
} // namespace
SensorRequestManager::SensorRequestManager() {
@@ -393,33 +387,31 @@
size_t sensorIndex = getSensorTypeArrayIndex(sensorType);
if (isValidSensorType(sensorType)
&& mSensorRequests[sensorIndex].isFlushRequestPending()) {
+
+ // Cancel flush request timer before posting to the event queue to ensure
+ // a timeout event isn't processed by CHRE now that the complete event
+ // has been received.
+ mSensorRequests[sensorIndex].cancelPendingFlushRequestTimer();
+
struct CallbackState {
uint8_t errorCode;
SensorType sensorType;
};
- // Enables passing data through void pointer to avoid allocation.
- union NestedCallbackState {
- void *eventData;
- CallbackState callbackState;
- };
- static_assert(sizeof(NestedCallbackState) == sizeof(void *),
- "Size of NestedCallbackState must equal that of void *");
-
- NestedCallbackState state = {};
- state.callbackState.errorCode = errorCode;
- state.callbackState.sensorType = sensorType;
+ NestedDataPtr<CallbackState> state = {};
+ state.data.errorCode = errorCode;
+ state.data.sensorType = sensorType;
auto callback = [](uint16_t /* eventType */, void *eventData) {
- NestedCallbackState nestedState;
- nestedState.eventData = eventData;
+ NestedDataPtr<CallbackState> nestedState;
+ nestedState.dataPtr = eventData;
EventLoopManagerSingleton::get()->getSensorRequestManager()
- .handleFlushCompleteEventSync(nestedState.callbackState.errorCode,
- nestedState.callbackState.sensorType);
+ .handleFlushCompleteEventSync(nestedState.data.errorCode,
+ nestedState.data.sensorType);
};
EventLoopManagerSingleton::get()->deferCallback(
- SystemCallbackType::SensorFlushComplete, state.eventData, callback);
+ SystemCallbackType::SensorFlushComplete, state.dataPtr, callback);
}
}
@@ -665,10 +657,29 @@
errorCode = CHRE_ERROR_NONE;
Nanoseconds delay = deadline - now;
request.isActive = true;
+
+ NestedDataPtr<SensorType> nestedType = {};
+ nestedType.data = request.sensorType;
+
+ auto callback = [](uint16_t /* eventType */, void * eventData) {
+ LOGE("Flush request timed out.");
+ NestedDataPtr<SensorType> nestedType;
+ nestedType.dataPtr = eventData;
+ // Send a complete event, thus closing out this flush request. If the
+ // request that has just timed out receives a response later, this may
+ // inadvertently close out a new request before it has actually
+ // completed.
+ // TODO: Attach an ID to all flush requests / responses so stale
+ // responses can be properly dropped.
+ EventLoopManagerSingleton::get()->getSensorRequestManager()
+ .handleFlushCompleteEventSync(CHRE_ERROR_TIMEOUT,
+ nestedType.data);
+ };
+
mFlushRequestTimerHandle =
EventLoopManagerSingleton::get()->setDelayedCallback(
- SystemCallbackType::SensorFlushTimeout, nullptr /* data */,
- flushTimerCallback, delay);
+ SystemCallbackType::SensorFlushTimeout, nestedType.dataPtr,
+ callback, delay);
}
} else {
// Flush request will be made once the pending request is completed.
@@ -681,12 +692,16 @@
}
void SensorRequestManager::SensorRequests::clearPendingFlushRequest() {
+ cancelPendingFlushRequestTimer();
+ mFlushRequestPending = false;
+}
+
+void SensorRequestManager::SensorRequests::cancelPendingFlushRequestTimer() {
if (mFlushRequestTimerHandle != CHRE_TIMER_INVALID) {
EventLoopManagerSingleton::get()->cancelDelayedCallback(
mFlushRequestTimerHandle);
mFlushRequestTimerHandle = CHRE_TIMER_INVALID;
}
- mFlushRequestPending = false;
}
bool SensorRequestManager::SensorRequests::doMakeFlushRequest() {
diff --git a/util/include/chre/util/nested_data_ptr.h b/util/include/chre/util/nested_data_ptr.h
new file mode 100644
index 0000000..f4e4b2c
--- /dev/null
+++ b/util/include/chre/util/nested_data_ptr.h
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef UTIL_CHRE_NESTED_DATA_PTR_H_
+#define UTIL_CHRE_NESTED_DATA_PTR_H_
+
+namespace chre {
+
+/**
+ * A template that provides the ability to store data inside of a void pointer
+ * to avoid allocating space on the heap in the case where the data is smaller
+ * than the size of a void pointer.
+ */
+template<typename DataType>
+union NestedDataPtr {
+ NestedDataPtr() {
+ static_assert(sizeof(NestedDataPtr<DataType>) == sizeof(void *),
+ "Size of NestedDataPtr must be equal to that of void *");
+ }
+ void *dataPtr;
+ DataType data;
+};
+
+} // namespace chre
+
+#endif // UTIL_CHRE_NESTED_DATA_PTR_H_
\ No newline at end of file
