From 9ac40f1cf1125c6144e92518a9d44b0eea2ef7b7 Mon Sep 17 00:00:00 2001 From: Fyodor Kupolov Date: Tue, 28 Mar 2017 19:11:17 -0700 Subject: [PATCH] Only use cacheLock when it's needed When reading from cache, we can avoid synchronization on dbLock if we only read from cache (no db access). When doing updates to db and cache, we should hold cacheLock only when updating the cache. This change improves locking in the following methods: - getAccountVisibilityFromCache - saveAuthTokenToDatabase - getAccountsFromCacheLocked no longer allows outside locking. The method was renamed to getAccountsFromCache and now self-manages locks - writeAuthTokenIntoCacheLocked - readAuthTokenInternal Test: AccountManagerServiceTest Bug: 36485175 Bug: 35262596 Change-Id: I9aca45c31716c4f0e0fd9f07859e88a7f5ba6922 --- .../accounts/AccountManagerService.java | 284 +++++++++--------- 1 file changed, 140 insertions(+), 144 deletions(-) diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java index 79be3e6cfa131..0ccaf8e914b8c 100644 --- a/services/core/java/com/android/server/accounts/AccountManagerService.java +++ b/services/core/java/com/android/server/accounts/AccountManagerService.java @@ -114,7 +114,6 @@ import java.util.Date; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -201,7 +200,7 @@ public class AccountManagerService private final HashMap signinRequiredNotificationIds = new HashMap(); final Object cacheLock = new Object(); - final Object dbLock = new Object(); + final Object dbLock = new Object(); // if needed, dbLock must be obtained before cacheLock /** protected by the {@link #cacheLock} */ final HashMap accountCache = new LinkedHashMap<>(); /** protected by the {@link #cacheLock} */ @@ -586,13 +585,11 @@ public class AccountManagerService */ private int getAccountVisibilityFromCache(Account account, String packageName, UserAccounts accounts) { - synchronized (accounts.dbLock) { - synchronized (accounts.cacheLock) { - Map accountVisibility = - getPackagesAndVisibilityForAccountLocked(account, accounts); - Integer visibility = accountVisibility.get(packageName); - return visibility != null ? visibility : AccountManager.VISIBILITY_UNDEFINED; - } + synchronized (accounts.cacheLock) { + Map accountVisibility = + getPackagesAndVisibilityForAccountLocked(account, accounts); + Integer visibility = accountVisibility.get(packageName); + return visibility != null ? visibility : AccountManager.VISIBILITY_UNDEFINED; } } @@ -2240,16 +2237,23 @@ public class AccountManagerService long identityToken = clearCallingIdentity(); try { UserAccounts accounts = getUserAccounts(userId); + List> deletedTokens; synchronized (accounts.dbLock) { + accounts.accountsDb.beginTransaction(); + try { + deletedTokens = invalidateAuthTokenLocked(accounts, accountType, authToken); + accounts.accountsDb.setTransactionSuccessful(); + } finally { + accounts.accountsDb.endTransaction(); + } synchronized (accounts.cacheLock) { - accounts.accountsDb.beginTransaction(); - try { - invalidateAuthTokenLocked(accounts, accountType, authToken); - invalidateCustomTokenLocked(accounts, accountType, authToken); - accounts.accountsDb.setTransactionSuccessful(); - } finally { - accounts.accountsDb.endTransaction(); + for (Pair tokenInfo : deletedTokens) { + Account act = tokenInfo.first; + String tokenType = tokenInfo.second; + writeAuthTokenIntoCacheLocked(accounts, act, tokenType, null); } + // wipe out cached token in memory. + accounts.accountTokenCaches.remove(accountType, authToken); } } } finally { @@ -2257,38 +2261,24 @@ public class AccountManagerService } } - private void invalidateCustomTokenLocked( - UserAccounts accounts, - String accountType, + private List> invalidateAuthTokenLocked(UserAccounts accounts, String accountType, String authToken) { - if (authToken == null || accountType == null) { - return; - } - // Also wipe out cached token in memory. - accounts.accountTokenCaches.remove(accountType, authToken); - } - - private void invalidateAuthTokenLocked(UserAccounts accounts, String accountType, - String authToken) { - if (authToken == null || accountType == null) { - return; - } + // TODO Move to AccountsDB + List> results = new ArrayList<>(); Cursor cursor = accounts.accountsDb.findAuthtokenForAllAccounts(accountType, authToken); + try { while (cursor.moveToNext()) { String authTokenId = cursor.getString(0); String accountName = cursor.getString(1); String authTokenType = cursor.getString(2); accounts.accountsDb.deleteAuthToken(authTokenId); - writeAuthTokenIntoCacheLocked( - accounts, - new Account(accountName, accountType), - authTokenType, - null); + results.add(Pair.create(new Account(accountName, accountType), authTokenType)); } } finally { cursor.close(); } + return results; } private void saveCachedToken( @@ -2305,11 +2295,9 @@ public class AccountManagerService } cancelNotification(getSigninRequiredNotificationId(accounts, account), UserHandle.of(accounts.userId)); - synchronized (accounts.dbLock) { - synchronized (accounts.cacheLock) { - accounts.accountTokenCaches.put( - account, token, tokenType, callerPkg, callerSigDigest, expiryMillis); - } + synchronized (accounts.cacheLock) { + accounts.accountTokenCaches.put( + account, token, tokenType, callerPkg, callerSigDigest, expiryMillis); } } @@ -2321,22 +2309,26 @@ public class AccountManagerService cancelNotification(getSigninRequiredNotificationId(accounts, account), UserHandle.of(accounts.userId)); synchronized (accounts.dbLock) { - synchronized (accounts.cacheLock) { - accounts.accountsDb.beginTransaction(); - try { - long accountId = accounts.accountsDb.findDeAccountId(account); - if (accountId < 0) { - return false; - } - accounts.accountsDb.deleteAuthtokensByAccountIdAndType(accountId, type); - if (accounts.accountsDb.insertAuthToken(accountId, type, authToken) >= 0) { - accounts.accountsDb.setTransactionSuccessful(); - writeAuthTokenIntoCacheLocked(accounts, account, type, authToken); - return true; - } + accounts.accountsDb.beginTransaction(); + boolean updateCache = false; + try { + long accountId = accounts.accountsDb.findDeAccountId(account); + if (accountId < 0) { return false; - } finally { - accounts.accountsDb.endTransaction(); + } + accounts.accountsDb.deleteAuthtokensByAccountIdAndType(accountId, type); + if (accounts.accountsDb.insertAuthToken(accountId, type, authToken) >= 0) { + accounts.accountsDb.setTransactionSuccessful(); + updateCache = true; + return true; + } + return false; + } finally { + accounts.accountsDb.endTransaction(); + if (updateCache) { + synchronized (accounts.cacheLock) { + writeAuthTokenIntoCacheLocked(accounts, account, type, authToken); + } } } } @@ -3947,13 +3939,8 @@ public class AccountManagerService @Override public void run() throws RemoteException { - synchronized (mAccounts.dbLock) { - synchronized (mAccounts.cacheLock) { - mAccountsOfType = getAccountsFromCacheLocked(mAccounts, mAccountType, - mCallingUid, - mPackageName, false /* include managed not visible*/); - } - } + mAccountsOfType = getAccountsFromCache(mAccounts, mAccountType, + mCallingUid, mPackageName, false /* include managed not visible*/); // check whether each account matches the requested features mAccountsWithFeatures = new ArrayList<>(mAccountsOfType.length); mCurrentAccount = 0; @@ -4097,18 +4084,14 @@ public class AccountManagerService for (int userId : userIds) { UserAccounts userAccounts = getUserAccounts(userId); if (userAccounts == null) continue; - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - Account[] accounts = getAccountsFromCacheLocked( - userAccounts, - null /* type */, - Binder.getCallingUid(), - null /* packageName */, - false /* include managed not visible*/); - for (int a = 0; a < accounts.length; a++) { - runningAccounts.add(new AccountAndUser(accounts[a], userId)); - } - } + Account[] accounts = getAccountsFromCache( + userAccounts, + null /* type */, + Binder.getCallingUid(), + null /* packageName */, + false /* include managed not visible*/); + for (Account account : accounts) { + runningAccounts.add(new AccountAndUser(account, userId)); } } @@ -4194,24 +4177,20 @@ public class AccountManagerService String callingPackage, List visibleAccountTypes, boolean includeUserManagedNotVisible) { - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - ArrayList visibleAccounts = new ArrayList<>(); - for (String visibleType : visibleAccountTypes) { - Account[] accountsForType = getAccountsFromCacheLocked( - userAccounts, visibleType, callingUid, callingPackage, - includeUserManagedNotVisible); - if (accountsForType != null) { - visibleAccounts.addAll(Arrays.asList(accountsForType)); - } - } - Account[] result = new Account[visibleAccounts.size()]; - for (int i = 0; i < visibleAccounts.size(); i++) { - result[i] = visibleAccounts.get(i); - } - return result; + ArrayList visibleAccounts = new ArrayList<>(); + for (String visibleType : visibleAccountTypes) { + Account[] accountsForType = getAccountsFromCache( + userAccounts, visibleType, callingUid, callingPackage, + includeUserManagedNotVisible); + if (accountsForType != null) { + visibleAccounts.addAll(Arrays.asList(accountsForType)); } } + Account[] result = new Account[visibleAccounts.size()]; + for (int i = 0; i < visibleAccounts.size(); i++) { + result[i] = visibleAccounts.get(i); + } + return result; } @Override @@ -4277,10 +4256,12 @@ public class AccountManagerService public Account[] getSharedAccountsAsUser(int userId) { userId = handleIncomingUser(userId); UserAccounts accounts = getUserAccounts(userId); - List accountList = accounts.accountsDb.getSharedAccounts(); - Account[] accountArray = new Account[accountList.size()]; - accountList.toArray(accountArray); - return accountArray; + synchronized (accounts.dbLock) { + List accountList = accounts.accountsDb.getSharedAccounts(); + Account[] accountArray = new Account[accountList.size()]; + accountList.toArray(accountArray); + return accountArray; + } } @Override @@ -4360,13 +4341,8 @@ public class AccountManagerService try { UserAccounts userAccounts = getUserAccounts(userId); if (features == null || features.length == 0) { - Account[] accounts; - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - accounts = getAccountsFromCacheLocked( - userAccounts, type, callingUid, opPackageName, false); - } - } + Account[] accounts = getAccountsFromCache(userAccounts, type, callingUid, + opPackageName, false); Bundle result = new Bundle(); result.putParcelableArray(AccountManager.KEY_ACCOUNTS, accounts); onResult(response, result); @@ -4922,36 +4898,36 @@ public class AccountManagerService private void dumpUser(UserAccounts userAccounts, FileDescriptor fd, PrintWriter fout, String[] args, boolean isCheckinRequest) { - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - if (isCheckinRequest) { - // This is a checkin request. *Only* upload the account types and the count of - // each. - userAccounts.accountsDb.dumpDeAccountsTable(fout); - } else { - Account[] accounts = getAccountsFromCacheLocked(userAccounts, null /* type */, - Process.SYSTEM_UID, null /* packageName */, false); - fout.println("Accounts: " + accounts.length); - for (Account account : accounts) { - fout.println(" " + account); - } + if (isCheckinRequest) { + // This is a checkin request. *Only* upload the account types and the count of + // each. + synchronized (userAccounts.dbLock) { + userAccounts.accountsDb.dumpDeAccountsTable(fout); + } + } else { + Account[] accounts = getAccountsFromCache(userAccounts, null /* type */, + Process.SYSTEM_UID, null /* packageName */, false); + fout.println("Accounts: " + accounts.length); + for (Account account : accounts) { + fout.println(" " + account); + } - // Add debug information. - fout.println(); - userAccounts.accountsDb.dumpDebugTable(fout); - fout.println(); - synchronized (mSessions) { - final long now = SystemClock.elapsedRealtime(); - fout.println("Active Sessions: " + mSessions.size()); - for (Session session : mSessions.values()) { - fout.println(" " + session.toDebugString(now)); - } - } - - fout.println(); - mAuthenticatorCache.dump(fd, fout, args, userAccounts.userId); + // Add debug information. + fout.println(); + synchronized (userAccounts.dbLock) { + userAccounts.accountsDb.dumpDebugTable(fout); + } + fout.println(); + synchronized (mSessions) { + final long now = SystemClock.elapsedRealtime(); + fout.println("Active Sessions: " + mSessions.size()); + for (Session session : mSessions.values()) { + fout.println(" " + session.toDebugString(now)); } } + + fout.println(); + mAuthenticatorCache.dump(fd, fout, args, userAccounts.userId); } } @@ -5609,12 +5585,20 @@ public class AccountManagerService /* * packageName can be null. If not null, it should be used to filter out restricted accounts * that the package is not allowed to access. + * + *

The method shouldn't be called with UserAccounts#cacheLock held, otherwise it will cause a + * deadlock */ @NonNull - protected Account[] getAccountsFromCacheLocked(UserAccounts userAccounts, String accountType, + protected Account[] getAccountsFromCache(UserAccounts userAccounts, String accountType, int callingUid, @Nullable String callingPackage, boolean includeManagedNotVisible) { + Preconditions.checkState(!Thread.holdsLock(userAccounts.cacheLock), + "Method should not be called with cacheLock"); if (accountType != null) { - final Account[] accounts = userAccounts.accountCache.get(accountType); + Account[] accounts; + synchronized (userAccounts.cacheLock) { + accounts = userAccounts.accountCache.get(accountType); + } if (accounts == null) { return EMPTY_ACCOUNT_ARRAY; } else { @@ -5623,20 +5607,23 @@ public class AccountManagerService } } else { int totalLength = 0; - for (Account[] accounts : userAccounts.accountCache.values()) { - totalLength += accounts.length; + Account[] accountsArray; + synchronized (userAccounts.cacheLock) { + for (Account[] accounts : userAccounts.accountCache.values()) { + totalLength += accounts.length; + } + if (totalLength == 0) { + return EMPTY_ACCOUNT_ARRAY; + } + accountsArray = new Account[totalLength]; + totalLength = 0; + for (Account[] accountsOfType : userAccounts.accountCache.values()) { + System.arraycopy(accountsOfType, 0, accountsArray, totalLength, + accountsOfType.length); + totalLength += accountsOfType.length; + } } - if (totalLength == 0) { - return EMPTY_ACCOUNT_ARRAY; - } - Account[] accounts = new Account[totalLength]; - totalLength = 0; - for (Account[] accountsOfType : userAccounts.accountCache.values()) { - System.arraycopy(accountsOfType, 0, accounts, totalLength, - accountsOfType.length); - totalLength += accountsOfType.length; - } - return filterAccounts(userAccounts, accounts, callingUid, callingPackage, + return filterAccounts(userAccounts, accountsArray, callingUid, callingPackage, includeManagedNotVisible); } } @@ -5669,6 +5656,7 @@ public class AccountManagerService } } + /** protected by the {@code dbLock}, {@code cacheLock} */ protected void writeAuthTokenIntoCacheLocked(UserAccounts accounts, Account account, String key, String value) { Map authTokensForAccount = accounts.authTokenCache.get(account); @@ -5685,6 +5673,14 @@ public class AccountManagerService protected String readAuthTokenInternal(UserAccounts accounts, Account account, String authTokenType) { + // Fast path - check if account is already cached + synchronized (accounts.cacheLock) { + Map authTokensForAccount = accounts.authTokenCache.get(account); + if (authTokensForAccount != null) { + return authTokensForAccount.get(authTokenType); + } + } + // If not cached yet - do slow path and sync with db if necessary synchronized (accounts.dbLock) { synchronized (accounts.cacheLock) { Map authTokensForAccount = accounts.authTokenCache.get(account);