From 2720a818d5de169734434b114adfdf824a485f55 Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Wed, 29 Jun 2011 11:41:28 -0700 Subject: [PATCH] Don't cache ImapFolders * ImapFolder is currently very unsafe for use by multiple threads, causing, among other things, the referenced bug * Since ImapFolder is very lightweight, there's no particularly good reason to be caching them anyway * Rename isOpenForTest to isOpen Bug: 4972084 Change-Id: I2bf17b9cfc8549a222e991f3e59abfd00a4d3afd --- .../com/android/emailcommon/mail/Folder.java | 2 +- .../android/email/mail/store/ImapFolder.java | 8 +++-- .../android/email/mail/store/ImapStore.java | 34 +++++-------------- .../android/email/mail/store/Pop3Store.java | 2 +- .../email/mail/store/ImapStoreUnitTests.java | 4 +-- .../email/mail/store/Pop3StoreUnitTests.java | 16 ++++----- .../android/emailcommon/mail/MockFolder.java | 2 +- 7 files changed, 27 insertions(+), 41 deletions(-) diff --git a/emailcommon/src/com/android/emailcommon/mail/Folder.java b/emailcommon/src/com/android/emailcommon/mail/Folder.java index 9bdbdd44a..0faf9d19c 100644 --- a/emailcommon/src/com/android/emailcommon/mail/Folder.java +++ b/emailcommon/src/com/android/emailcommon/mail/Folder.java @@ -76,7 +76,7 @@ public abstract class Folder { * @return True if further commands are not expected to have to open the * connection. */ - public abstract boolean isOpenForTest(); + public abstract boolean isOpen(); /** * Returns the mode the folder was opened with. This may be different than the mode the open diff --git a/src/com/android/email/mail/store/ImapFolder.java b/src/com/android/email/mail/store/ImapFolder.java index 06ed1a5fb..c5ceb94d4 100644 --- a/src/com/android/email/mail/store/ImapFolder.java +++ b/src/com/android/email/mail/store/ImapFolder.java @@ -49,6 +49,7 @@ import com.android.emailcommon.mail.Part; import com.android.emailcommon.provider.Mailbox; import com.android.emailcommon.service.SearchParams; import com.android.emailcommon.utility.Utility; +import com.google.common.annotations.VisibleForTesting; import java.io.IOException; import java.io.InputStream; @@ -90,7 +91,7 @@ class ImapFolder extends Folder { public void open(OpenMode mode, PersistentDataCallbacks callbacks) throws MessagingException { try { - if (isOpenForTest()) { + if (isOpen()) { if (mMode == mode) { // Make sure the connection is valid. // If it's not we'll close it down and continue on to get a new one. @@ -140,7 +141,8 @@ class ImapFolder extends Folder { } @Override - public boolean isOpenForTest() { + @VisibleForTesting + public boolean isOpen() { return mExists && mConnection != null; } @@ -1059,7 +1061,7 @@ class ImapFolder extends Folder { } private void checkOpen() throws MessagingException { - if (!isOpenForTest()) { + if (!isOpen()) { throw new MessagingException("Folder " + mName + " is not open."); } } diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java index 9102f41eb..b5f79788c 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/src/com/android/email/mail/store/ImapStore.java @@ -16,6 +16,14 @@ package com.android.email.mail.store; +import android.content.Context; +import android.os.Build; +import android.os.Bundle; +import android.telephony.TelephonyManager; +import android.text.TextUtils; +import android.util.Base64; +import android.util.Log; + import com.android.email.LegacyConversions; import com.android.email.Preferences; import com.android.email.VendorPolicyLoader; @@ -40,14 +48,6 @@ import com.android.emailcommon.utility.Utility; import com.beetstra.jutf7.CharsetProvider; import com.google.common.annotations.VisibleForTesting; -import android.content.Context; -import android.os.Build; -import android.os.Bundle; -import android.telephony.TelephonyManager; -import android.text.TextUtils; -import android.util.Base64; -import android.util.Log; - import java.io.IOException; import java.io.InputStream; import java.nio.ByteBuffer; @@ -90,14 +90,6 @@ public class ImapStore extends Store { private final ConcurrentLinkedQueue mConnectionPool = new ConcurrentLinkedQueue(); - /** - * Cache of ImapFolder objects. ImapFolders are attached to a given folder on the server - * and as long as their associated connection remains open they are reusable between - * requests. This cache lets us make sure we always reuse, if possible, for a given - * folder name. - */ - private final HashMap mFolderCache = new HashMap(); - /** * Static named constructor. */ @@ -321,15 +313,7 @@ public class ImapStore extends Store { @Override public Folder getFolder(String name) { - ImapFolder folder; - synchronized (mFolderCache) { - folder = mFolderCache.get(name); - if (folder == null) { - folder = new ImapFolder(this, name); - mFolderCache.put(name, folder); - } - } - return folder; + return new ImapFolder(this, name); } /** diff --git a/src/com/android/email/mail/store/Pop3Store.java b/src/com/android/email/mail/store/Pop3Store.java index 45203a4df..cb923cf25 100644 --- a/src/com/android/email/mail/store/Pop3Store.java +++ b/src/com/android/email/mail/store/Pop3Store.java @@ -934,7 +934,7 @@ public class Pop3Store extends Store { } @Override - public boolean isOpenForTest() { + public boolean isOpen() { return mTransport.isOpen(); } diff --git a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java index 4ed789d3f..07c69dfe0 100644 --- a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java +++ b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java @@ -1479,9 +1479,9 @@ public class ImapStoreUnitTests extends InstrumentationTestCase { assertEquals(1, folder.getMessageCount()); assertEquals(OpenMode.READ_WRITE, folder.getMode()); - assertTrue(folder.isOpenForTest()); + assertTrue(folder.isOpen()); folder.close(false); - assertFalse(folder.isOpenForTest()); + assertFalse(folder.isOpen()); // READ-ONLY expectNoop(mock, true); // Need it because we reuse the connection. diff --git a/tests/src/com/android/email/mail/store/Pop3StoreUnitTests.java b/tests/src/com/android/email/mail/store/Pop3StoreUnitTests.java index 3823d8fc5..601a41421 100644 --- a/tests/src/com/android/email/mail/store/Pop3StoreUnitTests.java +++ b/tests/src/com/android/email/mail/store/Pop3StoreUnitTests.java @@ -403,7 +403,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // NOTE: everything from here down is copied from testOneUnread() and should be consolidated // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); @@ -445,7 +445,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // NOTE: everything from here down is copied from testOneUnread() and should be consolidated // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); @@ -488,7 +488,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // NOTE: everything from here down is copied from testOneUnread() and should be consolidated // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); @@ -540,7 +540,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // NOTE: everything from here down is copied from testOneUnread() and should be consolidated // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); @@ -587,7 +587,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // NOTE: everything from here down is copied from testOneUnread() and should be consolidated // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); @@ -646,7 +646,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // NOTE: everything from here down is copied from testOneUnread() and should be consolidated // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); @@ -693,7 +693,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // NOTE: everything from here down is copied from testOneUnread() and should be consolidated // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); @@ -742,7 +742,7 @@ public class Pop3StoreUnitTests extends InstrumentationTestCase { // test is, can we recover? So I'll try a new connection, without the failure. // confirm that we're closed at this point - assertFalse("folder should be 'closed' after an IOError", mFolder.isOpenForTest()); + assertFalse("folder should be 'closed' after an IOError", mFolder.isOpen()); // and confirm that the next connection will be OK checkOneUnread(mockTransport); diff --git a/tests/src/com/android/emailcommon/mail/MockFolder.java b/tests/src/com/android/emailcommon/mail/MockFolder.java index cf139ddeb..ca2fe56ce 100644 --- a/tests/src/com/android/emailcommon/mail/MockFolder.java +++ b/tests/src/com/android/emailcommon/mail/MockFolder.java @@ -103,7 +103,7 @@ public class MockFolder extends Folder { } @Override - public boolean isOpenForTest() { + public boolean isOpen() { return false; }