ADHOC
/
replicant-packages_apps_Email
3
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Do less work on IDLE refresh. 

Don't do a full reconnection, but just a stopIdling/startIdling pair. In
order to be able to do that, make sure the IDLE connection is fully shut
down when stopIdling() returns, for which some refactoring was needed to
avoid a deadlock on mIdledFolders: the ImapIdleListener callbacks
acquire this lock, so stopIdling() now MUST NOT be called with
mIdledFolders lock held.

Change-Id: Ifa1677d7845722ccee2b1b9380c7b7e4014bcd97
This commit is contained in:
Danny Baumann 2015-06-10 14:16:34 +02:00 committed by Steve Kondik
parent 9e42c23d4c
commit cb4fda8286
4 changed files with 126 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
provider_src/com/android/email/mail/store/ImapConnection.java
View File

@ -235,6 +235,7 @@ class ImapConnection {
destroyResponses(); destroyResponses();
mParser = null; mParser = null;
mImapStore = null; mImapStore = null;
mIdling = false;
} }
int getReadTimeout() throws IOException { int getReadTimeout() throws IOException {
@ -397,13 +398,14 @@ class ImapConnection {
*/ */
List<ImapResponse> getCommandResponses() throws IOException, MessagingException { List<ImapResponse> getCommandResponses() throws IOException, MessagingException {
final List<ImapResponse> responses = new ArrayList<ImapResponse>(); final List<ImapResponse> responses = new ArrayList<ImapResponse>();
final ImapResponseParser parser = mParser; // might get reset during idling
ImapResponse response = null; ImapResponse response = null;
boolean idling = false; boolean idling = false;
boolean throwSocketTimeoutEx = true; boolean throwSocketTimeoutEx = true;
int lastSocketTimeout = getReadTimeout(); final int lastSocketTimeout = getReadTimeout();
try { try {
do { do {
response = mParser.readResponse(); response = parser.readResponse();
if (idling) { if (idling) {
setReadTimeout(IDLE_OP_READ_TIMEOUT); setReadTimeout(IDLE_OP_READ_TIMEOUT);
throwSocketTimeoutEx = false; throwSocketTimeoutEx = false;
@ -418,7 +420,7 @@ class ImapConnection {
throw ex; throw ex;
} }
} finally { } finally {
mParser.resetIdlingStatus(); parser.resetIdlingStatus();
if (lastSocketTimeout != getReadTimeout()) { if (lastSocketTimeout != getReadTimeout()) {
setReadTimeout(lastSocketTimeout); setReadTimeout(lastSocketTimeout);
} }

82
provider_src/com/android/email/mail/store/ImapFolder.java
View File

@ -234,6 +234,11 @@ public class ImapFolder extends Folder {
mDiscardIdlingConnection = false; mDiscardIdlingConnection = false;
} }
final ImapConnection connection;
synchronized (this) {
connection = mConnection;
}
// Run idle in background // Run idle in background
mIdleReader = new Thread() { mIdleReader = new Thread() {
@Override @Override
@ -244,8 +249,8 @@ public class ImapFolder extends Folder {
// We setup the max time specified in RFC 2177 to re-issue // We setup the max time specified in RFC 2177 to re-issue
// an idle request to the server // an idle request to the server
mConnection.setReadTimeout(ImapConnection.PING_IDLE_TIMEOUT); connection.setReadTimeout(ImapConnection.PING_IDLE_TIMEOUT);
mConnection.destroyResponses(); connection.destroyResponses();
// Enter now in idle status (we hold a connection with // Enter now in idle status (we hold a connection with
// the server to listen for new changes) // the server to listen for new changes)
@ -259,7 +264,7 @@ public class ImapFolder extends Folder {
if (callback != null) { if (callback != null) {
callback.onIdled(); callback.onIdled();
} }
List<ImapResponse> responses = mConnection.executeIdleCommand(); List<ImapResponse> responses = connection.executeIdleCommand();
// Check whether IDLE was successful (first response is an idling response) // Check whether IDLE was successful (first response is an idling response)
if (responses.isEmpty() || (mIdling && !responses.get(0).isIdling())) { if (responses.isEmpty() || (mIdling && !responses.get(0).isIdling())) {
@ -279,8 +284,8 @@ public class ImapFolder extends Folder {
synchronized (mIdleSync) { synchronized (mIdleSync) {
if (!mIdlingCancelled) { if (!mIdlingCancelled) {
try { try {
mConnection.setReadTimeout(ImapConnection.DONE_TIMEOUT); connection.setReadTimeout(ImapConnection.DONE_TIMEOUT);
mConnection.executeSimpleCommand(ImapConstants.DONE); connection.executeSimpleCommand(ImapConstants.DONE);
} catch (MessagingException me) { } catch (MessagingException me) {
// Ignore this exception caused by messages in the queue // Ignore this exception caused by messages in the queue
} }
@ -301,7 +306,7 @@ public class ImapFolder extends Folder {
if (discardConnection) { if (discardConnection) {
// Return the connection to the pool // Return the connection to the pool
close(false); cleanupConnection(connection, false);
} }
synchronized (mIdleSync) { synchronized (mIdleSync) {
@ -309,7 +314,7 @@ public class ImapFolder extends Folder {
} }
} catch (MessagingException me) { } catch (MessagingException me) {
close(false); cleanupConnection(connection, false);
synchronized (mIdleSync) { synchronized (mIdleSync) {
mIdling = false; mIdling = false;
} }
@ -318,7 +323,7 @@ public class ImapFolder extends Folder {
} }
} catch (SocketTimeoutException ste) { } catch (SocketTimeoutException ste) {
close(false); cleanupConnection(connection, false);
synchronized (mIdleSync) { synchronized (mIdleSync) {
mIdling = false; mIdling = false;
} }
@ -327,12 +332,13 @@ public class ImapFolder extends Folder {
} }
} catch (IOException ioe) { } catch (IOException ioe) {
close(false);
synchronized (mIdleSync) { synchronized (mIdleSync) {
mIdling = false; mIdling = false;
} }
if (callback != null) { if (callback != null) {
callback.onException(ioExceptionHandler(mConnection, ioe)); callback.onException(ioExceptionHandler(connection, ioe));
} else {
cleanupConnection(connection, false);
} }
} }
@ -347,6 +353,12 @@ public class ImapFolder extends Folder {
if (!isOpen()) { if (!isOpen()) {
throw new MessagingException("Folder " + mName + " is not open."); throw new MessagingException("Folder " + mName + " is not open.");
} }
final ImapConnection connection;
synchronized (this) {
connection = mConnection;
}
synchronized (mIdleSync) { synchronized (mIdleSync) {
if (!mIdling) { if (!mIdling) {
throw new MessagingException("Folder " + mName + " isn't in IDLE state."); throw new MessagingException("Folder " + mName + " isn't in IDLE state.");
@ -354,14 +366,15 @@ public class ImapFolder extends Folder {
try { try {
mIdlingCancelled = true; mIdlingCancelled = true;
mDiscardIdlingConnection = discardConnection; mDiscardIdlingConnection = discardConnection;
// We can read responses here because we can block the buffer. Read commands // Send the DONE command to make the idle reader thread exit. Shorten
// are always done by startListener method (blocking idle) // the read timeout for doing that in order to not wait indefinitely,
mConnection.sendCommand(ImapConstants.DONE, false); // the server should respond to the DONE command quickly anyway
connection.sendCommand(ImapConstants.DONE, false);
} catch (MessagingException me) { } catch (MessagingException me) {
// Treat IOERROR messaging exception as IOException // Treat IOERROR messaging exception as IOException
if (me.getExceptionType() == MessagingException.IOERROR) { if (me.getExceptionType() == MessagingException.IOERROR) {
close(false); cleanupConnection(connection, false);
throw me; throw me;
} }
@ -370,6 +383,24 @@ public class ImapFolder extends Folder {
} }
} }
// Try to join the thread, but make sure to not wait indefinitely. This should
// be the normal case (server sends the response to DONE quickly)
try {
mIdleReader.join(1000, 0);
} catch (InterruptedException e) {
// ignore
}
// In case the server didn't respond quickly, the connection is likely broken;
// close it (which definitely will cause the thread to return) and finally join the thread
if (mIdleReader.isAlive()) {
cleanupConnection(connection, true);
try {
mIdleReader.join();
} catch (InterruptedException e) {
// ignore
}
}
} }
public boolean isIdling() { public boolean isIdling() {
@ -606,6 +637,21 @@ public class ImapFolder extends Folder {
return allReturnStatuses; return allReturnStatuses;
} }
private void cleanupConnection(ImapConnection connection, boolean close) {
if (close) {
connection.close();
}
synchronized (this) {
if (connection == mConnection) {
if (close) {
// To prevent close() from returning the connection to the pool
mConnection = null;
}
close(false);
}
}
}
private List<String> getNewMessagesFromUid(String uid) throws MessagingException { private List<String> getNewMessagesFromUid(String uid) throws MessagingException {
checkOpen(); checkOpen();
List<String> nextMSNs = new ArrayList<>(); List<String> nextMSNs = new ArrayList<>();
@ -1526,14 +1572,10 @@ public class ImapFolder extends Folder {
private MessagingException ioExceptionHandler(ImapConnection connection, IOException ioe) { private MessagingException ioExceptionHandler(ImapConnection connection, IOException ioe) {
if (DebugUtils.DEBUG) { if (DebugUtils.DEBUG) {
LogUtils.d(Logging.LOG_TAG, "IO Exception detected: ", ioe); LogUtils.d(Logging.LOG_TAG, ioe, "IO Exception detected: ");
} }
if (connection != null) { if (connection != null) {
connection.close(); cleanupConnection(connection, true);
}
if (connection == mConnection) {
mConnection = null; // To prevent close() from returning the connection to the pool.
close(false);
} }
return new MessagingException(MessagingException.IOERROR, "IO Error", ioe); return new MessagingException(MessagingException.IOERROR, "IO Error", ioe);
} }

1
provider_src/com/android/email/mail/store/ImapStore.java
View File

@ -467,6 +467,7 @@ public class ImapStore extends Store {
return mailboxes.values().toArray(new Folder[mailboxes.size()]); return mailboxes.values().toArray(new Folder[mailboxes.size()]);
} catch (IOException ioe) { } catch (IOException ioe) {
connection.close(); connection.close();
connection = null;
throw new MessagingException("Unable to get folder list", ioe); throw new MessagingException("Unable to get folder list", ioe);
} catch (AuthenticationFailedException afe) { } catch (AuthenticationFailedException afe) {
// We do NOT want this connection pooled, or we will continue to send NOOP and SELECT // We do NOT want this connection pooled, or we will continue to send NOOP and SELECT

92
provider_src/com/android/email/service/ImapService.java
View File

@ -176,13 +176,13 @@ public class ImapService extends Service {
private static class ImapIdleListener implements ImapFolder.IdleCallback { private static class ImapIdleListener implements ImapFolder.IdleCallback {
private final Context mContext; private final Context mContext;
private final Store mStore; private final Account mAccount;
private final Mailbox mMailbox; private final Mailbox mMailbox;
public ImapIdleListener(Context context, Store store, Mailbox mailbox) { public ImapIdleListener(Context context, Account account, Mailbox mailbox) {
super(); super();
mContext = context; mContext = context;
mStore = store; mAccount = account;
mMailbox = mailbox; mMailbox = mailbox;
} }
@ -205,7 +205,7 @@ public class ImapService extends Service {
@Override @Override
public void run() { public void run() {
// Selectively process all the retrieved changes // Selectively process all the retrieved changes
processImapIdleChangesLocked(mContext, mStore.getAccount(), mMailbox, processImapIdleChangesLocked(mContext, mAccount, mMailbox,
needSync, fetchMessages); needSync, fetchMessages);
} }
}); });
@ -330,13 +330,17 @@ public class ImapService extends Service {
return sInstance; return sInstance;
} }
private boolean isMailboxIdled(long mailboxId) { private ImapFolder getIdledMailbox(long mailboxId) {
synchronized (mIdledFolders) { synchronized (mIdledFolders) {
ImapFolder folder = mIdledFolders.get((int) mailboxId); ImapFolder folder = mIdledFolders.get((int) mailboxId);
return folder != null && folder.isIdling(); return folder != null && folder.isIdling() ? folder : null;
} }
} }
private boolean isMailboxIdled(long mailboxId) {
return getIdledMailbox(mailboxId) != null;
}
private boolean registerMailboxForIdle(Context context, Account account, Mailbox mailbox) private boolean registerMailboxForIdle(Context context, Account account, Mailbox mailbox)
throws MessagingException { throws MessagingException {
synchronized (mIdledFolders) { synchronized (mIdledFolders) {
@ -372,7 +376,8 @@ public class ImapService extends Service {
mIdledFolders.put((int) mailbox.mId, folder); mIdledFolders.put((int) mailbox.mId, folder);
} }
folder.open(OpenMode.READ_WRITE); folder.open(OpenMode.READ_WRITE);
folder.startIdling(new ImapIdleListener(context, remoteStore, mailbox)); folder.startIdling(new ImapIdleListener(context,
remoteStore.getAccount(), mailbox));
LogUtils.i(LOG_TAG, "Registered idle for mailbox " + mailbox.mId); LogUtils.i(LOG_TAG, "Registered idle for mailbox " + mailbox.mId);
return true; return true;
@ -383,31 +388,32 @@ public class ImapService extends Service {
} }
} }
private void unregisterIdledMailboxLocked(long mailboxId, boolean remove) private void unregisterIdledMailbox(long mailboxId, boolean remove)
throws MessagingException { throws MessagingException {
final ImapFolder folder;
synchronized (mIdledFolders) { synchronized (mIdledFolders) {
unregisterIdledMailbox(mailboxId, remove, true); folder = unregisterIdledMailboxLocked(mailboxId, remove);
}
if (folder != null) {
folder.stopIdling(remove);
} }
} }
private void unregisterIdledMailbox(long mailboxId, boolean remove, boolean disconnect) private ImapFolder unregisterIdledMailboxLocked(long mailboxId, boolean remove)
throws MessagingException { throws MessagingException {
// Check that the folder is already registered // Check that the folder is already registered
if (!isMailboxIdled(mailboxId)) { ImapFolder folder = mIdledFolders.get((int) mailboxId);
if (folder == null || !folder.isIdling()) {
LogUtils.i(LOG_TAG, "Mailbox isn't idled yet: " + mailboxId); LogUtils.i(LOG_TAG, "Mailbox isn't idled yet: " + mailboxId);
return; return null;
} }
// Stop idling
ImapFolder folder = mIdledFolders.get((int) mailboxId);
if (disconnect) {
folder.stopIdling(remove);
}
if (remove) { if (remove) {
mIdledFolders.remove((int) mailboxId); mIdledFolders.remove((int) mailboxId);
} }
LogUtils.i(LOG_TAG, "Unregister idle for mailbox " + mailboxId); LogUtils.i(LOG_TAG, "Unregistered idle for mailbox " + mailboxId);
return folder;
} }
private void registerAccountForIdle(Context context, Account account) private void registerAccountForIdle(Context context, Account account)
@ -461,15 +467,17 @@ public class ImapService extends Service {
private void kickIdledMailbox(Context context, Mailbox mailbox, Account account) private void kickIdledMailbox(Context context, Mailbox mailbox, Account account)
throws MessagingException { throws MessagingException {
synchronized (mIdledFolders) { final ImapFolder folder = getIdledMailbox((int) mailbox.mId);
unregisterIdledMailboxLocked(mailbox.mId, true); if (folder != null) {
registerMailboxForIdle(context, account, mailbox); folder.stopIdling(false);
folder.startIdling(new ImapIdleListener(context, account, mailbox));
} }
} }
private void unregisterAccountIdledMailboxes(Context context, long accountId, private void unregisterAccountIdledMailboxes(Context context, long accountId,
boolean remove) { boolean remove) {
LogUtils.i(LOG_TAG, "Unregister idle for account " + accountId); LogUtils.i(LOG_TAG, "Unregister idle for account " + accountId);
final ArrayList<ImapFolder> foldersToStop = new ArrayList<>();
synchronized (mIdledFolders) { synchronized (mIdledFolders) {
int count = mIdledFolders.size() - 1; int count = mIdledFolders.size() - 1;
@ -478,9 +486,11 @@ public class ImapService extends Service {
try { try {
Mailbox mailbox = Mailbox.restoreMailboxWithId(context, mailboxId); Mailbox mailbox = Mailbox.restoreMailboxWithId(context, mailboxId);
if (mailbox == null || mailbox.mAccountKey == accountId) { if (mailbox == null || mailbox.mAccountKey == accountId) {
unregisterIdledMailbox(mailboxId, remove, true); ImapFolder folder = unregisterIdledMailboxLocked(mailboxId, remove);
if (folder != null) {
LogUtils.i(LOG_TAG, "Unregister idle for mailbox " + mailboxId); foldersToStop.add(folder);
LogUtils.i(LOG_TAG, "Unregister idle for mailbox " + mailboxId);
}
} }
} catch (MessagingException ex) { } catch (MessagingException ex) {
LogUtils.w(LOG_TAG, "Failed to unregister mailbox " LogUtils.w(LOG_TAG, "Failed to unregister mailbox "
@ -488,6 +498,7 @@ public class ImapService extends Service {
} }
} }
} }
stopIdlingForFolders(foldersToStop);
} }
private void unregisterAllIdledMailboxes(final boolean disconnect) { private void unregisterAllIdledMailboxes(final boolean disconnect) {
@ -495,22 +506,35 @@ public class ImapService extends Service {
sExecutor.execute(new Runnable() { sExecutor.execute(new Runnable() {
@Override @Override
public void run() { public void run() {
final ArrayList<ImapFolder> foldersToStop = new ArrayList<>();
synchronized (mIdledFolders) { synchronized (mIdledFolders) {
LogUtils.i(LOG_TAG, "Unregister all idle mailboxes"); LogUtils.i(LOG_TAG, "Unregister all idle mailboxes");
int count = mIdledFolders.size() - 1; if (disconnect) {
for (int index = count; index >= 0; index--) { int count = mIdledFolders.size();
long mailboxId = mIdledFolders.keyAt(index); for (int i = 0; i < count; i++) {
try { ImapFolder folder = mIdledFolders.get(mIdledFolders.keyAt(i));
unregisterIdledMailbox(mailboxId, true, disconnect); if (folder != null && folder.isIdling()) {
} catch (MessagingException ex) { foldersToStop.add(folder);
LogUtils.w(LOG_TAG, "Failed to unregister mailbox " + mailboxId); }
} }
} }
mIdledFolders.clear();
} }
stopIdlingForFolders(foldersToStop);
} }
}); });
} }
private void stopIdlingForFolders(final List<ImapFolder> folders) {
for (ImapFolder folder : folders) {
try {
folder.stopIdling(true);
} catch (MessagingException me) {
// ignored
}
}
}
} }
private static class ImapEmailConnectivityManager extends EmailConnectivityManager { private static class ImapEmailConnectivityManager extends EmailConnectivityManager {
@ -673,7 +697,7 @@ public class ImapService extends Service {
// For delete operations we can't fetch the mailbox, so process it first // For delete operations we can't fetch the mailbox, so process it first
if (op.equals(EmailProvider.NOTIFICATION_OP_DELETE)) { if (op.equals(EmailProvider.NOTIFICATION_OP_DELETE)) {
try { try {
ImapIdleFolderHolder.getInstance().unregisterIdledMailboxLocked(id, true); ImapIdleFolderHolder.getInstance().unregisterIdledMailbox(id, true);
} catch (MessagingException me) { } catch (MessagingException me) {
LogUtils.e(LOG_TAG, me, "Failed to process imap mailbox " + id + " changes."); LogUtils.e(LOG_TAG, me, "Failed to process imap mailbox " + id + " changes.");
} }
@ -702,7 +726,7 @@ public class ImapService extends Service {
&& account.getSyncInterval() == Account.CHECK_INTERVAL_PUSH; && account.getSyncInterval() == Account.CHECK_INTERVAL_PUSH;
if (registered != toRegister) { if (registered != toRegister) {
if (registered) { if (registered) {
holder.unregisterIdledMailboxLocked(id, true); holder.unregisterIdledMailbox(id, true);
} }
if (toRegister) { if (toRegister) {
holder.registerMailboxForIdle(mContext, account, mailbox); holder.registerMailboxForIdle(mContext, account, mailbox);
@ -1084,7 +1108,7 @@ public class ImapService extends Service {
// Unregister the imap idle // Unregister the imap idle
if (account.getSyncInterval() == Account.CHECK_INTERVAL_PUSH) { if (account.getSyncInterval() == Account.CHECK_INTERVAL_PUSH) {
imapHolder.unregisterIdledMailboxLocked(folder.mId, false); imapHolder.unregisterIdledMailbox(folder.mId, false);
} else { } else {
imapHolder.unregisterAccountIdledMailboxes(context, account.mId, false); imapHolder.unregisterAccountIdledMailboxes(context, account.mId, false);
} }