From cb4fda8286fae2fa83c2fe611a1c34da5d518ed4 Mon Sep 17 00:00:00 2001 From: Danny Baumann Date: Wed, 10 Jun 2015 14:16:34 +0200 Subject: [PATCH] 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 --- .../email/mail/store/ImapConnection.java | 8 +- .../android/email/mail/store/ImapFolder.java | 82 +++++++++++++---- .../android/email/mail/store/ImapStore.java | 1 + .../android/email/service/ImapService.java | 92 ++++++++++++------- 4 files changed, 126 insertions(+), 57 deletions(-) diff --git a/provider_src/com/android/email/mail/store/ImapConnection.java b/provider_src/com/android/email/mail/store/ImapConnection.java index 3e5eb2a8a..7bb604e02 100644 --- a/provider_src/com/android/email/mail/store/ImapConnection.java +++ b/provider_src/com/android/email/mail/store/ImapConnection.java @@ -235,6 +235,7 @@ class ImapConnection { destroyResponses(); mParser = null; mImapStore = null; + mIdling = false; } int getReadTimeout() throws IOException { @@ -397,13 +398,14 @@ class ImapConnection { */ List getCommandResponses() throws IOException, MessagingException { final List responses = new ArrayList(); + final ImapResponseParser parser = mParser; // might get reset during idling ImapResponse response = null; boolean idling = false; boolean throwSocketTimeoutEx = true; - int lastSocketTimeout = getReadTimeout(); + final int lastSocketTimeout = getReadTimeout(); try { do { - response = mParser.readResponse(); + response = parser.readResponse(); if (idling) { setReadTimeout(IDLE_OP_READ_TIMEOUT); throwSocketTimeoutEx = false; @@ -418,7 +420,7 @@ class ImapConnection { throw ex; } } finally { - mParser.resetIdlingStatus(); + parser.resetIdlingStatus(); if (lastSocketTimeout != getReadTimeout()) { setReadTimeout(lastSocketTimeout); } diff --git a/provider_src/com/android/email/mail/store/ImapFolder.java b/provider_src/com/android/email/mail/store/ImapFolder.java index 97dca901d..05f44c477 100644 --- a/provider_src/com/android/email/mail/store/ImapFolder.java +++ b/provider_src/com/android/email/mail/store/ImapFolder.java @@ -234,6 +234,11 @@ public class ImapFolder extends Folder { mDiscardIdlingConnection = false; } + final ImapConnection connection; + synchronized (this) { + connection = mConnection; + } + // Run idle in background mIdleReader = new Thread() { @Override @@ -244,8 +249,8 @@ public class ImapFolder extends Folder { // We setup the max time specified in RFC 2177 to re-issue // an idle request to the server - mConnection.setReadTimeout(ImapConnection.PING_IDLE_TIMEOUT); - mConnection.destroyResponses(); + connection.setReadTimeout(ImapConnection.PING_IDLE_TIMEOUT); + connection.destroyResponses(); // Enter now in idle status (we hold a connection with // the server to listen for new changes) @@ -259,7 +264,7 @@ public class ImapFolder extends Folder { if (callback != null) { callback.onIdled(); } - List responses = mConnection.executeIdleCommand(); + List responses = connection.executeIdleCommand(); // Check whether IDLE was successful (first response is an idling response) if (responses.isEmpty() || (mIdling && !responses.get(0).isIdling())) { @@ -279,8 +284,8 @@ public class ImapFolder extends Folder { synchronized (mIdleSync) { if (!mIdlingCancelled) { try { - mConnection.setReadTimeout(ImapConnection.DONE_TIMEOUT); - mConnection.executeSimpleCommand(ImapConstants.DONE); + connection.setReadTimeout(ImapConnection.DONE_TIMEOUT); + connection.executeSimpleCommand(ImapConstants.DONE); } catch (MessagingException me) { // Ignore this exception caused by messages in the queue } @@ -301,7 +306,7 @@ public class ImapFolder extends Folder { if (discardConnection) { // Return the connection to the pool - close(false); + cleanupConnection(connection, false); } synchronized (mIdleSync) { @@ -309,7 +314,7 @@ public class ImapFolder extends Folder { } } catch (MessagingException me) { - close(false); + cleanupConnection(connection, false); synchronized (mIdleSync) { mIdling = false; } @@ -318,7 +323,7 @@ public class ImapFolder extends Folder { } } catch (SocketTimeoutException ste) { - close(false); + cleanupConnection(connection, false); synchronized (mIdleSync) { mIdling = false; } @@ -327,12 +332,13 @@ public class ImapFolder extends Folder { } } catch (IOException ioe) { - close(false); synchronized (mIdleSync) { mIdling = false; } 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()) { throw new MessagingException("Folder " + mName + " is not open."); } + + final ImapConnection connection; + synchronized (this) { + connection = mConnection; + } + synchronized (mIdleSync) { if (!mIdling) { throw new MessagingException("Folder " + mName + " isn't in IDLE state."); @@ -354,14 +366,15 @@ public class ImapFolder extends Folder { try { mIdlingCancelled = true; mDiscardIdlingConnection = discardConnection; - // We can read responses here because we can block the buffer. Read commands - // are always done by startListener method (blocking idle) - mConnection.sendCommand(ImapConstants.DONE, false); + // Send the DONE command to make the idle reader thread exit. Shorten + // the read timeout for doing that in order to not wait indefinitely, + // the server should respond to the DONE command quickly anyway + connection.sendCommand(ImapConstants.DONE, false); } catch (MessagingException me) { // Treat IOERROR messaging exception as IOException if (me.getExceptionType() == MessagingException.IOERROR) { - close(false); + cleanupConnection(connection, false); 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() { @@ -606,6 +637,21 @@ public class ImapFolder extends Folder { 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 getNewMessagesFromUid(String uid) throws MessagingException { checkOpen(); List nextMSNs = new ArrayList<>(); @@ -1526,14 +1572,10 @@ public class ImapFolder extends Folder { private MessagingException ioExceptionHandler(ImapConnection connection, IOException ioe) { if (DebugUtils.DEBUG) { - LogUtils.d(Logging.LOG_TAG, "IO Exception detected: ", ioe); + LogUtils.d(Logging.LOG_TAG, ioe, "IO Exception detected: "); } if (connection != null) { - connection.close(); - } - if (connection == mConnection) { - mConnection = null; // To prevent close() from returning the connection to the pool. - close(false); + cleanupConnection(connection, true); } return new MessagingException(MessagingException.IOERROR, "IO Error", ioe); } diff --git a/provider_src/com/android/email/mail/store/ImapStore.java b/provider_src/com/android/email/mail/store/ImapStore.java index ddabe1c4a..35c402126 100644 --- a/provider_src/com/android/email/mail/store/ImapStore.java +++ b/provider_src/com/android/email/mail/store/ImapStore.java @@ -467,6 +467,7 @@ public class ImapStore extends Store { return mailboxes.values().toArray(new Folder[mailboxes.size()]); } catch (IOException ioe) { connection.close(); + connection = null; throw new MessagingException("Unable to get folder list", ioe); } catch (AuthenticationFailedException afe) { // We do NOT want this connection pooled, or we will continue to send NOOP and SELECT diff --git a/provider_src/com/android/email/service/ImapService.java b/provider_src/com/android/email/service/ImapService.java index 4522809f0..8a61da31b 100644 --- a/provider_src/com/android/email/service/ImapService.java +++ b/provider_src/com/android/email/service/ImapService.java @@ -176,13 +176,13 @@ public class ImapService extends Service { private static class ImapIdleListener implements ImapFolder.IdleCallback { private final Context mContext; - private final Store mStore; + private final Account mAccount; private final Mailbox mMailbox; - public ImapIdleListener(Context context, Store store, Mailbox mailbox) { + public ImapIdleListener(Context context, Account account, Mailbox mailbox) { super(); mContext = context; - mStore = store; + mAccount = account; mMailbox = mailbox; } @@ -205,7 +205,7 @@ public class ImapService extends Service { @Override public void run() { // Selectively process all the retrieved changes - processImapIdleChangesLocked(mContext, mStore.getAccount(), mMailbox, + processImapIdleChangesLocked(mContext, mAccount, mMailbox, needSync, fetchMessages); } }); @@ -330,13 +330,17 @@ public class ImapService extends Service { return sInstance; } - private boolean isMailboxIdled(long mailboxId) { + private ImapFolder getIdledMailbox(long mailboxId) { synchronized (mIdledFolders) { 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) throws MessagingException { synchronized (mIdledFolders) { @@ -372,7 +376,8 @@ public class ImapService extends Service { mIdledFolders.put((int) mailbox.mId, folder); } 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); 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 { + final ImapFolder folder; 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 { // 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); - return; + return null; } - // Stop idling - ImapFolder folder = mIdledFolders.get((int) mailboxId); - if (disconnect) { - folder.stopIdling(remove); - } if (remove) { 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) @@ -461,15 +467,17 @@ public class ImapService extends Service { private void kickIdledMailbox(Context context, Mailbox mailbox, Account account) throws MessagingException { - synchronized (mIdledFolders) { - unregisterIdledMailboxLocked(mailbox.mId, true); - registerMailboxForIdle(context, account, mailbox); + final ImapFolder folder = getIdledMailbox((int) mailbox.mId); + if (folder != null) { + folder.stopIdling(false); + folder.startIdling(new ImapIdleListener(context, account, mailbox)); } } private void unregisterAccountIdledMailboxes(Context context, long accountId, boolean remove) { LogUtils.i(LOG_TAG, "Unregister idle for account " + accountId); + final ArrayList foldersToStop = new ArrayList<>(); synchronized (mIdledFolders) { int count = mIdledFolders.size() - 1; @@ -478,9 +486,11 @@ public class ImapService extends Service { try { Mailbox mailbox = Mailbox.restoreMailboxWithId(context, mailboxId); if (mailbox == null || mailbox.mAccountKey == accountId) { - unregisterIdledMailbox(mailboxId, remove, true); - - LogUtils.i(LOG_TAG, "Unregister idle for mailbox " + mailboxId); + ImapFolder folder = unregisterIdledMailboxLocked(mailboxId, remove); + if (folder != null) { + foldersToStop.add(folder); + LogUtils.i(LOG_TAG, "Unregister idle for mailbox " + mailboxId); + } } } catch (MessagingException ex) { 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) { @@ -495,22 +506,35 @@ public class ImapService extends Service { sExecutor.execute(new Runnable() { @Override public void run() { + final ArrayList foldersToStop = new ArrayList<>(); synchronized (mIdledFolders) { LogUtils.i(LOG_TAG, "Unregister all idle mailboxes"); - int count = mIdledFolders.size() - 1; - for (int index = count; index >= 0; index--) { - long mailboxId = mIdledFolders.keyAt(index); - try { - unregisterIdledMailbox(mailboxId, true, disconnect); - } catch (MessagingException ex) { - LogUtils.w(LOG_TAG, "Failed to unregister mailbox " + mailboxId); + if (disconnect) { + int count = mIdledFolders.size(); + for (int i = 0; i < count; i++) { + ImapFolder folder = mIdledFolders.get(mIdledFolders.keyAt(i)); + if (folder != null && folder.isIdling()) { + foldersToStop.add(folder); + } } } + mIdledFolders.clear(); } + stopIdlingForFolders(foldersToStop); } }); } + + private void stopIdlingForFolders(final List folders) { + for (ImapFolder folder : folders) { + try { + folder.stopIdling(true); + } catch (MessagingException me) { + // ignored + } + } + } } 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 if (op.equals(EmailProvider.NOTIFICATION_OP_DELETE)) { try { - ImapIdleFolderHolder.getInstance().unregisterIdledMailboxLocked(id, true); + ImapIdleFolderHolder.getInstance().unregisterIdledMailbox(id, true); } catch (MessagingException me) { 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; if (registered != toRegister) { if (registered) { - holder.unregisterIdledMailboxLocked(id, true); + holder.unregisterIdledMailbox(id, true); } if (toRegister) { holder.registerMailboxForIdle(mContext, account, mailbox); @@ -1084,7 +1108,7 @@ public class ImapService extends Service { // Unregister the imap idle if (account.getSyncInterval() == Account.CHECK_INTERVAL_PUSH) { - imapHolder.unregisterIdledMailboxLocked(folder.mId, false); + imapHolder.unregisterIdledMailbox(folder.mId, false); } else { imapHolder.unregisterAccountIdledMailboxes(context, account.mId, false); }