diff --git a/src/core/imap/MCIMAPSession.cpp b/src/core/imap/MCIMAPSession.cpp index e9bef0e90..ab94490c5 100644 --- a/src/core/imap/MCIMAPSession.cpp +++ b/src/core/imap/MCIMAPSession.cpp @@ -654,6 +654,9 @@ void IMAPSession::unsetup() mImap = NULL; mIdleEnabled = false; mStreamCancelled = false; + // Published with mImap, not after the stream is closed and freed: needsReconnect() reads the + // state from another thread and would otherwise call this session logged in meanwhile. + mState = STATE_DISCONNECTED; UNLOCK(); if (imap != NULL) { @@ -664,8 +667,6 @@ void IMAPSession::unsetup() mailimap_free(imap); imap = NULL; } - - mState = STATE_DISCONNECTED; } void IMAPSession::connect(ErrorCode * pError) @@ -1381,9 +1382,6 @@ void IMAPSession::noop(ErrorCode * pError) { int r; - if (mImap == NULL) - return; - MCLog("connect"); loginIfNeeded(pError); if (* pError != ErrorNone) { diff --git a/unittest/IMAPConnectionLeaseTests.swift b/unittest/IMAPConnectionLeaseTests.swift index 3bbfa0ba9..723ae10e9 100644 --- a/unittest/IMAPConnectionLeaseTests.swift +++ b/unittest/IMAPConnectionLeaseTests.swift @@ -180,6 +180,51 @@ final class IMAPConnectionLeaseTests: XCTestCase { } } + /// A NOOP is the probe the client sends to learn whether a connection is up and its + /// credentials still good, and the command its keep-alive uses to hold a pooled connection + /// warm. Both are pointless on a connection that reports success without connecting - never + /// connected, or torn down behind the keep-alive. + func testNoopConnectsAColdConnection() throws { + let endpoint = try LeaseTestTCPEndpoint(greeting: "* OK [CAPABILITY IMAP4rev1] LeaseTestTCPEndpoint ready\r\n", + answers: ["LOGIN": "", + "CAPABILITY": "* CAPABILITY IMAP4rev1\r\n", + "LIST": "* LIST (\\Noselect) \"/\" \"\"\r\n", + "NOOP": ""]) + defer { endpoint.stop() } + + let session = makeSession(port: endpoint.port, maximumConnections: 1) + + guard let leased = session.acquireConnection(folder: nil) else { + return XCTFail("An empty pool with room for 1 connection must satisfy the lease") + } + + func noop() -> Error? { + let noop = session.noopOperation() + noop.setConnection(leased) + var error: Error? + let finished = DispatchSemaphore(value: 0) + noop.start { opError in + error = opError + finished.signal() + } + XCTAssertEqual(finished.wait(timeout: .now() + 5), .success) + return error + } + + runOffMainThread(timeout: 30) { + XCTAssertNil(noop()) + XCTAssertEqual(endpoint.acceptedClientCount, 1, "A NOOP on a never-connected connection must connect and log in") + + XCTAssertEqual(self.start(leased.disconnectOperation()).wait(timeout: .now() + 5), .success) + XCTAssertTrue(endpoint.waitForClientDisconnect(timeout: 5)) + + XCTAssertNil(noop()) + XCTAssertEqual(endpoint.acceptedClientCount, 2, "A NOOP on a torn-down connection must reconnect rather than report success") + + session.releaseConnection(leased, disconnect: false) + } + } + func testCutWithNothingOnTheWireReconnectsBeforeTheNextOperation() throws { // Every command is answered, so the NOOPs complete and the cut below meets no read in // flight - the case an interrupt of a running command cannot produce. @@ -197,10 +242,6 @@ final class IMAPConnectionLeaseTests: XCTestCase { } runOffMainThread(timeout: 30) { - // A NOOP on a never-connected connection is a no-op, so connect first. - let connect = session.connectOperation() - connect.setConnection(leased) - XCTAssertEqual(self.start(connect).wait(timeout: .now() + 5), .success) let first = session.noopOperation() first.setConnection(leased) XCTAssertEqual(self.start(first).wait(timeout: .now() + 5), .success) diff --git a/unittest/IMAPInterruptCurrentCommandTests.swift b/unittest/IMAPInterruptCurrentCommandTests.swift index ced1cc7e2..fb12d221f 100644 --- a/unittest/IMAPInterruptCurrentCommandTests.swift +++ b/unittest/IMAPInterruptCurrentCommandTests.swift @@ -237,8 +237,16 @@ final class IMAPInterruptCurrentCommandTests: XCTestCase { XCTAssertEqual(runningFinished.wait(timeout: .now() + 10), .success, "interruptCurrentCommand() did not unblock the running command") + + // The queued NOOP now runs and, its connection torn down by the interrupt, reconnects to + // the silent endpoint and blocks in turn - so it is the running operation now, and the + // same interrupt frees it. + XCTAssertEqual(queuedFinished.wait(timeout: .now() + 2), .timedOut, + "The queued NOOP was expected to reconnect and block on the silent socket") + XCTAssertTrue(queued.interruptCurrentCommand(), + "The queued operation was expected to be the running one by now") XCTAssertEqual(queuedFinished.wait(timeout: .now() + 10), .success, - "The queued operation was expected to finish once the connection was freed") + "interruptCurrentCommand() did not unblock the queued operation") } } }