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

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<ImapResponse> getCommandResponses() throws IOException, MessagingException {
         final List<ImapResponse> responses = new ArrayList<ImapResponse>();
+        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<ImapResponse> responses = mConnection.executeIdleCommand();
+                    List<ImapResponse> 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<String> getNewMessagesFromUid(String uid) throws MessagingException {
         checkOpen();
         List<String> 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 075e11d4d..1e2001596 100644
--- a/provider_src/com/android/email/service/ImapService.java
+++ b/provider_src/com/android/email/service/ImapService.java
@@ -175,13 +175,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;
         }
 
@@ -204,7 +204,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);
                 }
             });
@@ -329,13 +329,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) {
@@ -371,7 +375,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;
@@ -382,31 +387,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)
@@ -460,15 +466,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<ImapFolder> foldersToStop = new ArrayList<>();
 
             synchronized (mIdledFolders) {
                 int count = mIdledFolders.size() - 1;
@@ -477,9 +485,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 "
@@ -487,6 +497,7 @@ public class ImapService extends Service {
                     }
                 }
             }
+            stopIdlingForFolders(foldersToStop);
         }
 
         private void unregisterAllIdledMailboxes(final boolean disconnect) {
@@ -494,22 +505,35 @@ public class ImapService extends Service {
             sExecutor.execute(new Runnable() {
                 @Override
                 public void run() {
+                    final ArrayList<ImapFolder> 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<ImapFolder> folders) {
+            for (ImapFolder folder : folders) {
+                try {
+                    folder.stopIdling(true);
+                } catch (MessagingException me) {
+                    // ignored
+                }
+            }
+        }
     }
 
     private static class ImapEmailConnectivityManager extends EmailConnectivityManager {
@@ -672,7 +696,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.");
                 }
@@ -701,7 +725,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);
@@ -1082,7 +1106,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);
             }