COR-205 - Rank a pooled connection by the reconnect it owes

src/async/imap/MCIMAPAsyncConnection.cpp

@@ -291,9 +291,9 @@ double IMAPAsyncConnection::lastLoginTime()
     return mSession->lastLoginTime();
 }
 
-bool IMAPAsyncConnection::isDisconnected()
+bool IMAPAsyncConnection::needsReconnect()
 {
-    return mSession->isDisconnected();
+    return mSession->needsReconnect();
 }
 
 unsigned int IMAPAsyncConnection::operationsCount()

src/async/imap/MCIMAPAsyncConnection.h

@@ -169,12 +169,11 @@ namespace mailcore {
         virtual bool isQueueRunning();
         virtual void setQueueRunning(bool running);
 
-        // Whether this connection's IMAP session was torn down or never established, so that its
-        // next command connects and logs in first (a connection whose stream failed still reports
-        // connected until the next command notices). Declared last on purpose: this class is
-        // exported, and a virtual inserted among the existing ones would shift every vtable slot
-        // after it.
-        virtual bool isDisconnected();
+        // Whether the next command on this connection has to build it again before it can run -
+        // never connected, torn down, or left with a stream a failed command marked for teardown
+        // (see IMAPSession::needsReconnect). Declared last on purpose: this class is exported, and
+        // a virtual inserted among the existing ones would shift every vtable slot after it.
+        virtual bool needsReconnect();
     };
 
 }

src/async/imap/MCIMAPAsyncSession.cpp

@@ -411,15 +411,15 @@ IMAPAsyncConnection * IMAPAsyncSession::sessionWithMinQueue(bool filterByFolder,
 {
     IMAPAsyncConnection * chosenSession = NULL;
     unsigned int minOperationsCount = 0;
-    bool chosenSessionConnected = false;
+    bool chosenSessionReady = false;
 
     for (unsigned int i = 0 ; i < mSessions->count() ; i ++) {
         IMAPAsyncConnection * s = (IMAPAsyncConnection *) mSessions->objectAtIndex(i);
         // an equally free session that owes a handshake loses to one that does not
         unsigned int operationsCount = s->operationsCount();
-        bool connected = !s->isDisconnected();
+        bool ready = !s->needsReconnect();
         if ((chosenSession == NULL) || (operationsCount < minOperationsCount)
-            || ((operationsCount == minOperationsCount) && connected && !chosenSessionConnected)) {
+            || ((operationsCount == minOperationsCount) && ready && !chosenSessionReady)) {
             bool matched = includeReserved || !s->isReserved();
             if (matched && filterByFolder) {
                 // filter by last selested folder
@@ -429,7 +429,7 @@ IMAPAsyncConnection * IMAPAsyncSession::sessionWithMinQueue(bool filterByFolder,
             if (matched) {
                 chosenSession = s;
                 minOperationsCount = operationsCount;
-                chosenSessionConnected = connected;
+                chosenSessionReady = ready;
             }
         }
     }

src/core/imap/MCIMAPSession.cpp

@@ -1081,6 +1081,8 @@ void IMAPSession::login(ErrorCode * pError)
 
             r = mailimap_list(mImap, "", "", &imap_folders);
             folders = resultsWithError(r, imap_folders, pError);
+            if (* pError == ErrorConnection || * pError == ErrorParse)
+                mShouldDisconnect = true;
             if (* pError != ErrorNone)
                 return;
 
@@ -1380,6 +1382,11 @@ void IMAPSession::noop(ErrorCode * pError)
         r = mailimap_noop(mImap);
         if (r == MAILIMAP_ERROR_STREAM) {
             * pError = ErrorConnection;
+            mShouldDisconnect = true;
+        }
+        if (r == MAILIMAP_ERROR_PARSE) {
+            * pError = ErrorParse;
+            mShouldDisconnect = true;
         }
         if (r == MAILIMAP_ERROR_NOOP) {
             * pError = ErrorNoop;
@@ -4420,6 +4427,11 @@ bool IMAPSession::isDisconnected()
     return mState == STATE_DISCONNECTED;
 }
 
+bool IMAPSession::needsReconnect()
+{
+    return mState == STATE_DISCONNECTED || mShouldDisconnect;
+}
+
 double IMAPSession::lastLoginTime()
 {
     LOCK();

src/core/imap/MCIMAPSession.h

@@ -254,6 +254,13 @@ namespace mailcore {
         virtual void unlockConnectionLogger();
         virtual ConnectionLogger * connectionLoggerNoLock();
 
+        // Whether the next command on this session has to build the connection again - the socket
+        // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike
+        // isDisconnected(), which answers only for the socket and is what the idle timer asks.
+        // Declared last: this class is exported, and a virtual inserted among the existing ones
+        // would shift every vtable slot after it.
+        virtual bool needsReconnect();
+
     private:
         String * mHostname;
         unsigned int mPort;
@@ -300,8 +307,8 @@ namespace mailcore {
         unsigned int mLastFetchedSequenceNumber;
         String * mCurrentFolder;
 		MCB_LOCK_TYPE mIdleLock;
-        // Written on this session's own thread, read by IMAPAsyncSession's connection
-        // selection through IMAPAsyncConnection::isDisconnected: atomic so that read is defined.
+        // Written on this session's own thread, read by IMAPAsyncSession's connection selection
+        // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined.
         std::atomic<int> mState;
         double mLastLoginTime;
         mailimap * mImap;
@@ -311,7 +318,8 @@ namespace mailcore {
 		MCB_LOCK_TYPE mConnectionLoggerLock;
         bool mAutomaticConfigurationEnabled;
         bool mAutomaticConfigurationDone;
-        bool mShouldDisconnect;
+        // Read cross-thread with mState, and for the same reason: see above.
+        std::atomic<bool> mShouldDisconnect;
 
         String * mLoginResponse;
         String * mGmailUserDisplayName;

src/include/MailCore/MCIMAPAsyncConnection.h

@@ -169,12 +169,11 @@ namespace mailcore {
         virtual bool isQueueRunning();
         virtual void setQueueRunning(bool running);
 
-        // Whether this connection's IMAP session was torn down or never established, so that its
-        // next command connects and logs in first (a connection whose stream failed still reports
-        // connected until the next command notices). Declared last on purpose: this class is
-        // exported, and a virtual inserted among the existing ones would shift every vtable slot
-        // after it.
-        virtual bool isDisconnected();
+        // Whether the next command on this connection has to build it again before it can run -
+        // never connected, torn down, or left with a stream a failed command marked for teardown
+        // (see IMAPSession::needsReconnect). Declared last on purpose: this class is exported, and
+        // a virtual inserted among the existing ones would shift every vtable slot after it.
+        virtual bool needsReconnect();
     };
 
 }

src/include/MailCore/MCIMAPSession.h

@@ -254,6 +254,13 @@ namespace mailcore {
         virtual void unlockConnectionLogger();
         virtual ConnectionLogger * connectionLoggerNoLock();
 
+        // Whether the next command on this session has to build the connection again - the socket
+        // is gone, or a failed command left a stream that connectIfNeeded tears down first. Unlike
+        // isDisconnected(), which answers only for the socket and is what the idle timer asks.
+        // Declared last: this class is exported, and a virtual inserted among the existing ones
+        // would shift every vtable slot after it.
+        virtual bool needsReconnect();
+
     private:
         String * mHostname;
         unsigned int mPort;
@@ -300,8 +307,8 @@ namespace mailcore {
         unsigned int mLastFetchedSequenceNumber;
         String * mCurrentFolder;
 		MCB_LOCK_TYPE mIdleLock;
-        // Written on this session's own thread, read by IMAPAsyncSession's connection
-        // selection through IMAPAsyncConnection::isDisconnected: atomic so that read is defined.
+        // Written on this session's own thread, read by IMAPAsyncSession's connection selection
+        // through IMAPAsyncConnection::needsReconnect: atomic so that read is defined.
         std::atomic<int> mState;
         double mLastLoginTime;
         mailimap * mImap;
@@ -311,7 +318,8 @@ namespace mailcore {
 		MCB_LOCK_TYPE mConnectionLoggerLock;
         bool mAutomaticConfigurationEnabled;
         bool mAutomaticConfigurationDone;
-        bool mShouldDisconnect;
+        // Read cross-thread with mState, and for the same reason: see above.
+        std::atomic<bool> mShouldDisconnect;
 
         String * mLoginResponse;
         String * mGmailUserDisplayName;

unittest/IMAPConnectionLeaseTests.swift

@@ -668,6 +668,51 @@ final class IMAPConnectionLeaseTests: XCTestCase {
         }
     }
 
+    /// A connection whose stream died under a command is the case the socket state alone gets
+    /// wrong: libetpan never clears a cancelled stream, so the session stays "connected" while its
+    /// next command has to tear that stream down and build the connection again — strictly more
+    /// than a closed socket costs. It must lose the tie to a connection that can answer.
+    func testAcquirePrefersTheLiveConnectionOverAnInterruptedOne() throws {
+        // LOGIN and what mailcore sends after it are answered, so the command the interrupt cuts is
+        // the NOOP itself - the connection is fully logged in when its stream dies, which is the
+        // state this is about.
+        let endpoint = try LeaseTestTCPEndpoint(greeting: Self.bannerOnlyGreeting,
+                                                answers: ["LOGIN": "",
+                                                          "CAPABILITY": "* CAPABILITY IMAP4rev1\r\n",
+                                                          "LIST": "* LIST (\\Noselect) \"/\" \"\"\r\n"])
+        defer { endpoint.stop() }
+
+        let session = makeSession(port: endpoint.port, maximumConnections: 2)
+        guard let pool = leaseTwoConnections(session) else {
+            return
+        }
+
+        runOffMainThread(timeout: 60) {
+            for connection in pool {
+                self.runConnect(session, on: connection)
+            }
+
+            // Nothing answers the NOOP, so interrupting it is what cancels the stream and leaves
+            // the connection pooled, connected, and owing a reconnect.
+            let noop = session.noopOperation()
+            noop.setConnection(pool[0])
+            let finished = self.start(noop)
+            XCTAssertEqual(finished.wait(timeout: .now() + 2), .timedOut,
+                           "The NOOP was expected to be blocked on the silent socket")
+            XCTAssertTrue(noop.interruptCurrentCommand())
+            XCTAssertEqual(finished.wait(timeout: .now() + 10), .success)
+            self.releaseAll(session, pool)
+
+            guard let acquired = session.acquireConnection(folder: nil) else {
+                return XCTFail("With both connections back in the pool a lease must be satisfied")
+            }
+            defer { session.releaseConnection(acquired, disconnect: false) }
+
+            XCTAssertEqual(acquired.identity, pool[1].identity,
+                           "A cancelled stream costs more than a closed socket, not less")
+        }
+    }
+
     /// Two live idle connections stay interchangeable, and the pick stays the first in the pool.
     func testTiesAmongLiveConnectionsKeepThePoolOrder() throws {
         let endpoint = try LeaseTestTCPEndpoint(greeting: Self.bannerOnlyGreeting)