Increase the timeout in SystemTextClassifier to 1 minute
TetxClassifier APIs are supposed to call on a worker thread
(@WorkerThread), so there is no reason to enforce a short timeout like
2s.
We are seeing timeouts in some emulator tests.
SuggestConversationActions takes around 3s on the emulator.
I can imagine this can also happen on slow device or the input is
sufficiently long.
Also
- Fixed a bug that forgot to call callback.onFailure() when the caller
is not allowed to see the request.
- mOnServiceFailure should be @NonNull. It is important to invoke
callback.onFailure() to notify the client that the request has just
failed. Currently, there is only callsite that creates the
PendingRequest object and it is not null.
Test: atest -p external/libtextclassifier --all-abi
Test: atest TextClassifierServiceSwapTest
Fixes: 171543321
Change-Id: I24ca3e8d116073650ecf588593fc1e4beba86a45
diff --git a/core/java/android/view/textclassifier/SystemTextClassifier.java b/core/java/android/view/textclassifier/SystemTextClassifier.java
index 8eac1c1..2c844eb 100644
--- a/core/java/android/view/textclassifier/SystemTextClassifier.java
+++ b/core/java/android/view/textclassifier/SystemTextClassifier.java
@@ -89,7 +89,7 @@
try {
request.setSystemTextClassifierMetadata(mSystemTcMetadata);
final BlockingCallback<TextSelection> callback =
- new BlockingCallback<>("textselection");
+ new BlockingCallback<>("textselection", mSettings);
mManagerService.onSuggestSelection(mSessionId, request, callback);
final TextSelection selection = callback.get();
if (selection != null) {
@@ -112,7 +112,7 @@
try {
request.setSystemTextClassifierMetadata(mSystemTcMetadata);
final BlockingCallback<TextClassification> callback =
- new BlockingCallback<>("textclassification");
+ new BlockingCallback<>("textclassification", mSettings);
mManagerService.onClassifyText(mSessionId, request, callback);
final TextClassification classification = callback.get();
if (classification != null) {
@@ -142,7 +142,7 @@
try {
request.setSystemTextClassifierMetadata(mSystemTcMetadata);
final BlockingCallback<TextLinks> callback =
- new BlockingCallback<>("textlinks");
+ new BlockingCallback<>("textlinks", mSettings);
mManagerService.onGenerateLinks(mSessionId, request, callback);
final TextLinks links = callback.get();
if (links != null) {
@@ -193,7 +193,7 @@
try {
request.setSystemTextClassifierMetadata(mSystemTcMetadata);
final BlockingCallback<TextLanguage> callback =
- new BlockingCallback<>("textlanguage");
+ new BlockingCallback<>("textlanguage", mSettings);
mManagerService.onDetectLanguage(mSessionId, request, callback);
final TextLanguage textLanguage = callback.get();
if (textLanguage != null) {
@@ -213,7 +213,7 @@
try {
request.setSystemTextClassifierMetadata(mSystemTcMetadata);
final BlockingCallback<ConversationActions> callback =
- new BlockingCallback<>("conversation-actions");
+ new BlockingCallback<>("conversation-actions", mSettings);
mManagerService.onSuggestConversationActions(mSessionId, request, callback);
final ConversationActions conversationActions = callback.get();
if (conversationActions != null) {
@@ -279,8 +279,8 @@
extends ITextClassifierCallback.Stub {
private final ResponseReceiver<T> mReceiver;
- BlockingCallback(String name) {
- mReceiver = new ResponseReceiver<>(name);
+ BlockingCallback(String name, TextClassificationConstants settings) {
+ mReceiver = new ResponseReceiver<>(name, settings);
}
@Override
@@ -303,10 +303,12 @@
private final CountDownLatch mLatch = new CountDownLatch(1);
private final String mName;
+ private final TextClassificationConstants mSettings;
private T mResponse;
- private ResponseReceiver(String name) {
+ private ResponseReceiver(String name, TextClassificationConstants settings) {
mName = name;
+ mSettings = settings;
}
public void onSuccess(T response) {
@@ -327,7 +329,9 @@
// NOTE that TextClassifier calls should preferably always be called on a worker thread.
if (Looper.myLooper() != Looper.getMainLooper()) {
try {
- boolean success = mLatch.await(2, TimeUnit.SECONDS);
+ boolean success = mLatch.await(
+ mSettings.getSystemTextClassifierApiTimeoutInSecond(),
+ TimeUnit.SECONDS);
if (!success) {
Log.w(LOG_TAG, "Timeout in ResponseReceiver.get(): " + mName);
}
diff --git a/core/java/android/view/textclassifier/TextClassificationConstants.java b/core/java/android/view/textclassifier/TextClassificationConstants.java
index adb6fea..2975afc 100644
--- a/core/java/android/view/textclassifier/TextClassificationConstants.java
+++ b/core/java/android/view/textclassifier/TextClassificationConstants.java
@@ -82,6 +82,14 @@
static final String TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE =
"textclassifier_service_package_override";
+ /**
+ * The timeout value in seconds used by {@link SystemTextClassifier} for each TextClassifier
+ * API calls.
+ */
+ @VisibleForTesting
+ static final String SYSTEM_TEXT_CLASSIFIER_API_TIMEOUT_IN_SECOND =
+ "system_textclassifier_api_timeout_in_second";
+
private static final String DEFAULT_TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE = null;
private static final boolean LOCAL_TEXT_CLASSIFIER_ENABLED_DEFAULT = true;
private static final boolean SYSTEM_TEXT_CLASSIFIER_ENABLED_DEFAULT = true;
@@ -91,6 +99,7 @@
private static final boolean SMART_LINKIFY_ENABLED_DEFAULT = true;
private static final boolean SMART_SELECT_ANIMATION_ENABLED_DEFAULT = true;
private static final int GENERATE_LINKS_MAX_TEXT_LENGTH_DEFAULT = 100 * 1000;
+ private static final long SYSTEM_TEXT_CLASSIFIER_API_TIMEOUT_IN_SECOND_DEFAULT = 60;
@Nullable
public String getTextClassifierServicePackageOverride() {
@@ -140,27 +149,27 @@
GENERATE_LINKS_MAX_TEXT_LENGTH, GENERATE_LINKS_MAX_TEXT_LENGTH_DEFAULT);
}
+ public long getSystemTextClassifierApiTimeoutInSecond() {
+ return DeviceConfig.getLong(DeviceConfig.NAMESPACE_TEXTCLASSIFIER,
+ SYSTEM_TEXT_CLASSIFIER_API_TIMEOUT_IN_SECOND,
+ SYSTEM_TEXT_CLASSIFIER_API_TIMEOUT_IN_SECOND_DEFAULT);
+ }
+
void dump(IndentingPrintWriter pw) {
pw.println("TextClassificationConstants:");
pw.increaseIndent();
- pw.printPair("generate_links_max_text_length", getGenerateLinksMaxTextLength())
- .println();
- pw.printPair("local_textclassifier_enabled", isLocalTextClassifierEnabled())
- .println();
- pw.printPair("model_dark_launch_enabled", isModelDarkLaunchEnabled())
- .println();
- pw.printPair("smart_linkify_enabled", isSmartLinkifyEnabled())
- .println();
- pw.printPair("smart_select_animation_enabled", isSmartSelectionAnimationEnabled())
- .println();
- pw.printPair("smart_selection_enabled", isSmartSelectionEnabled())
- .println();
- pw.printPair("smart_text_share_enabled", isSmartTextShareEnabled())
- .println();
- pw.printPair("system_textclassifier_enabled", isSystemTextClassifierEnabled())
- .println();
- pw.printPair("textclassifier_service_package_override",
+ pw.print(GENERATE_LINKS_MAX_TEXT_LENGTH, getGenerateLinksMaxTextLength()).println();
+ pw.print(LOCAL_TEXT_CLASSIFIER_ENABLED, isLocalTextClassifierEnabled()).println();
+ pw.print(MODEL_DARK_LAUNCH_ENABLED, isModelDarkLaunchEnabled()).println();
+ pw.print(SMART_LINKIFY_ENABLED, isSmartLinkifyEnabled()).println();
+ pw.print(SMART_SELECT_ANIMATION_ENABLED, isSmartSelectionAnimationEnabled()).println();
+ pw.print(SMART_SELECTION_ENABLED, isSmartSelectionEnabled()).println();
+ pw.print(SMART_TEXT_SHARE_ENABLED, isSmartTextShareEnabled()).println();
+ pw.print(SYSTEM_TEXT_CLASSIFIER_ENABLED, isSystemTextClassifierEnabled()).println();
+ pw.print(TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE,
getTextClassifierServicePackageOverride()).println();
+ pw.print(SYSTEM_TEXT_CLASSIFIER_API_TIMEOUT_IN_SECOND,
+ getSystemTextClassifierApiTimeoutInSecond()).println();
pw.decreaseIndent();
}
}
\ No newline at end of file
diff --git a/core/tests/coretests/src/android/view/textclassifier/TextClassificationConstantsTest.java b/core/tests/coretests/src/android/view/textclassifier/TextClassificationConstantsTest.java
index 2f21b7f..16e750a 100644
--- a/core/tests/coretests/src/android/view/textclassifier/TextClassificationConstantsTest.java
+++ b/core/tests/coretests/src/android/view/textclassifier/TextClassificationConstantsTest.java
@@ -16,7 +16,7 @@
package android.view.textclassifier;
-import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.common.truth.Truth.assertThat;
import android.provider.DeviceConfig;
@@ -26,73 +26,63 @@
import org.junit.Test;
import org.junit.runner.RunWith;
+import java.util.function.Consumer;
+
@SmallTest
@RunWith(AndroidJUnit4.class)
public class TextClassificationConstantsTest {
- private static final float EPSILON = 0.0001f;
@Test
- public void testLoadFromDeviceConfig_booleanValue() throws Exception {
- // Saves config original value.
- final String originalValue = DeviceConfig.getProperty(DeviceConfig.NAMESPACE_TEXTCLASSIFIER,
- TextClassificationConstants.LOCAL_TEXT_CLASSIFIER_ENABLED);
-
- final TextClassificationConstants constants = new TextClassificationConstants();
- try {
- // Sets and checks different value.
- setDeviceConfig(TextClassificationConstants.LOCAL_TEXT_CLASSIFIER_ENABLED, "false");
- assertWithMessage(TextClassificationConstants.LOCAL_TEXT_CLASSIFIER_ENABLED)
- .that(constants.isLocalTextClassifierEnabled()).isFalse();
- } finally {
- // Restores config original value.
- setDeviceConfig(TextClassificationConstants.LOCAL_TEXT_CLASSIFIER_ENABLED,
- originalValue);
- }
+ public void booleanSettings() {
+ assertSettings(
+ TextClassificationConstants.LOCAL_TEXT_CLASSIFIER_ENABLED,
+ "false",
+ settings -> assertThat(settings.isLocalTextClassifierEnabled()).isFalse());
}
@Test
- public void testLoadFromDeviceConfig_IntValue() throws Exception {
- // Saves config original value.
- final String originalValue = DeviceConfig.getProperty(DeviceConfig.NAMESPACE_TEXTCLASSIFIER,
- TextClassificationConstants.GENERATE_LINKS_MAX_TEXT_LENGTH);
-
- final TextClassificationConstants constants = new TextClassificationConstants();
- try {
- // Sets and checks different value.
- setDeviceConfig(TextClassificationConstants.GENERATE_LINKS_MAX_TEXT_LENGTH, "8");
- assertWithMessage(TextClassificationConstants.GENERATE_LINKS_MAX_TEXT_LENGTH)
- .that(constants.getGenerateLinksMaxTextLength()).isEqualTo(8);
- } finally {
- // Restores config original value.
- setDeviceConfig(TextClassificationConstants.GENERATE_LINKS_MAX_TEXT_LENGTH,
- originalValue);
- }
+ public void intSettings() {
+ assertSettings(
+ TextClassificationConstants.GENERATE_LINKS_MAX_TEXT_LENGTH,
+ "128",
+ settings -> assertThat(settings.getGenerateLinksMaxTextLength()).isEqualTo(128));
}
@Test
- public void testLoadFromDeviceConfig_StringValue() throws Exception {
- // Saves config original value.
- final String originalValue = DeviceConfig.getProperty(DeviceConfig.NAMESPACE_TEXTCLASSIFIER,
- TextClassificationConstants.TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE);
+ public void stringSettings() {
+ assertSettings(
+ TextClassificationConstants.TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE,
+ "com.example.textclassifier",
+ settings -> assertThat(
+ settings.getTextClassifierServicePackageOverride())
+ .isEqualTo("com.example.textclassifier"));
+ }
- final TextClassificationConstants constants = new TextClassificationConstants();
+ @Test
+ public void longSettings() {
+ assertSettings(
+ TextClassificationConstants.SYSTEM_TEXT_CLASSIFIER_API_TIMEOUT_IN_SECOND,
+ "1",
+ settings -> assertThat(
+ settings.getSystemTextClassifierApiTimeoutInSecond())
+ .isEqualTo(1));
+ }
+
+ private static void assertSettings(
+ String key, String value, Consumer<TextClassificationConstants> settingsConsumer) {
+ final String originalValue =
+ DeviceConfig.getProperty(DeviceConfig.NAMESPACE_TEXTCLASSIFIER, key);
+ TextClassificationConstants settings = new TextClassificationConstants();
try {
- // Sets and checks different value.
- final String testTextClassifier = "com.example.textclassifier";
- setDeviceConfig(TextClassificationConstants.TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE,
- testTextClassifier);
- assertWithMessage(TextClassificationConstants.TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE)
- .that(constants.getTextClassifierServicePackageOverride()).isEqualTo(
- testTextClassifier);
+ setDeviceConfig(key, value);
+ settingsConsumer.accept(settings);
} finally {
- // Restores config original value.
- setDeviceConfig(TextClassificationConstants.TEXT_CLASSIFIER_SERVICE_PACKAGE_OVERRIDE,
- originalValue);
+ setDeviceConfig(key, originalValue);
}
}
- private void setDeviceConfig(String key, String value) {
- DeviceConfig.setProperty(DeviceConfig.NAMESPACE_TEXTCLASSIFIER, key,
- value, /* makeDefault */ false);
+ private static void setDeviceConfig(String key, String value) {
+ DeviceConfig.setProperty(
+ DeviceConfig.NAMESPACE_TEXTCLASSIFIER, key, value, /* makeDefault */ false);
}
}
diff --git a/services/core/java/com/android/server/textclassifier/TextClassificationManagerService.java b/services/core/java/com/android/server/textclassifier/TextClassificationManagerService.java
index 363e86d..93ba758 100644
--- a/services/core/java/com/android/server/textclassifier/TextClassificationManagerService.java
+++ b/services/core/java/com/android/server/textclassifier/TextClassificationManagerService.java
@@ -66,7 +66,6 @@
import com.android.internal.util.IndentingPrintWriter;
import com.android.internal.util.Preconditions;
import com.android.server.SystemService;
-import com.android.server.SystemService.TargetUser;
import java.io.FileDescriptor;
import java.io.PrintWriter;
@@ -458,6 +457,9 @@
callback.onFailure();
} else if (serviceState.isBoundLocked()) {
if (!serviceState.checkRequestAcceptedLocked(Binder.getCallingUid(), methodName)) {
+ Slog.w(LOG_TAG, String.format("UID %d is not allowed to see the %s request",
+ Binder.getCallingUid(), methodName));
+ callback.onFailure();
return;
}
textClassifierServiceConsumer.accept(serviceState.mService);
@@ -497,7 +499,7 @@
private final IBinder mBinder;
@NonNull
private final Runnable mRequest;
- @Nullable
+ @NonNull
private final Runnable mOnServiceFailure;
@GuardedBy("mLock")
@NonNull
@@ -516,7 +518,7 @@
* @param uid the calling uid of the request.
*/
PendingRequest(@Nullable String name,
- @NonNull ThrowingRunnable request, @Nullable ThrowingRunnable onServiceFailure,
+ @NonNull ThrowingRunnable request, @NonNull ThrowingRunnable onServiceFailure,
@Nullable IBinder binder,
@NonNull TextClassificationManagerService service,
@NonNull ServiceState serviceState, int uid) {
@@ -524,7 +526,8 @@
mRequest =
logOnFailure(Objects.requireNonNull(request), "handling pending request");
mOnServiceFailure =
- logOnFailure(onServiceFailure, "notifying callback of service failure");
+ logOnFailure(Objects.requireNonNull(onServiceFailure),
+ "notifying callback of service failure");
mBinder = binder;
mService = service;
mServiceState = Objects.requireNonNull(serviceState);
@@ -799,9 +802,7 @@
request -> {
Slog.w(LOG_TAG,
String.format("Pending request[%s] is dropped", request.mName));
- if (request.mOnServiceFailure != null) {
- request.mOnServiceFailure.run();
- }
+ request.mOnServiceFailure.run();
});
@Nullable
@GuardedBy("mLock")
@@ -843,15 +844,16 @@
while ((request = mPendingRequests.poll()) != null) {
if (isBoundLocked()) {
if (!checkRequestAcceptedLocked(request.mUid, request.mName)) {
- return;
- }
- request.mRequest.run();
- } else {
- if (request.mOnServiceFailure != null) {
- Slog.d(LOG_TAG, "Unable to bind TextClassifierService for PendingRequest "
- + request.mName);
+ Slog.w(LOG_TAG, String.format("UID %d is not allowed to see the %s request",
+ request.mUid, request.mName));
request.mOnServiceFailure.run();
+ } else {
+ request.mRequest.run();
}
+ } else {
+ Slog.d(LOG_TAG, "Unable to bind TextClassifierService for PendingRequest "
+ + request.mName);
+ request.mOnServiceFailure.run();
}
if (request.mBinder != null) {
