Added UserHalService.HalCallback.STATUS_CONCURRENT_OPERATION.
This status is called when a UserHalService method is called before
the HAL returned from a previous call to the same method.
Also fixed mPendingCallbacks thread-safety on UserHalService.
Test: atest CarServiceUnitTest:com.android.car.hal.UserHalServiceTest#testGetUserInfo_secondCallFailWhilePending
Test: atest CarServiceUnitTest:com.android.car.hal.UserHalServiceTest
Bug: 146207078
Bug: 150399261
Change-Id: I4970ca32f64b5166ec6eabfd039afb18d50facf1
diff --git a/service/src/com/android/car/hal/UserHalHelper.java b/service/src/com/android/car/hal/UserHalHelper.java
index 18f9a58..d5311dc 100644
--- a/service/src/com/android/car/hal/UserHalHelper.java
+++ b/service/src/com/android/car/hal/UserHalHelper.java
@@ -37,11 +37,13 @@
case HalCallback.STATUS_OK:
return "OK";
case HalCallback.STATUS_HAL_SET_TIMEOUT:
- return "STATUS_HAL_SET_TIMEOUT";
+ return "HAL_SET_TIMEOUT";
case HalCallback.STATUS_HAL_RESPONSE_TIMEOUT:
- return "STATUS_HAL_RESPONSE_TIMEOUT";
+ return "HAL_RESPONSE_TIMEOUT";
case HalCallback.STATUS_WRONG_HAL_RESPONSE:
- return "STATUS_WRONG_HAL_RESPONSE";
+ return "WRONG_HAL_RESPONSE";
+ case HalCallback.STATUS_CONCURRENT_OPERATION:
+ return "CONCURRENT_OPERATION";
default:
return "UNKNOWN-" + status;
}
diff --git a/service/src/com/android/car/hal/UserHalService.java b/service/src/com/android/car/hal/UserHalService.java
index 72d1fb1..336f28d 100644
--- a/service/src/com/android/car/hal/UserHalService.java
+++ b/service/src/com/android/car/hal/UserHalService.java
@@ -38,8 +38,8 @@
import android.util.Pair;
import android.util.Slog;
import android.util.SparseArray;
+import android.util.SparseIntArray;
-import com.android.car.CarLog;
import com.android.internal.annotations.GuardedBy;
import com.android.internal.util.Preconditions;
@@ -50,6 +50,8 @@
import java.util.Collection;
import java.util.List;
import java.util.Objects;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
/**
* Service used to integrate the OEM's custom user management with Android's.
@@ -58,11 +60,20 @@
private static final String UNSUPPORTED_MSG = "Vehicle HAL does not support user management";
- private static final String TAG = CarLog.TAG_USER;
+ private static final String TAG = UserHalService.class.getSimpleName();
// TODO(b/146207078): STOPSHIP - change to false before R is launched
private static final boolean DBG = true;
+ private static final int REQUEST_TYPE_GET_INITIAL_INFO = 1;
+
+ /** @hide */
+ @IntDef(prefix = { "REQUEST_TYPE_" }, value = {
+ REQUEST_TYPE_GET_INITIAL_INFO
+ })
+ @Retention(RetentionPolicy.SOURCE)
+ @interface RequestType{}
+
private final Object mLock = new Object();
private final VehicleHal mHal;
@@ -83,11 +94,17 @@
private int mNextRequestId = 1;
/**
- * Map of callback by request id.
+ * Map of callbacks by request id.
*/
@GuardedBy("mHandler")
private SparseArray<Pair<Class<?>, HalCallback<?>>> mPendingCallbacks = new SparseArray<>();
+ /**
+ * Map of request ids by {@link RequestType}.
+ */
+ @GuardedBy("mLock")
+ private SparseIntArray mPendingRequests = new SparseIntArray();
+
public UserHalService(VehicleHal hal) {
mHal = hal;
}
@@ -173,13 +190,15 @@
int STATUS_HAL_SET_TIMEOUT = 2;
int STATUS_HAL_RESPONSE_TIMEOUT = 3;
int STATUS_WRONG_HAL_RESPONSE = 4;
+ int STATUS_CONCURRENT_OPERATION = 5;
/** @hide */
@IntDef(prefix = { "STATUS_" }, value = {
STATUS_OK,
STATUS_HAL_SET_TIMEOUT,
STATUS_HAL_RESPONSE_TIMEOUT,
- STATUS_WRONG_HAL_RESPONSE
+ STATUS_WRONG_HAL_RESPONSE,
+ STATUS_CONCURRENT_OPERATION
})
@Retention(RetentionPolicy.SOURCE)
@interface HalCallbackStatus{}
@@ -203,6 +222,7 @@
}
}
+ @GuardedBy("mLock")
private void checkSupportedLocked() {
Preconditions.checkState(isSupported(), UNSUPPORTED_MSG);
}
@@ -232,7 +252,8 @@
int requestId;
synchronized (mLock) {
checkSupportedLocked();
- requestId = mNextRequestId++;
+ if (hasPendingRequestLocked(REQUEST_TYPE_GET_INITIAL_INFO, callback)) return;
+ requestId = addPendingRequestLocked(REQUEST_TYPE_GET_INITIAL_INFO);
// TODO(b/146207078): use helper method to convert request to prop value
propRequest.value.int32Values.add(requestId);
propRequest.value.int32Values.add(requestType);
@@ -245,9 +266,10 @@
propRequest.value.int32Values.add(userInfo.flags);
}
setTimestamp(propRequest);
- if (DBG) Log.d(TAG, "adding pending callback for request " + requestId);
- mPendingCallbacks.put(requestId, new Pair<>(InitialUserInfoResponse.class, callback));
}
+ mHandler.sendMessage(obtainMessage(
+ UserHalService::handleAddPendingRequest, this, requestId,
+ InitialUserInfoResponse.class, callback));
mHandler.sendMessageDelayed(obtainMessage(
UserHalService::handleCheckIfRequestTimedOut, this, requestId).setWhat(requestId),
@@ -261,6 +283,42 @@
}
}
+ private void handleAddPendingRequest(int requestId, @NonNull Class<?> responseClass,
+ @NonNull HalCallback<?> callback) {
+ if (DBG) {
+ Log.d(TAG, "adding pending callback (of type " + responseClass.getName()
+ + ") for request " + requestId);
+ }
+ mPendingCallbacks.put(requestId, new Pair<>(responseClass, callback));
+ }
+
+ /**
+ * Checks if there is a pending request of type {@code requestType}, calling {@code callback}
+ * with {@link HalCallback#STATUS_CONCURRENT_OPERATION} when there is.
+ */
+ private boolean hasPendingRequestLocked(@RequestType int requestType,
+ @NonNull HalCallback<?> callback) {
+ int index = mPendingRequests.indexOfKey(requestType);
+ if (index < 0) return false;
+
+ int requestId = mPendingRequests.valueAt(index);
+ Log.w(TAG, "Already have pending request of type " + requestTypeToString(requestType)
+ + ": id=" + requestId);
+
+ callback.onResponse(HalCallback.STATUS_CONCURRENT_OPERATION, null);
+ return true;
+ }
+
+ /**
+ * Adds a new pending request to the queue, returning its request id.
+ */
+ @GuardedBy("mLock")
+ private int addPendingRequestLocked(@RequestType int requestType) {
+ int requestId = mNextRequestId++;
+ mPendingRequests.put(requestType, requestId);
+ return requestId;
+ }
+
/**
* Removes the pending request and its timeout callback.
*/
@@ -334,7 +392,7 @@
return callback;
}
- private void setTimestamp(VehiclePropValue propRequest) {
+ private void setTimestamp(@NonNull VehiclePropValue propRequest) {
propRequest.timestamp = SystemClock.elapsedRealtime();
}
@@ -353,11 +411,50 @@
writer.printf("%s%s\n", indent, mProperties.valueAt(i));
}
writer.printf("next request id: %d\n", mNextRequestId);
- if (mPendingCallbacks.size() == 0) {
- writer.println("no pending callbacks");
+ int size = mPendingRequests.size();
+ if (size == 0) {
+ writer.println("no pending requests");
} else {
- writer.printf("pending callbacks: %s\n", mPendingCallbacks);
+ writer.printf("%d pending requests\n", size);
+ for (int i = 0; i < size; i++) {
+ String type = requestTypeToString(mPendingRequests.keyAt(i));
+ int requestId = mPendingRequests.valueAt(i);
+ writer.printf("%s#%d: %s=req_id(%d)\n", indent, i, type, requestId);
+ }
}
}
+
+ CountDownLatch latch = new CountDownLatch(1);
+ mHandler.sendMessage(obtainMessage(UserHalService::handleDump, this, writer, latch));
+ try {
+ if (!latch.await(500, TimeUnit.SECONDS)) {
+ writer.println("\nTIMED OUT");
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ writer.println("\nINTERRUPTED");
+ }
+ }
+
+ /**
+ * Dumps the state that's guarded by {@code mHandler}.
+ */
+ private void handleDump(@NonNull PrintWriter writer, @NonNull CountDownLatch latch) {
+ if (mPendingCallbacks.size() == 0) {
+ writer.println("no pending callbacks");
+ } else {
+ writer.printf("pending callbacks: %s\n", mPendingCallbacks);
+ }
+ latch.countDown();
+ }
+
+ @NonNull
+ private static String requestTypeToString(@RequestType int type) {
+ switch (type) {
+ case REQUEST_TYPE_GET_INITIAL_INFO:
+ return "TYPE_GET_INITIAL_INFO";
+ default:
+ return "UNKNOWN-" + type;
+ }
}
}
diff --git a/tests/carservice_unit_test/src/com/android/car/hal/UserHalServiceTest.java b/tests/carservice_unit_test/src/com/android/car/hal/UserHalServiceTest.java
index 9e3c4e3..d5f4ccc 100644
--- a/tests/carservice_unit_test/src/com/android/car/hal/UserHalServiceTest.java
+++ b/tests/carservice_unit_test/src/com/android/car/hal/UserHalServiceTest.java
@@ -196,6 +196,26 @@
}
@Test
+ public void testGetUserInfo_secondCallFailWhilePending() throws Exception {
+ GenericHalCallback<InitialUserInfoResponse> callback1 = new GenericHalCallback<>(
+ INITIAL_USER_CALLBACK_TIMEOUT_TIMEOUT);
+ GenericHalCallback<InitialUserInfoResponse> callback2 = new GenericHalCallback<>(
+ INITIAL_USER_CALLBACK_TIMEOUT_TIMEOUT);
+ mUserHalService.getInitialUserInfo(COLD_BOOT, INITIAL_USER_TIMEOUT_MS, mUsersInfo,
+ callback1);
+ mUserHalService.getInitialUserInfo(COLD_BOOT, INITIAL_USER_TIMEOUT_MS, mUsersInfo,
+ callback2);
+
+ callback1.assertCalled();
+ assertCallbackStatus(callback1, HalCallback.STATUS_HAL_RESPONSE_TIMEOUT);
+ assertThat(callback1.response).isNull();
+
+ callback2.assertCalled();
+ assertCallbackStatus(callback2, HalCallback.STATUS_CONCURRENT_OPERATION);
+ assertThat(callback1.response).isNull();
+ }
+
+ @Test
public void testGetUserInfo_halReplyWithWrongRequestId() throws Exception {
// TODO(b/146207078): use helper method to convert prop value to proper req
VehiclePropValue propResponse = new VehiclePropValue();
