From a56d5d99a87c2730605e3e068c9d5c27ee730b06 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Mon, 15 Jun 2020 10:39:28 +0100 Subject: [PATCH 1/2] fix test and a crash when closed --- Sources/AsyncHTTPClient/ConnectionsState.swift | 1 - .../ConnectionPoolTests+XCTest.swift | 1 + .../ConnectionPoolTests.swift | 17 +++++++++++++++++ .../HTTPClientInternalTests.swift | 11 ++++++++--- 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionsState.swift b/Sources/AsyncHTTPClient/ConnectionsState.swift index 9962d5dd5..dd215f59e 100644 --- a/Sources/AsyncHTTPClient/ConnectionsState.swift +++ b/Sources/AsyncHTTPClient/ConnectionsState.swift @@ -229,7 +229,6 @@ extension HTTP1ConnectionProvider { self.openedConnectionsCount -= 1 return self.processNextWaiter() case .closed: - assertionFailure("should not happen") return .none } } diff --git a/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift b/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift index 0350edf72..b751ec1ce 100644 --- a/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/ConnectionPoolTests+XCTest.swift @@ -34,6 +34,7 @@ extension ConnectionPoolTests { ("testAcquireReplace", testAcquireReplace), ("testAcquireWhenUnavailableSpecificEL", testAcquireWhenUnavailableSpecificEL), ("testAcquireWhenClosed", testAcquireWhenClosed), + ("testConnectFailedWhenClosed", testConnectFailedWhenClosed), ("testReleaseAliveConnectionEmptyQueue", testReleaseAliveConnectionEmptyQueue), ("testReleaseAliveButClosingConnectionEmptyQueue", testReleaseAliveButClosingConnectionEmptyQueue), ("testReleaseInactiveConnectionEmptyQueue", testReleaseInactiveConnectionEmptyQueue), diff --git a/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift b/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift index 0b7f94dee..ba7c94028 100644 --- a/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift +++ b/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift @@ -320,6 +320,23 @@ class ConnectionPoolTests: XCTestCase { } } + func testConnectFailedWhenClosed() { + var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop) + var snapshot = state.testsOnly_getInternalState() + snapshot.state = .closed + state.testsOnly_setInternalState(snapshot) + + XCTAssertFalse(state.enqueue()) + + let action = state.connectFailed() + switch action { + case .none: + break + default: + XCTFail("Unexpected action: \(action)") + } + } + // MARK: - Release Tests func testReleaseAliveConnectionEmptyQueue() throws { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index c3cd1241e..5656fbe46 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -797,13 +797,18 @@ class HTTPClientInternalTests: XCTestCase { } func testUncleanCloseThrows() { - let httpBin = HTTPBin() + let server = NIOHTTP1TestServer(group: self.clientGroup) defer { - XCTAssertNoThrow(try httpBin.shutdown()) + XCTAssertNoThrow(try server.stop()) } + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) - _ = httpClient.get(url: "http://localhost:\(httpBin.port)/wait") + _ = httpClient.get(url: "http://localhost:\(server.serverPort)/wait") + + XCTAssertNoThrow(try server.readInbound()) // .head + XCTAssertNoThrow(try server.readInbound()) // .end + do { try httpClient.syncShutdown(requiresCleanClose: true) XCTFail("There should be an error on shutdown") From b9f23c803e37b35f1ba0cfd1e0bb59915d53f8e0 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Mon, 15 Jun 2020 12:17:09 +0100 Subject: [PATCH 2/2] review fixes --- Sources/AsyncHTTPClient/ConnectionsState.swift | 4 ++++ Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift | 10 ++-------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionsState.swift b/Sources/AsyncHTTPClient/ConnectionsState.swift index dd215f59e..0c5f908e2 100644 --- a/Sources/AsyncHTTPClient/ConnectionsState.swift +++ b/Sources/AsyncHTTPClient/ConnectionsState.swift @@ -229,6 +229,10 @@ extension HTTP1ConnectionProvider { self.openedConnectionsCount -= 1 return self.processNextWaiter() case .closed: + // This can happen in the following scenario: user initiates a connection that will fail to connect, + // user calls `syncShutdown` before we received an error from the bootstrap. In this scenario, + // pool will be `.closed` but connection will be still in the process of being established/failed, + // so then this process finishes, it will get to this point. return .none } } diff --git a/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift b/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift index ba7c94028..bacc04dd8 100644 --- a/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift +++ b/Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift @@ -304,9 +304,7 @@ class ConnectionPoolTests: XCTestCase { func testAcquireWhenClosed() { var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop) - var snapshot = state.testsOnly_getInternalState() - snapshot.state = .closed - state.testsOnly_setInternalState(snapshot) + _ = state.close() XCTAssertFalse(state.enqueue()) @@ -322,11 +320,7 @@ class ConnectionPoolTests: XCTestCase { func testConnectFailedWhenClosed() { var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop) - var snapshot = state.testsOnly_getInternalState() - snapshot.state = .closed - state.testsOnly_setInternalState(snapshot) - - XCTAssertFalse(state.enqueue()) + _ = state.close() let action = state.connectFailed() switch action {