From f255081a85bd1680c6a2d2b7225aa45cd104cb66 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Thu, 5 Aug 2010 16:43:39 -0700 Subject: [PATCH] DO NOT MERGE: Fix handling IOException in ImapStore - mConnection.destroyResponses() should be protected with if (mConnection != null). When we get an IOException, we close the connection and null it out in ioExceptionHandler(). So mConnection can be null at any point after where ioExceptionHandler() first appears. - ioExceptionHandler should close its parent ImapFolder only if the argument connection is mConnection. Methods like exists() may pass an ImapConnection which is not mConnection to ioExceptionHandler. In which case we don't have to close the ImapFolder. Bug 2898211 Backport of I8f9f45d91f596bb8da1a1575593e652d66deb643 Change-Id: I070458b5535540aba69ad7eee88bd2af8ad5f7b1 --- .../android/email/mail/store/ImapStore.java | 45 ++++--- .../email/mail/store/ImapStoreUnitTests.java | 112 ++++++++++++++++++ .../email/mail/transport/MockTransport.java | 55 ++++++--- 3 files changed, 181 insertions(+), 31 deletions(-) diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java index 7e2bc8679..c96cd532c 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/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); } diff --git a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java index fbdbfbe30..867e86acf 100644 --- a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java +++ b/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); + } + }); + } } diff --git a/tests/src/com/android/email/mail/transport/MockTransport.java b/tests/src/com/android/email/mail/transport/MockTransport.java index a54424b2c..81bfb026a 100644 --- a/tests/src/com/android/email/mail/transport/MockTransport.java +++ b/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 = 2; + public static final int ACTION_CLIENT_CLOSE = 1; + 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) { - for (String s : responses) { - mQueuedInput.add(s); + public void expectIOException() { + mPairs.add(new Transaction(Transaction.ACTION_IO_EXCEPTION)); + } + + 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()) { - Transaction pair = mPairs.size() > 0 ? mPairs.get(0) : null; - if (pair != null && pair.mPattern == null) { + if ((mQueuedInput.size() == 0) && (mPairs.size() > 0)) { + Transaction pair = mPairs.get(0); + 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);