diff --git a/src/async/imap/MCIMAPAsyncConnection.cpp b/src/async/imap/MCIMAPAsyncConnection.cpp index 9bd7904b8..a3d4a5dd1 100644 --- a/src/async/imap/MCIMAPAsyncConnection.cpp +++ b/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() diff --git a/src/async/imap/MCIMAPAsyncConnection.h b/src/async/imap/MCIMAPAsyncConnection.h index 66a0679af..0b0cad6e6 100644 --- a/src/async/imap/MCIMAPAsyncConnection.h +++ b/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(); }; } diff --git a/src/async/imap/MCIMAPAsyncSession.cpp b/src/async/imap/MCIMAPAsyncSession.cpp index f6ff80a2c..a0b047c6b 100644 --- a/src/async/imap/MCIMAPAsyncSession.cpp +++ b/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; } } } diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index 2eee3a795..921840c0a 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/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(); diff --git a/src/core/imap/MCIMAPSession.h b/src/core/imap/MCIMAPSession.h index d6ca5f015..fd9890808 100644 --- a/src/core/imap/MCIMAPSession.h +++ b/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 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 mShouldDisconnect; String * mLoginResponse; String * mGmailUserDisplayName; diff --git a/src/include/MailCore/MCIMAPAsyncConnection.h b/src/include/MailCore/MCIMAPAsyncConnection.h index 66a0679af..0b0cad6e6 100644 --- a/src/include/MailCore/MCIMAPAsyncConnection.h +++ b/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(); }; } diff --git a/src/include/MailCore/MCIMAPSession.h b/src/include/MailCore/MCIMAPSession.h index d6ca5f015..fd9890808 100644 --- a/src/include/MailCore/MCIMAPSession.h +++ b/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 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 mShouldDisconnect; String * mLoginResponse; String * mGmailUserDisplayName; diff --git a/unittest/IMAPConnectionLeaseTests.swift b/unittest/IMAPConnectionLeaseTests.swift index 7ac9a2ee4..f4b7d2af9 100644 --- a/unittest/IMAPConnectionLeaseTests.swift +++ b/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)