From c14ff6ea453530eb06e41b81e6513b32f63631b4 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Thu, 5 Aug 2010 16:43:39 -0700 Subject: [PATCH] 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 Change-Id: I8f9f45d91f596bb8da1a1575593e652d66deb643 --- .../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 4e698670c..656b4e33e 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/src/com/android/email/mail/store/ImapStore.java @@ -517,10 +517,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) { @@ -533,7 +538,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. @@ -589,7 +594,7 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); } finally { - mConnection.destroyResponses(); + destroyResponses(); } } catch (MessagingException e) { mExists = false; @@ -613,6 +618,7 @@ public class ImapStore extends Store { // TODO implement expunge mMessageCount = -1; synchronized (this) { + destroyResponses(); mStore.poolConnection(mConnection); mConnection = null; } @@ -714,7 +720,7 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); } finally { - mConnection.destroyResponses(); + destroyResponses(); } } @@ -742,7 +748,7 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); } finally { - mConnection.destroyResponses(); + destroyResponses(); } } @@ -785,7 +791,7 @@ public class ImapStore extends Store { return ret.toArray(Utility.EMPTY_STRINGS); } } finally { - mConnection.destroyResponses(); + destroyResponses(); } return Utility.EMPTY_STRINGS; } @@ -848,16 +854,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(); @@ -910,7 +916,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); @@ -1004,7 +1010,7 @@ public class ImapStore extends Store { listener.messageRetrieved(message); } } finally { - mConnection.destroyResponses(); + destroyResponses(); } } while (!response.isTagged()); } catch (IOException ioe) { @@ -1298,7 +1304,7 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); } finally { - mConnection.destroyResponses(); + destroyResponses(); } } @@ -1310,7 +1316,7 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); } finally { - mConnection.destroyResponses(); + destroyResponses(); } return null; } @@ -1345,7 +1351,7 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); } finally { - mConnection.destroyResponses(); + destroyResponses(); } } @@ -1357,8 +1363,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 6d6113ac2..5648a5496 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; @@ -1551,4 +1552,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);