From f23aae68ad17677270fe0c6bfef4545d858ed8a1 Mon Sep 17 00:00:00 2001 From: Yasin Kilicdere Date: Wed, 8 Feb 2023 10:30:54 +0000 Subject: [PATCH] Fix UserManagerTest waitForUserRemoval timeout. This CL introduces a new UserRemovalWaiter helper class to wait for user removal. Previously user removal was only waited until ACTION_USER_REMOVED broadcast was received for the user, which was delayed for several minutes in some cases and was causing timeouts on the tests and making them flaky. Now in addition to the broadcast, we are checking whether the user is present in the alive users collection. This CL also switches back to the original current user on the tearDown stage of the UserManagerTest. Bug: 264534887 Test: atest com.android.server.pm.UserManagerTest Change-Id: I7df7e324ca41acbd1e60aa28c37da172f6cf9d43 --- .../android/server/pm/UserManagerTest.java | 127 ++++++++---------- .../android/server/pm/UserRemovalWaiter.java | 100 ++++++++++++++ 2 files changed, 153 insertions(+), 74 deletions(-) create mode 100644 services/tests/servicestests/src/com/android/server/pm/UserRemovalWaiter.java diff --git a/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java b/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java index b4dd63189edda..ddd879a414163 100644 --- a/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java @@ -26,7 +26,6 @@ import static org.testng.Assert.assertThrows; import android.annotation.UserIdInt; import android.app.ActivityManager; import android.content.Context; -import android.content.Intent; import android.content.pm.PackageManager; import android.content.pm.UserInfo; import android.content.pm.UserProperties; @@ -46,8 +45,6 @@ import androidx.test.InstrumentationRegistry; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; -import com.android.compatibility.common.util.BlockingBroadcastReceiver; - import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; @@ -61,10 +58,13 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Function; import java.util.stream.Collectors; -/** Test {@link UserManager} functionality. */ +/** + * Test {@link UserManager} functionality. + * + * atest com.android.server.pm.UserManagerTest + */ @Postsubmit @RunWith(AndroidJUnit4.class) public final class UserManagerTest { @@ -88,13 +88,17 @@ public final class UserManagerTest { private PackageManager mPackageManager; private ArraySet mUsersToRemove; private UserSwitchWaiter mUserSwitchWaiter; + private UserRemovalWaiter mUserRemovalWaiter; + private int mOriginalCurrentUserId; @Before public void setUp() throws Exception { + mOriginalCurrentUserId = ActivityManager.getCurrentUser(); mUserManager = UserManager.get(mContext); mActivityManager = mContext.getSystemService(ActivityManager.class); mPackageManager = mContext.getPackageManager(); mUserSwitchWaiter = new UserSwitchWaiter(TAG, SWITCH_USER_TIMEOUT_SECONDS); + mUserRemovalWaiter = new UserRemovalWaiter(mContext, TAG, REMOVE_USER_TIMEOUT_SECONDS); mUsersToRemove = new ArraySet<>(); removeExistingUsers(); @@ -102,10 +106,14 @@ public final class UserManagerTest { @After public void tearDown() throws Exception { + if (mOriginalCurrentUserId != ActivityManager.getCurrentUser()) { + switchUser(mOriginalCurrentUserId); + } mUserSwitchWaiter.close(); // Making a copy of mUsersToRemove to avoid ConcurrentModificationException mUsersToRemove.stream().toList().forEach(this::removeUser); + mUserRemovalWaiter.close(); } private void removeExistingUsers() { @@ -394,11 +402,10 @@ public final class UserManagerTest { mUserManager.setUserRestriction(UserManager.DISALLOW_REMOVE_USER, /* value= */ true, asHandle(currentUser)); try { - runThenWaitForUserRemoval(() -> { - assertThat(mUserManager.removeUserWhenPossible(user1.getUserHandle(), - /* overrideDevicePolicy= */ true)) - .isEqualTo(UserManager.REMOVE_RESULT_REMOVED); - }, user1.id); // wait for user removal + assertThat(mUserManager.removeUserWhenPossible(user1.getUserHandle(), + /* overrideDevicePolicy= */ true)) + .isEqualTo(UserManager.REMOVE_RESULT_REMOVED); + waitForUserRemoval(user1.id); } finally { mUserManager.setUserRestriction(UserManager.DISALLOW_REMOVE_USER, /* value= */ false, asHandle(currentUser)); @@ -463,11 +470,10 @@ public final class UserManagerTest { assertThat(hasUser(user1.id)).isTrue(); assertThat(getUser(user1.id).isEphemeral()).isTrue(); - runThenWaitForUserRemoval(() -> { - // Switch back to the starting user. - switchUser(startUser); - // User will be removed once switch is complete - }, user1.id); // wait for user removal + // Switch back to the starting user. + switchUser(startUser); + // User will be removed once switch is complete + waitForUserRemoval(user1.id); assertThat(hasUser(user1.id)).isFalse(); } @@ -480,18 +486,17 @@ public final class UserManagerTest { // Switch to the user just created. switchUser(testUser.id); - runThenWaitForUserRemoval(() -> { - switchUserThenRun(startUser, () -> { - // While the switch is happening, call removeUserWhenPossible for the current user. - assertThat(mUserManager.removeUserWhenPossible(testUser.getUserHandle(), - /* overrideDevicePolicy= */ false)) - .isEqualTo(UserManager.REMOVE_RESULT_DEFERRED); + switchUserThenRun(startUser, () -> { + // While the switch is happening, call removeUserWhenPossible for the current user. + assertThat(mUserManager.removeUserWhenPossible(testUser.getUserHandle(), + /* overrideDevicePolicy= */ false)) + .isEqualTo(UserManager.REMOVE_RESULT_DEFERRED); - assertThat(hasUser(testUser.id)).isTrue(); - assertThat(getUser(testUser.id).isEphemeral()).isTrue(); - }); // wait for user switch - startUser - // User will be removed once switch is complete - }, testUser.id); // wait for user removal + assertThat(hasUser(testUser.id)).isTrue(); + assertThat(getUser(testUser.id).isEphemeral()).isTrue(); + }); // wait for user switch - startUser + // User will be removed once switch is complete + waitForUserRemoval(testUser.id); assertThat(hasUser(testUser.id)).isFalse(); } @@ -512,11 +517,10 @@ public final class UserManagerTest { assertThat(getUser(testUser.id).isEphemeral()).isTrue(); }); // wait for user switch - testUser - runThenWaitForUserRemoval(() -> { - // Switch back to the starting user. - switchUser(startUser); - // User will be removed once switch is complete - }, testUser.id); // wait for user removal + // Switch back to the starting user. + switchUser(startUser); + // User will be removed once switch is complete + waitForUserRemoval(testUser.id); assertThat(hasUser(testUser.id)).isFalse(); } @@ -526,11 +530,10 @@ public final class UserManagerTest { public void testRemoveUserWhenPossible_nonCurrentUserRemoved() throws Exception { final UserInfo user1 = createUser("User 1", /* flags= */ 0); - runThenWaitForUserRemoval(() -> { - assertThat(mUserManager.removeUserWhenPossible(user1.getUserHandle(), - /* overrideDevicePolicy= */ false)) - .isEqualTo(UserManager.REMOVE_RESULT_REMOVED); - }, user1.id); // wait for user removal + assertThat(mUserManager.removeUserWhenPossible(user1.getUserHandle(), + /* overrideDevicePolicy= */ false)) + .isEqualTo(UserManager.REMOVE_RESULT_REMOVED); + waitForUserRemoval(user1.id); assertThat(hasUser(user1.id)).isFalse(); } @@ -549,11 +552,10 @@ public final class UserManagerTest { UserManager.USER_TYPE_PROFILE_MANAGED, parentUser.id); - runThenWaitForUserRemoval(() -> { - assertThat(mUserManager.removeUserWhenPossible(parentUser.getUserHandle(), - /* overrideDevicePolicy= */ false)) - .isEqualTo(UserManager.REMOVE_RESULT_REMOVED); - }, parentUser.id); // wait for user removal + assertThat(mUserManager.removeUserWhenPossible(parentUser.getUserHandle(), + /* overrideDevicePolicy= */ false)) + .isEqualTo(UserManager.REMOVE_RESULT_REMOVED); + waitForUserRemoval(parentUser.id); assertThat(hasUser(parentUser.id)).isFalse(); assertThat(hasUser(cloneProfileUser.id)).isFalse(); @@ -1120,7 +1122,7 @@ public final class UserManagerTest { @Nullable private UserInfo getUser(int id) { - List list = mUserManager.getUsers(); + List list = mUserManager.getAliveUsers(); for (UserInfo user : list) { if (user.id == id) { @@ -1275,7 +1277,7 @@ public final class UserManagerTest { @MediumTest @Test public void testConcurrentUserCreate() throws Exception { - int userCount = mUserManager.getUserCount(); + int userCount = mUserManager.getAliveUsers().size(); int maxSupportedUsers = UserManager.getMaxSupportedUsers(); int canBeCreatedCount = maxSupportedUsers - userCount; // Test exceeding the limit while running in parallel @@ -1294,7 +1296,7 @@ public final class UserManagerTest { } es.shutdown(); es.awaitTermination(20, TimeUnit.SECONDS); - assertThat(mUserManager.getUserCount()).isEqualTo(maxSupportedUsers); + assertThat(mUserManager.getAliveUsers().size()).isEqualTo(maxSupportedUsers); assertThat(created.get()).isEqualTo(canBeCreatedCount); } @@ -1451,41 +1453,18 @@ public final class UserManagerTest { } private void removeUser(UserHandle userHandle) { - runThenWaitForUserRemoval( - () -> mUserManager.removeUser(userHandle), - userHandle == null ? UserHandle.USER_NULL : userHandle.getIdentifier() - ); + mUserManager.removeUser(userHandle); + waitForUserRemoval(userHandle.getIdentifier()); } private void removeUser(int userId) { - runThenWaitForUserRemoval( - () -> mUserManager.removeUser(userId), - userId - ); + mUserManager.removeUser(userId); + waitForUserRemoval(userId); } - private void runThenWaitForUserRemoval(Runnable runnable, int userIdToWaitUntilDeleted) { - if (!hasUser(userIdToWaitUntilDeleted)) { - runnable.run(); - mUsersToRemove.remove(userIdToWaitUntilDeleted); - return; - } - - Function checker = intent -> { - UserHandle userHandle = intent.getParcelableExtra(Intent.EXTRA_USER, UserHandle.class); - return userHandle != null && userHandle.getIdentifier() == userIdToWaitUntilDeleted; - }; - - BlockingBroadcastReceiver blockingBroadcastReceiver = BlockingBroadcastReceiver.create( - mContext, Intent.ACTION_USER_REMOVED, checker); - - blockingBroadcastReceiver.setTimeout(REMOVE_USER_TIMEOUT_SECONDS); - blockingBroadcastReceiver.register(); - - try (blockingBroadcastReceiver) { - runnable.run(); - } - mUsersToRemove.remove(userIdToWaitUntilDeleted); + private void waitForUserRemoval(int userId) { + mUserRemovalWaiter.waitFor(userId); + mUsersToRemove.remove(userId); } private UserInfo createUser(String name, int flags) { diff --git a/services/tests/servicestests/src/com/android/server/pm/UserRemovalWaiter.java b/services/tests/servicestests/src/com/android/server/pm/UserRemovalWaiter.java new file mode 100644 index 0000000000000..9e1af0ce485ea --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/pm/UserRemovalWaiter.java @@ -0,0 +1,100 @@ +/* + * Copyright (C) 2023 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. + */ + +package com.android.server.pm; + +import static org.junit.Assert.fail; + +import android.content.BroadcastReceiver; +import android.content.Context; +import android.content.Intent; +import android.content.IntentFilter; +import android.os.UserHandle; +import android.os.UserManager; +import android.util.Log; + +import java.io.Closeable; +import java.io.IOException; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +public class UserRemovalWaiter extends BroadcastReceiver implements Closeable { + + private final Context mContext; + private final UserManager mUserManager; + private final String mTag; + private final long mTimeoutMillis; + private final Map mMap = new ConcurrentHashMap<>(); + + private CountDownLatch getLatch(final int userId) { + return mMap.computeIfAbsent(userId, absentKey -> new CountDownLatch(1)); + } + + public UserRemovalWaiter(Context context, String tag, int timeoutInSeconds) { + mContext = context; + mUserManager = UserManager.get(mContext); + mTag = tag; + mTimeoutMillis = timeoutInSeconds * 1000L; + + mContext.registerReceiver(this, new IntentFilter(Intent.ACTION_USER_REMOVED)); + } + + @Override + public void close() throws IOException { + mContext.unregisterReceiver(this); + } + + @Override + public void onReceive(Context context, Intent intent) { + if (Intent.ACTION_USER_REMOVED.equals(intent.getAction())) { + int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, UserHandle.USER_NULL); + Log.i(mTag, "ACTION_USER_REMOVED received for user " + userId); + getLatch(userId).countDown(); + } + } + + /** + * Waits for the removal of the given user, or fails if it times out. + */ + public void waitFor(int userId) { + Log.i(mTag, "Waiting for user " + userId + " to be removed"); + CountDownLatch latch = getLatch(userId); + long startTime = System.currentTimeMillis(); + while (System.currentTimeMillis() - startTime < mTimeoutMillis) { + if (hasUserGone(userId) || waitLatchForOneSecond(latch)) { + Log.i(mTag, "User " + userId + " is removed in " + + (System.currentTimeMillis() - startTime) + " ms"); + return; + } + } + fail("Timeout waiting for user removal. userId = " + userId); + } + + private boolean hasUserGone(int userId) { + return mUserManager.getAliveUsers().stream().noneMatch(x -> x.id == userId); + } + + private boolean waitLatchForOneSecond(CountDownLatch latch) { + try { + return latch.await(1, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Log.e(mTag, "Thread interrupted unexpectedly.", e); + return false; + } + } +}