From 2fabab32b90a7a01b72ec69cee48192bd880aab0 Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Fri, 21 Jan 2011 17:31:46 -0800 Subject: [PATCH] Don't require sSyncLock for sync error map * Use ConcurrentHashMap; check that this provides enough protection for all uses * This resolves known deadlock issues in ExchangeService Bug: 3371039 Change-Id: Ie4ccbe7cdfe8c3d4ec7a0f789409126c8c09f8c4 --- src/com/android/exchange/ExchangeService.java | 83 ++++++++----------- .../exchange/ExchangeServiceAccountTests.java | 8 +- 2 files changed, 40 insertions(+), 51 deletions(-) diff --git a/src/com/android/exchange/ExchangeService.java b/src/com/android/exchange/ExchangeService.java index 594561785..0ce8dfcd0 100644 --- a/src/com/android/exchange/ExchangeService.java +++ b/src/com/android/exchange/ExchangeService.java @@ -93,6 +93,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; +import java.util.concurrent.ConcurrentHashMap; /** * The ExchangeService handles all aspects of starting, maintaining, and stopping the various sync @@ -193,7 +194,8 @@ public class ExchangeService extends Service implements Runnable { private HashMap mServiceMap = new HashMap(); // Keeps track of services whose last sync ended with an error (by mailbox id) - /*package*/ HashMap mSyncErrorMap = new HashMap(); + /*package*/ ConcurrentHashMap mSyncErrorMap = + new ConcurrentHashMap(); // Keeps track of which services require a wake lock (by mailbox id) private HashMap mWakeLocks = new HashMap(); // Keeps track of PendingIntents for mailbox alarms (by mailbox id) @@ -394,25 +396,19 @@ public class ExchangeService extends Service implements Runnable { public void hostChanged(long accountId) throws RemoteException { ExchangeService exchangeService = INSTANCE; if (exchangeService == null) return; - synchronized (sSyncLock) { - HashMap syncErrorMap = exchangeService.mSyncErrorMap; - ArrayList deletedMailboxes = new ArrayList(); - // Go through the various error mailboxes - for (long mailboxId: syncErrorMap.keySet()) { - SyncError error = syncErrorMap.get(mailboxId); - // If it's a login failure, look a little harder - Mailbox m = Mailbox.restoreMailboxWithId(exchangeService, mailboxId); - // If it's for the account whose host has changed, clear the error - // If the mailbox is no longer around, remove the entry in the map - if (m == null) { - deletedMailboxes.add(mailboxId); - } else if (m.mAccountKey == accountId) { - error.fatal = false; - error.holdEndTime = 0; - } - } - for (long mailboxId: deletedMailboxes) { + ConcurrentHashMap syncErrorMap = exchangeService.mSyncErrorMap; + // Go through the various error mailboxes + for (long mailboxId: syncErrorMap.keySet()) { + SyncError error = syncErrorMap.get(mailboxId); + // If it's a login failure, look a little harder + Mailbox m = Mailbox.restoreMailboxWithId(exchangeService, mailboxId); + // If it's for the account whose host has changed, clear the error + // If the mailbox is no longer around, remove the entry in the map + if (m == null) { syncErrorMap.remove(mailboxId); + } else if (error != null && m.mAccountKey == accountId) { + error.fatal = false; + error.holdEndTime = 0; } } // Stop any running syncs @@ -967,16 +963,16 @@ public class ExchangeService extends Service implements Runnable { } private void logSyncHolds() { - if (Eas.USER_LOG && !mSyncErrorMap.isEmpty()) { + if (Eas.USER_LOG) { log("Sync holds:"); long time = System.currentTimeMillis(); - synchronized (sSyncLock) { - for (long mailboxId : mSyncErrorMap.keySet()) { - Mailbox m = Mailbox.restoreMailboxWithId(this, mailboxId); - if (m == null) { - log("Mailbox " + mailboxId + " no longer exists"); - } else { - SyncError error = mSyncErrorMap.get(mailboxId); + for (long mailboxId : mSyncErrorMap.keySet()) { + Mailbox m = Mailbox.restoreMailboxWithId(this, mailboxId); + if (m == null) { + log("Mailbox " + mailboxId + " no longer exists"); + } else { + SyncError error = mSyncErrorMap.get(mailboxId); + if (error != null) { log("Mailbox " + m.mDisplayName + ", error = " + error.reason + ", fatal = " + error.fatal); if (error.holdEndTime > 0) { @@ -1014,29 +1010,23 @@ public class ExchangeService extends Service implements Runnable { } private boolean releaseSyncHoldsImpl(Context context, int reason, Account account) { - synchronized(sSyncLock) { - boolean holdWasReleased = false; - ArrayList releaseList = new ArrayList(); - for (long mailboxId: mSyncErrorMap.keySet()) { - if (account != null) { - Mailbox m = Mailbox.restoreMailboxWithId(context, mailboxId); - if (m == null) { - releaseList.add(mailboxId); - } else if (m.mAccountKey != account.mId) { - continue; - } - } - SyncError error = mSyncErrorMap.get(mailboxId); - if (error.reason == reason) { - releaseList.add(mailboxId); + boolean holdWasReleased = false; + for (long mailboxId: mSyncErrorMap.keySet()) { + if (account != null) { + Mailbox m = Mailbox.restoreMailboxWithId(context, mailboxId); + if (m == null) { + mSyncErrorMap.remove(mailboxId); + } else if (m.mAccountKey != account.mId) { + continue; } } - for (long mailboxId: releaseList) { + SyncError error = mSyncErrorMap.get(mailboxId); + if (error != null && error.reason == reason) { mSyncErrorMap.remove(mailboxId); holdWasReleased = true; } - return holdWasReleased; } + return holdWasReleased; } public class EasSyncStatusObserver implements SyncStatusObserver { @@ -2371,8 +2361,7 @@ public class ExchangeService extends Service implements Runnable { */ static public void removeFromSyncErrorMap(long mailboxId) { ExchangeService exchangeService = INSTANCE; - if (exchangeService == null) return; - synchronized(sSyncLock) { + if (exchangeService != null) { exchangeService.mSyncErrorMap.remove(mailboxId); } } @@ -2388,7 +2377,7 @@ public class ExchangeService extends Service implements Runnable { if (exchangeService == null) return; synchronized(sSyncLock) { long mailboxId = svc.mMailboxId; - HashMap errorMap = exchangeService.mSyncErrorMap; + ConcurrentHashMap errorMap = exchangeService.mSyncErrorMap; SyncError syncError = errorMap.get(mailboxId); exchangeService.releaseMailbox(mailboxId); int exitStatus = svc.mExitStatus; diff --git a/tests/src/com/android/exchange/ExchangeServiceAccountTests.java b/tests/src/com/android/exchange/ExchangeServiceAccountTests.java index 9b9272695..990b9222f 100644 --- a/tests/src/com/android/exchange/ExchangeServiceAccountTests.java +++ b/tests/src/com/android/exchange/ExchangeServiceAccountTests.java @@ -18,15 +18,15 @@ package com.android.exchange; import com.android.email.AccountTestCase; -import com.android.email.provider.EmailProvider; -import com.android.email.provider.ProviderTestUtils; import com.android.email.provider.EmailContent.Account; import com.android.email.provider.EmailContent.Mailbox; +import com.android.email.provider.EmailProvider; +import com.android.email.provider.ProviderTestUtils; import com.android.exchange.ExchangeService.SyncError; import android.content.Context; -import java.util.HashMap; +import java.util.concurrent.ConcurrentHashMap; /** * You can run this entire test case with: @@ -64,7 +64,7 @@ public class ExchangeServiceAccountTests extends AccountTestCase { Mailbox box3 = ProviderTestUtils.setupMailbox("box3", acct2.mId, true, context); Mailbox box4 = ProviderTestUtils.setupMailbox("box4", acct2.mId, true, context); - HashMap errorMap = exchangeService.mSyncErrorMap; + ConcurrentHashMap errorMap = exchangeService.mSyncErrorMap; // Add errors into the map errorMap.put(box1.mId, securityErrorAccount1); errorMap.put(box2.mId, ioError);