Merge "DO NOT MERGE: Fix handling IOException in ImapStore" into gingerbread

src/com/android/email/mail/store/ImapStore.java

@@ -510,10 +510,15 @@ public class ImapStore extends Store {
             mName = name;
         }
 
+        private void destroyResponses() {
+            if (mConnection != null) {
+                mConnection.destroyResponses();
+            }
+        }
+
         @Override
         public void open(OpenMode mode, PersistentDataCallbacks callbacks)
                 throws MessagingException {
-
             try {
                 if (isOpen()) {
                     if (mMode == mode) {
@@ -526,7 +531,7 @@ public class ImapStore extends Store {
                         } catch (IOException ioe) {
                             ioExceptionHandler(mConnection, ioe);
                         } finally {
-                            mConnection.destroyResponses();
+                            destroyResponses();
                         }
                     } else {
                         // Return the connection to the pool, if exists.
@@ -582,7 +587,7 @@ public class ImapStore extends Store {
                 } catch (IOException ioe) {
                     throw ioExceptionHandler(mConnection, ioe);
                 } finally {
-                    mConnection.destroyResponses();
+                    destroyResponses();
                 }
             } catch (MessagingException e) {
                 mExists = false;
@@ -606,6 +611,7 @@ public class ImapStore extends Store {
             // TODO implement expunge
             mMessageCount = -1;
             synchronized (this) {
+                destroyResponses();
                 mStore.poolConnection(mConnection);
                 mConnection = null;
             }
@@ -707,7 +713,7 @@ public class ImapStore extends Store {
             } catch (IOException ioe) {
                 throw ioExceptionHandler(mConnection, ioe);
             } finally {
-                mConnection.destroyResponses();
+                destroyResponses();
             }
         }
 
@@ -735,7 +741,7 @@ public class ImapStore extends Store {
             } catch (IOException ioe) {
                 throw ioExceptionHandler(mConnection, ioe);
             } finally {
-                mConnection.destroyResponses();
+                destroyResponses();
             }
         }
 
@@ -773,7 +779,7 @@ public class ImapStore extends Store {
                 }
                 return uids.toArray(Utility.EMPTY_STRINGS);
             } finally {
-                mConnection.destroyResponses();
+                destroyResponses();
             }
         }
 
@@ -835,16 +841,16 @@ public class ImapStore extends Store {
                 fetchInternal(messages, fp, listener);
             } catch (RuntimeException e) { // Probably a parser error.
                 Log.w(Email.LOG_TAG, "Exception detected: " + e.getMessage());
-                mConnection.logLastDiscourse();
+                if (mConnection != null) {
+                    mConnection.logLastDiscourse();
+                }
                 throw e;
-            } finally {
-                mConnection.destroyResponses();
             }
         }
 
         public void fetchInternal(Message[] messages, FetchProfile fp,
                 MessageRetrievalListener listener) throws MessagingException {
-            if (messages == null || messages.length == 0) {
+            if (messages.length == 0) {
                 return;
             }
             checkOpen();
@@ -897,7 +903,7 @@ public class ImapStore extends Store {
             }
 
             try {
-                String tag = mConnection.sendCommand(String.format(
+                mConnection.sendCommand(String.format(
                         ImapConstants.UID_FETCH + " %s (%s)", joinMessageUids(messages),
                         Utility.combine(fetchFields.toArray(new String[fetchFields.size()]), ' ')
                         ), false);
@@ -991,7 +997,7 @@ public class ImapStore extends Store {
                             listener.messageRetrieved(message);
                         }
                     } finally {
-                        mConnection.destroyResponses();
+                        destroyResponses();
                     }
                 } while (!response.isTagged());
             } catch (IOException ioe) {
@@ -1285,7 +1291,7 @@ public class ImapStore extends Store {
             } catch (IOException ioe) {
                 throw ioExceptionHandler(mConnection, ioe);
             } finally {
-                mConnection.destroyResponses();
+                destroyResponses();
             }
         }
 
@@ -1297,7 +1303,7 @@ public class ImapStore extends Store {
             } catch (IOException ioe) {
                 throw ioExceptionHandler(mConnection, ioe);
             } finally {
-                mConnection.destroyResponses();
+                destroyResponses();
             }
             return null;
         }
@@ -1332,7 +1338,7 @@ public class ImapStore extends Store {
             } catch (IOException ioe) {
                 throw ioExceptionHandler(mConnection, ioe);
             } finally {
-                mConnection.destroyResponses();
+                destroyResponses();
             }
         }
 
@@ -1344,8 +1350,15 @@ public class ImapStore extends Store {
 
         private MessagingException ioExceptionHandler(ImapConnection connection, IOException ioe)
                 throws MessagingException {
+            if (Email.DEBUG) {
+                Log.d(Email.LOG_TAG, "IO Exception detected: ", ioe);
+            }
+            connection.destroyResponses();
             connection.close();
-            close(false);
+            if (connection == mConnection) {
+                mConnection = null; // To prevent close() from returning the connection to the pool.
+                close(false);
+            }
             return new MessagingException("IO Error", ioe);
         }
 

tests/src/com/android/email/mail/store/ImapStoreUnitTests.java

@@ -50,6 +50,7 @@ import android.test.AndroidTestCase;
 import android.test.MoreAsserts;
 import android.test.suitebuilder.annotation.SmallTest;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.regex.Pattern;
@@ -1612,4 +1613,115 @@ public class ImapStoreUnitTests extends AndroidTestCase {
         folders[1].open(OpenMode.READ_WRITE, null);
         folders[1].close(false);
     }
+
+    /**
+     * Callback for {@link #runAndExpectMessagingException}.
+     */
+    private interface RunAndExpectMessagingExceptionTarget {
+        public void run(MockTransport mockTransport) throws Exception;
+    }
+
+    /**
+     * Set up the usual mock transport, open the folder,
+     * run {@link RunAndExpectMessagingExceptionTarget} and make sure a {@link MessagingException}
+     * is thrown.
+     */
+    private void runAndExpectMessagingException(RunAndExpectMessagingExceptionTarget target)
+            throws Exception {
+        try {
+            final MockTransport mockTransport = openAndInjectMockTransport();
+            setupOpenFolder(mockTransport);
+            mFolder.open(OpenMode.READ_WRITE, null);
+
+            target.run(mockTransport);
+
+            fail("MessagingException expected.");
+        } catch (MessagingException expected) {
+        }
+    }
+
+    /**
+     * Make sure that IOExceptions are always converted to MessagingException.
+     */
+    public void testFetchIOException() throws Exception {
+        runAndExpectMessagingException(new RunAndExpectMessagingExceptionTarget() {
+            public void run(MockTransport mockTransport) throws Exception {
+                mockTransport.expectIOException();
+
+                final Message message = mFolder.createMessage("1");
+                final FetchProfile fp = new FetchProfile();
+                fp.add(FetchProfile.Item.STRUCTURE);
+
+                mFolder.fetch(new Message[] { message }, fp, null);
+            }
+        });
+    }
+
+    /**
+     * Make sure that IOExceptions are always converted to MessagingException.
+     */
+    public void testUnreadMessageCountIOException() throws Exception {
+        runAndExpectMessagingException(new RunAndExpectMessagingExceptionTarget() {
+            public void run(MockTransport mockTransport) throws Exception {
+                mockTransport.expectIOException();
+
+                mFolder.getUnreadMessageCount();
+            }
+        });
+    }
+
+    /**
+     * Make sure that IOExceptions are always converted to MessagingException.
+     */
+    public void testCopyMessagesIOException() throws Exception {
+        runAndExpectMessagingException(new RunAndExpectMessagingExceptionTarget() {
+            public void run(MockTransport mockTransport) throws Exception {
+                mockTransport.expectIOException();
+
+                final Message message = mFolder.createMessage("1");
+                final Folder folder = mStore.getFolder("test");
+
+                mFolder.copyMessages(new Message[] { message }, folder, null);
+            }
+        });
+    }
+
+    /**
+     * Make sure that IOExceptions are always converted to MessagingException.
+     */
+    public void testSearchForUidsIOException() throws Exception {
+        runAndExpectMessagingException(new RunAndExpectMessagingExceptionTarget() {
+            public void run(MockTransport mockTransport) throws Exception {
+                mockTransport.expectIOException();
+
+                mFolder.getMessage("uid");
+            }
+        });
+    }
+
+    /**
+     * Make sure that IOExceptions are always converted to MessagingException.
+     */
+    public void testExpungeIOException() throws Exception {
+        runAndExpectMessagingException(new RunAndExpectMessagingExceptionTarget() {
+            public void run(MockTransport mockTransport) throws Exception {
+                mockTransport.expectIOException();
+
+                mFolder.expunge();
+            }
+        });
+    }
+
+    /**
+     * Make sure that IOExceptions are always converted to MessagingException.
+     */
+    public void testOpenIOException() throws Exception {
+        runAndExpectMessagingException(new RunAndExpectMessagingExceptionTarget() {
+            public void run(MockTransport mockTransport) throws Exception {
+                mockTransport.expectIOException();
+                final Folder folder = mStore.getFolder("test");
+                folder.open(OpenMode.READ_WRITE, null);
+            }
+        });
+    }
 }

tests/src/com/android/email/mail/transport/MockTransport.java

@@ -42,6 +42,8 @@ public class MockTransport implements Transport {
 
     private static String LOG_TAG = "MockTransport";
 
+    private static final String SPECIAL_RESPONSE_IOEXCEPTION = "!!!IOEXCEPTION!!!";
+
     private boolean mSslAllowed = false;
     private boolean mTlsAllowed = false;
 
@@ -56,8 +58,8 @@ public class MockTransport implements Transport {
 
     private static class Transaction {
         public static final int ACTION_INJECT_TEXT = 0;
-        public static final int ACTION_SERVER_CLOSE = 1;
+        public static final int ACTION_CLIENT_CLOSE = 1;
-        public static final int ACTION_CLIENT_CLOSE = 2;
+        public static final int ACTION_IO_EXCEPTION = 2;
 
         int mAction;
         String mPattern;
@@ -80,10 +82,10 @@ public class MockTransport implements Transport {
             switch (mAction) {
                 case ACTION_INJECT_TEXT:
                     return mPattern + ": " + Arrays.toString(mResponses);
-                case ACTION_SERVER_CLOSE:
-                    return "Close the server connection";
                 case ACTION_CLIENT_CLOSE:
                     return "Expect the client to close";
+                case ACTION_IO_EXCEPTION:
+                    return "Expect IOException";
                 default:
                     return "(Hmm.  Unknown action.)";
             }
@@ -136,9 +138,22 @@ public class MockTransport implements Transport {
         mPairs.add(new Transaction(Transaction.ACTION_CLIENT_CLOSE));
     }
 
-    private void sendResponse(String[] responses) {
+    public void expectIOException() {
-        for (String s : responses) {
+        mPairs.add(new Transaction(Transaction.ACTION_IO_EXCEPTION));
-            mQueuedInput.add(s);
+    }
+
+    private void sendResponse(Transaction pair) {
+        switch (pair.mAction) {
+            case Transaction.ACTION_INJECT_TEXT:
+                for (String s : pair.mResponses) {
+                    mQueuedInput.add(s);
+                }
+                break;
+            case Transaction.ACTION_IO_EXCEPTION:
+                mQueuedInput.add(SPECIAL_RESPONSE_IOEXCEPTION);
+                break;
+            default:
+                Assert.fail("Invalid action for sendResponse: " + pair.mAction);
         }
     }
 
@@ -250,18 +265,25 @@ public class MockTransport implements Transport {
             throw new IOException("Reading from MockTransport with closed input");
         }
         // if there's nothing to read, see if we can find a null-pattern response
-        if (0 == mQueuedInput.size()) {
+        if ((mQueuedInput.size() == 0) && (mPairs.size() > 0)) {
-            Transaction pair = mPairs.size() > 0 ? mPairs.get(0) : null;
+            Transaction pair = mPairs.get(0);
-            if (pair != null && pair.mPattern == null) {
+            if (pair.mPattern == null) {
                 mPairs.remove(0);
-                sendResponse(pair.mResponses);
+                sendResponse(pair);
             }
         }
-        SmtpSenderUnitTests.assertTrue("Underflow reading from MockTransport", 0 != mQueuedInput.size());
+        if (mQueuedInput.size() == 0) {
+            // MailTransport returns "" at EOS.
+            Log.w(LOG_TAG, "Underflow reading from MockTransport");
+            return "";
+        }
         String line = mQueuedInput.remove(0);
         if (DEBUG_LOG_STREAMS) {
             Log.d(LOG_TAG, "<<< " + line);
         }
+        if (SPECIAL_RESPONSE_IOEXCEPTION.equals(line)) {
+            throw new IOException("Expected IOException.");
+        }
         return line;
     }
 
@@ -292,7 +314,7 @@ public class MockTransport implements Transport {
      *
      * Logs the written text if DEBUG_LOG_STREAMS is true.
      */
-    public void writeLine(String s, String sensitiveReplacement) /* throws IOException */ {
+    public void writeLine(String s, String sensitiveReplacement) throws IOException {
         if (DEBUG_LOG_STREAMS) {
             Log.d(LOG_TAG, ">>> " + s);
         }
@@ -300,11 +322,14 @@ public class MockTransport implements Transport {
         SmtpSenderUnitTests.assertTrue("Overflow writing to MockTransport: Getting " + s,
                 0 != mPairs.size());
         Transaction pair = mPairs.remove(0);
+        if (pair.mAction == Transaction.ACTION_IO_EXCEPTION) {
+            throw new IOException("Expected IOException.");
+        }
         SmtpSenderUnitTests.assertTrue("Unexpected string written to MockTransport: Actual=" + s
                 + "  Expected=" + pair.mPattern,
                 pair.mPattern != null && s.matches(pair.mPattern));
         if (pair.mResponses != null) {
-            sendResponse(pair.mResponses);
+            sendResponse(pair);
         }
     }
 
@@ -354,7 +379,7 @@ public class MockTransport implements Transport {
         StringBuilder sb = new StringBuilder();
 
         @Override
-        public void write(int oneByte) {
+        public void write(int oneByte) throws IOException {
             // CR or CRLF will immediately dump previous line (w/o CRLF)
             if (oneByte == '\r') {
                 writeLine(sb.toString(), null);