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
This commit is contained in:
parent
06421df25d
commit
2720a818d5
|
@ -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
|
||||
|
|
|
@ -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.");
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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<ImapConnection> mConnectionPool =
|
||||
new ConcurrentLinkedQueue<ImapConnection>();
|
||||
|
||||
/**
|
||||
* 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<String, ImapFolder> mFolderCache = new HashMap<String, ImapFolder>();
|
||||
|
||||
/**
|
||||
* 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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -934,7 +934,7 @@ public class Pop3Store extends Store {
|
|||
}
|
||||
|
||||
@Override
|
||||
public boolean isOpenForTest() {
|
||||
public boolean isOpen() {
|
||||
return mTransport.isOpen();
|
||||
}
|
||||
|
||||
|
|
|
@ -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.
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -103,7 +103,7 @@ public class MockFolder extends Folder {
|
|||
}
|
||||
|
||||
@Override
|
||||
public boolean isOpenForTest() {
|
||||
public boolean isOpen() {
|
||||
return false;
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in New Issue