From 5af1c38cd10f41b0684857e5c8f3b370787c2b9f Mon Sep 17 00:00:00 2001 From: Nikolas Havrikov Date: Thu, 20 Jan 2022 17:28:38 +0100 Subject: Avoid accumulation of user state persist requests This CL tackles the problem which arises that arises from frequent association changes for a given user. Until now, every association change allocates a map of previously used ids, which it holds until the current user state is written to storage. Unfortunately, frequent association changes can lead to a significant pileup of persist requests, leading to out-of-memory errors and an unresponsive system. It does not help that the requests to write out the user state are handled by the singleton background thread, which is shared across all system services. This CL does not solve this particular issue, and so it remains a future work item. This CL introduces a dedicated PersistUserStateHandler, which differs from the current handler in two ways: 1. It only ever accepts one persist request per user at a time. This prevents the aforementioned request pileup and OOM errors. 2. It reads the previously used ids and the changed associations at the time of writing out the state instead of the time of request arrival. This does not change the intended behavior, and comes with the advantage of not having to allocate Runnables. Test: atest CtsCompanionDeviceManagerCoreTestCases Test: atest CtsCompanionDeviceManagerUiAutomationTestCases Change-Id: I9aaad5a25fc4c09fcd9808a84d8ba2ce7962bfc5 --- .../companion/CompanionDeviceManagerService.java | 39 ++++++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java b/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java index 1e50c80f0e71..6aea92fbe730 100644 --- a/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java +++ b/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java @@ -82,6 +82,7 @@ import android.net.NetworkPolicyManager; import android.os.Binder; import android.os.Environment; import android.os.Handler; +import android.os.Message; import android.os.Parcel; import android.os.PowerWhitelistManager; import android.os.RemoteCallbackList; @@ -185,6 +186,7 @@ public class CompanionDeviceManagerService extends SystemService private final CompanionDeviceManagerServiceInternal mLocalService = new LocalService(this); final Handler mMainHandler = Handler.getMain(); + private final PersistUserStateHandler mUserPersistenceHandler = new PersistUserStateHandler(); private CompanionDevicePresenceController mCompanionDevicePresenceController; /** @@ -366,9 +368,8 @@ public class CompanionDeviceManagerService extends SystemService final List updatedAssociations = mAssociationStore.getAssociationsForUser(userId); - final Map> usedIdsForUser = getPreviouslyUsedIdsForUser(userId); - BackgroundThread.getHandler().post(() -> - mPersistentStore.persistStateForUser(userId, updatedAssociations, usedIdsForUser)); + + mUserPersistenceHandler.postPersistUserState(userId); // Notify listeners if ADDED, REMOVED or UPDATED_ADDRESS_CHANGED. // Do NOT notify when UPDATED_ADDRESS_UNCHANGED, which means a minor tweak in association's @@ -381,6 +382,13 @@ public class CompanionDeviceManagerService extends SystemService restartBleScan(); } + private void persistStateForUser(@UserIdInt int userId) { + final List updatedAssociations = + mAssociationStore.getAssociationsForUser(userId); + final Map> usedIdsForUser = getPreviouslyUsedIdsForUser(userId); + mPersistentStore.persistStateForUser(userId, updatedAssociations, usedIdsForUser); + } + private void notifyListeners( @UserIdInt int userId, @NonNull List associations) { mListeners.broadcast((listener, callbackUserId) -> { @@ -1349,4 +1357,29 @@ public class CompanionDeviceManagerService extends SystemService mService.associationCleanUp(profile); } } + + /** + * This class is dedicated to handling requests to persist user state. + */ + @SuppressLint("HandlerLeak") + private class PersistUserStateHandler extends Handler { + PersistUserStateHandler() { + super(BackgroundThread.get().getLooper()); + } + + /** + * Persists user state unless there is already an outstanding request for the given user. + */ + synchronized void postPersistUserState(@UserIdInt int userId) { + if (!hasMessages(userId)) { + sendMessage(obtainMessage(userId)); + } + } + + @Override + public void handleMessage(@NonNull Message msg) { + final int userId = msg.what; + persistStateForUser(userId); + } + } } -- cgit v1.2.3-59-g8ed1b From 1c620df4b83294fdcb6cb0b9f42bf20c7b0e3834 Mon Sep 17 00:00:00 2001 From: Nikolas Havrikov Date: Thu, 20 Jan 2022 17:46:51 +0100 Subject: Ensure cdm clear cache command persists state This CL ensures that the clear-association-memory-cache shell command immediately persists all user state before dumping clearing caches. This prevents state change updates from getting lost. This is not a critical issue because this shell command is only used for testing, but it allows for more graceful and reliable test code. Test: atest CtsCompanionDeviceManagerCoreTestCases Test: atest CtsCompanionDeviceManagerUiAutomationTestCases Change-Id: Ib981862c587a86bce57c4a5d67c7c18a40b47235 --- .../companion/CompanionDeviceManagerService.java | 19 +++++++++++++++++++ .../server/companion/CompanionDeviceShellCommand.java | 1 + 2 files changed, 20 insertions(+) diff --git a/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java b/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java index 6aea92fbe730..b2b55765f178 100644 --- a/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java +++ b/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java @@ -77,6 +77,7 @@ import android.content.pm.PackageInfo; import android.content.pm.PackageItemInfo; import android.content.pm.PackageManager; import android.content.pm.PackageManagerInternal; +import android.content.pm.UserInfo; import android.net.MacAddress; import android.net.NetworkPolicyManager; import android.os.Binder; @@ -1358,6 +1359,17 @@ public class CompanionDeviceManagerService extends SystemService } } + /** + * This method must only be called from {@link CompanionDeviceShellCommand} for testing + * purposes only! + */ + void persistState() { + mUserPersistenceHandler.clearMessages(); + for (UserInfo user : mUserManager.getAliveUsers()) { + persistStateForUser(user.id); + } + } + /** * This class is dedicated to handling requests to persist user state. */ @@ -1376,6 +1388,13 @@ public class CompanionDeviceManagerService extends SystemService } } + /** + * Clears *ALL* outstanding persist requests for *ALL* users. + */ + synchronized void clearMessages() { + removeCallbacksAndMessages(null); + } + @Override public void handleMessage(@NonNull Message msg) { final int userId = msg.what; diff --git a/services/companion/java/com/android/server/companion/CompanionDeviceShellCommand.java b/services/companion/java/com/android/server/companion/CompanionDeviceShellCommand.java index 5f46d5c4c4bf..9b2bd82fcfed 100644 --- a/services/companion/java/com/android/server/companion/CompanionDeviceShellCommand.java +++ b/services/companion/java/com/android/server/companion/CompanionDeviceShellCommand.java @@ -83,6 +83,7 @@ class CompanionDeviceShellCommand extends android.os.ShellCommand { } break; case "clear-association-memory-cache": { + mService.persistState(); mService.loadAssociationsFromDisk(); } break; -- cgit v1.2.3-59-g8ed1b