From a72c5adce3986ff6b5092ae0464a8f2675087860 Mon Sep 17 00:00:00 2001 From: 0xpablo Date: Tue, 5 Jan 2021 12:01:18 +0100 Subject: [PATCH 01/23] Fix Timeout snippet in README.md (#323) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7918ddf70..f287e7a79 100644 --- a/README.md +++ b/README.md @@ -94,7 +94,7 @@ let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, ### Timeouts Timeouts (connect and read) can also be set using the client configuration: ```swift -let timeout = HTTPClient.Timeout(connect: .seconds(1), read: .seconds(1)) +let timeout = HTTPClient.Configuration.Timeout(connect: .seconds(1), read: .seconds(1)) let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, configuration: HTTPClient.Configuration(timeout: timeout)) ``` From 1aec5d7d0ed714f6f2988b8231d0f494f4b96552 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Tue, 19 Jan 2021 17:27:09 +0000 Subject: [PATCH 02/23] Add defensive connection closure. (#328) Motivation: Currently when either we or the server send Connection: close, we correctly do not return that connection to the pool. However, we rely on the server actually performing the connection closure: we never call close() ourselves. This is unnecessarily optimistic: a server may absolutely fail to close this connection. To protect our own file descriptors, we should make sure that any connection we do not return the pool is closed. Modifications: If we think a connection is closing when we release it, we now call close() on it defensively. Result: We no longer leak connections when the server fails to close them. Fixes #324. --- Sources/AsyncHTTPClient/Connection.swift | 4 ++ Sources/AsyncHTTPClient/ConnectionPool.swift | 8 ++++ .../HTTPClientInternalTests.swift | 2 +- .../HTTPClientTestUtils.swift | 47 +++++++++++++++++++ .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 26 ++++++++++ 6 files changed, 87 insertions(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/Connection.swift b/Sources/AsyncHTTPClient/Connection.swift index 3923603ee..6542ca20a 100644 --- a/Sources/AsyncHTTPClient/Connection.swift +++ b/Sources/AsyncHTTPClient/Connection.swift @@ -72,6 +72,10 @@ extension Connection { func close() -> EventLoopFuture { return self.channel.close() } + + func close(promise: EventLoopPromise?) { + return self.channel.close(promise: promise) + } } /// Methods of Connection which are used in ConnectionsState extracted as protocol diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index 1ccd82ee9..e7c68aba4 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -352,6 +352,14 @@ class HTTP1ConnectionProvider { self.state.release(connection: connection, closing: closing) } + // We close defensively here: we may have failed to actually close on other codepaths, + // or we may be expecting the server to close. In either case, we want our FD back, so + // we close now to cover our backs. We don't care about the result: if the channel is + // _already_ closed, that's fine by us. + if closing { + connection.close(promise: nil) + } + switch action { case .none: break diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 803824a0c..24daf6c94 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -812,7 +812,7 @@ class HTTPClientInternalTests: XCTestCase { XCTAssert(connection !== connection2) try! connection2.channel.eventLoop.submit { - connection2.release(closing: true, logger: HTTPClient.loggingDisabled) + connection2.release(closing: false, logger: HTTPClient.loggingDisabled) }.wait() XCTAssertTrue(connection2.channel.isActive) } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 956d67cdd..9962c7209 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -772,6 +772,53 @@ internal final class HttpBinForSSLUncleanShutdownHandler: ChannelInboundHandler } } +internal final class CloseWithoutClosingServerHandler: ChannelInboundHandler { + typealias InboundIn = HTTPServerRequestPart + typealias OutboundOut = HTTPServerResponsePart + + private var callback: (() -> Void)? + private var onClosePromise: EventLoopPromise? + + init(_ callback: @escaping () -> Void) { + self.callback = callback + } + + func handlerAdded(context: ChannelHandlerContext) { + self.onClosePromise = context.eventLoop.makePromise() + self.onClosePromise!.futureResult.whenSuccess(self.callback!) + self.callback = nil + } + + func handlerRemoved(context: ChannelHandlerContext) { + assert(self.onClosePromise == nil) + } + + func channelInactive(context: ChannelHandlerContext) { + if let onClosePromise = self.onClosePromise { + self.onClosePromise = nil + onClosePromise.succeed(()) + } + } + + func channelRead(context: ChannelHandlerContext, data: NIOAny) { + guard case .end = self.unwrapInboundIn(data) else { + return + } + + // We're gonna send a response back here, with Connection: close, but we will + // not close the connection. This reproduces #324. + let headers = HTTPHeaders([ + ("Host", "CloseWithoutClosingServerHandler"), + ("Content-Length", "0"), + ("Connection", "close"), + ]) + let head = HTTPResponseHead(version: .init(major: 1, minor: 1), status: .ok, headers: headers) + + context.write(self.wrapOutboundOut(.head(head)), promise: nil) + context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) + } +} + struct EventLoopFutureTimeoutError: Error {} extension EventLoopFuture { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 655f31792..f54433b1d 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -126,6 +126,7 @@ extension HTTPClientTests { ("testNoBytesSentOverBodyLimit", testNoBytesSentOverBodyLimit), ("testDoubleError", testDoubleError), ("testSSLHandshakeErrorPropagation", testSSLHandshakeErrorPropagation), + ("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 94f649aab..9f59e2c8d 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2712,4 +2712,30 @@ class HTTPClientTests: XCTestCase { #endif } } + + func testWeCloseConnectionsWhenConnectionCloseSetByServer() throws { + let group = DispatchGroup() + group.enter() + + let server = try ServerBootstrap(group: self.serverGroup) + .serverChannelOption(ChannelOptions.socketOption(.so_reuseaddr), value: 1) + .childChannelInitializer { channel in + channel.pipeline.configureHTTPServerPipeline().flatMap { + channel.pipeline.addHandler(CloseWithoutClosingServerHandler(group.leave)) + } + } + .bind(host: "localhost", port: 0) + .wait() + + defer { + server.close(promise: nil) + } + + // Simple request, should go great. + XCTAssertNoThrow(try self.defaultClient.get(url: "http://localhost:\(server.localAddress!.port!)/").wait()) + + // Shouldn't need more than 100ms of waiting to see the close. + let result = group.wait(timeout: DispatchTime.now() + DispatchTimeInterval.milliseconds(100)) + XCTAssertEqual(result, .success, "we never closed the connection!") + } } From 9671de72fde994a0d4545e6e717adf41a5730fc0 Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Fri, 22 Jan 2021 18:25:58 +0100 Subject: [PATCH 03/23] Use welcoming language (#333) --- docker/docker-compose.yaml | 4 ++-- scripts/{sanity.sh => soundness.sh} | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) rename scripts/{sanity.sh => soundness.sh} (89%) diff --git a/docker/docker-compose.yaml b/docker/docker-compose.yaml index 653146147..9789f7a2d 100644 --- a/docker/docker-compose.yaml +++ b/docker/docker-compose.yaml @@ -22,9 +22,9 @@ services: - CAP_NET_RAW - CAP_NET_BIND_SERVICE - sanity: + soundness: <<: *common - command: /bin/bash -xcl "./scripts/sanity.sh" + command: /bin/bash -xcl "./scripts/soundness.sh" test: <<: *common diff --git a/scripts/sanity.sh b/scripts/soundness.sh similarity index 89% rename from scripts/sanity.sh rename to scripts/soundness.sh index b41a24364..bc972b760 100755 --- a/scripts/sanity.sh +++ b/scripts/soundness.sh @@ -33,6 +33,22 @@ else printf "\033[0;32mokay.\033[0m\n" fi +printf "=> Checking for unacceptable language... " +# This greps for unacceptable terminology. The square bracket[s] are so that +# "git grep" doesn't find the lines that greps :). +unacceptable_terms=( + -e blacklis[t] + -e whitelis[t] + -e slav[e] + -e sanit[y] +) +if git grep --color=never -i "${unacceptable_terms[@]}" > /dev/null; then + printf "\033[0;31mUnacceptable language found.\033[0m\n" + git grep -i "${unacceptable_terms[@]}" + exit 1 +fi +printf "\033[0;32mokay.\033[0m\n" + printf "=> Checking format... " FIRST_OUT="$(git status --porcelain)" swiftformat . > /dev/null 2>&1 @@ -46,7 +62,7 @@ else fi printf "=> Checking license headers\n" -tmp=$(mktemp /tmp/.async-http-client-sanity_XXXXXX) +tmp=$(mktemp /tmp/.async-http-client-soundness_XXXXXX) for language in swift-or-c bash dtrace; do printf " * $language... " From bbebce3739719dbd0101d9ced2ce3ea13bf7de91 Mon Sep 17 00:00:00 2001 From: tomer doron Date: Sun, 24 Jan 2021 15:12:34 -0800 Subject: [PATCH 04/23] fix doc generation setup (#336) --- .gitignore | 1 + scripts/generate_docs.sh | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 0ecffe6a8..c71941a4c 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ Package.resolved DerivedData .DS_Store .swiftpm/ +.SourceKitten diff --git a/scripts/generate_docs.sh b/scripts/generate_docs.sh index 26953c6ac..370858e23 100755 --- a/scripts/generate_docs.sh +++ b/scripts/generate_docs.sh @@ -30,7 +30,7 @@ if [[ "$(uname -s)" == "Linux" ]]; then if [[ ! -d "$source_kitten_source_path" ]]; then git clone https://github.com/jpsim/SourceKitten.git "$source_kitten_source_path" fi - source_kitten_path="$source_kitten_source_path/.build/x86_64-unknown-linux/debug" + source_kitten_path="$source_kitten_source_path/.build/debug" if [[ ! -d "$source_kitten_path" ]]; then rm -rf "$source_kitten_source_path/.swift-version" cd "$source_kitten_source_path" && swift build && cd "$root_path" @@ -39,7 +39,7 @@ if [[ "$(uname -s)" == "Linux" ]]; then mkdir -p "$root_path/.build/sourcekitten" for module in "${modules[@]}"; do if [[ ! -f "$root_path/.build/sourcekitten/$module.json" ]]; then - "$source_kitten_path/sourcekitten" doc --spm-module $module > "$root_path/.build/sourcekitten/$module.json" + "$source_kitten_path/sourcekitten" doc --spm --module-name $module > "$root_path/.build/sourcekitten/$module.json" fi done fi From 2bacb9710741bfeaf8832217436c4ddede5bf77f Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Mon, 25 Jan 2021 12:10:01 +0000 Subject: [PATCH 05/23] Address flakiness of testSSLHandshakeErrorPropagation (#335) Motivation: Flaky tests are bad. This test is flaky because the server closes the connection immediately upon channelActive. In practice this can mean that the handshake never even gets a chance to start: by the time the SSLHandler ends up in the pipeline the connection is already dead. Heck, by the time we attempt to complete the connection the connection might be dead. Modifications: - Change the shutdown to be on first read. - Remove the disabled autoRead. - Change the expected NIOTS failure mode to connectTimeout, which is how this manifests in NIOTS. Result: Test is no longer flaky. --- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 67 +++++++++++++++++-- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index f54433b1d..87859ad95 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -126,6 +126,7 @@ extension HTTPClientTests { ("testNoBytesSentOverBodyLimit", testNoBytesSentOverBodyLimit), ("testDoubleError", testDoubleError), ("testSSLHandshakeErrorPropagation", testSSLHandshakeErrorPropagation), + ("testSSLHandshakeErrorPropagationDelayedClose", testSSLHandshakeErrorPropagationDelayedClose), ("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer), ] } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 9f59e2c8d..6a62f9eef 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2680,13 +2680,12 @@ class HTTPClientTests: XCTestCase { class CloseHandler: ChannelInboundHandler { typealias InboundIn = Any - func channelActive(context: ChannelHandlerContext) { + func channelRead(context: ChannelHandlerContext, data: NIOAny) { context.close(promise: nil) } } let server = try ServerBootstrap(group: self.serverGroup) - .childChannelOption(ChannelOptions.autoRead, value: false) .childChannelInitializer { channel in channel.pipeline.addHandler(CloseHandler()) } @@ -2697,17 +2696,73 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(try server.close().wait()) } - let request = try Request(url: "https://localhost:\(server.localAddress!.port!)", method: .GET) - let task = self.defaultClient.execute(request: request, delegate: TestHTTPDelegate()) + // We set the connect timeout down very low here because on NIOTS this manifests as a connect + // timeout. + let config = HTTPClient.Configuration(timeout: HTTPClient.Configuration.Timeout(connect: .milliseconds(100), read: nil)) + let client = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config) + defer { + XCTAssertNoThrow(try client.syncShutdown()) + } + + let request = try Request(url: "https://127.0.0.1:\(server.localAddress!.port!)", method: .GET) + let task = client.execute(request: request, delegate: TestHTTPDelegate()) + + XCTAssertThrowsError(try task.wait()) { error in + #if os(Linux) + XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown) + #else + if isTestingNIOTS() { + XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(100))) + } else { + XCTAssertEqual((error as? IOError).map { $0.errnoCode }, ECONNRESET) + } + #endif + } + } + + func testSSLHandshakeErrorPropagationDelayedClose() throws { + // This is as the test above, but the close handler delays its close action by a few hundred ms. + // This will tend to catch the pipeline at different weird stages, and flush out different bugs. + class CloseHandler: ChannelInboundHandler { + typealias InboundIn = Any + + func channelRead(context: ChannelHandlerContext, data: NIOAny) { + context.eventLoop.scheduleTask(in: .milliseconds(100)) { + context.close(promise: nil) + } + } + } + + let server = try ServerBootstrap(group: self.serverGroup) + .childChannelInitializer { channel in + channel.pipeline.addHandler(CloseHandler()) + } + .bind(host: "127.0.0.1", port: 0) + .wait() + + defer { + XCTAssertNoThrow(try server.close().wait()) + } + + // We set the connect timeout down very low here because on NIOTS this manifests as a connect + // timeout. + let config = HTTPClient.Configuration(timeout: HTTPClient.Configuration.Timeout(connect: .milliseconds(200), read: nil)) + let client = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config) + defer { + XCTAssertNoThrow(try client.syncShutdown()) + } + + let request = try Request(url: "https://127.0.0.1:\(server.localAddress!.port!)", method: .GET) + let task = client.execute(request: request, delegate: TestHTTPDelegate()) XCTAssertThrowsError(try task.wait()) { error in #if os(Linux) XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown) #else if isTestingNIOTS() { - XCTAssertEqual((error as? AsyncHTTPClient.HTTPClient.NWTLSError).map { $0.status }, errSSLClosedNoNotify) + XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(200))) } else { - XCTAssertEqual((error as? IOError).map { $0.errnoCode }, 54) + XCTAssertEqual((error as? IOError).map { $0.errnoCode }, ECONNRESET) } #endif } From ba845ee93d6ea10671e829ebf273b6f7a0c92ef0 Mon Sep 17 00:00:00 2001 From: Daniel Jilg Date: Tue, 9 Feb 2021 14:26:43 +0100 Subject: [PATCH 06/23] Update Readme to account for Package.swift format (#339) Adding the product dependency to the target by name only produces an error in Xcode 12.4. Instead, the product dependency should be given as a `.product`. Updated the README with the new format, so that new user's won't stumble over this. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index f287e7a79..1ace5adee 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ Add the following entry in your Package.swift to start using ``` and `AsyncHTTPClient` dependency to your target: ```swift -.target(name: "MyApp", dependencies: ["AsyncHTTPClient"]), +.target(name: "MyApp", dependencies: [.product(name: "AsyncHTTPClient", package: "async-http-client")]), ``` #### Request-Response API From 5d9b784d69e73c67ba3339933c5209943639d472 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Wed, 3 Mar 2021 17:10:48 +0000 Subject: [PATCH 07/23] Fixes bi-directional streaming (#344) Motivation: When we stream request body, current implementation expects that body will finish streaming _before_ we start to receive response body parts. This is not correct, reponse body parts can start to arrive before we finish sending the request. Modifications: - Simplifies state machine, we only case about request being fully sent to prevent sending body parts after .end, but response state machine is mostly ignored and correct flow will be handled by NIOHTTP1 pipeline - Adds HTTPEchoHandler, that replies to each response body part - Adds bi-directional streaming test Result: Closes #327 --- Sources/AsyncHTTPClient/HTTPHandler.swift | 64 +++++++++---- .../HTTPClientInternalTests+XCTest.swift | 3 + .../HTTPClientInternalTests.swift | 96 ++++++++++++++++++- .../HTTPClientTestUtils.swift | 24 +++++ .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 60 ++++++++---- 6 files changed, 214 insertions(+), 34 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 361f61159..e79ceffbd 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -634,6 +634,9 @@ extension HTTPClient { self.promise.fail(error) connection.channel.close(promise: nil) } + } else { + // this is used in tests where we don't want to bootstrap the whole connection pool + self.promise.fail(error) } } @@ -665,11 +668,11 @@ internal struct TaskCancelEvent {} internal class TaskHandler: RemovableChannelHandler { enum State { case idle - case bodySent - case sent - case head + case sendingBodyWaitingResponseHead + case sendingBodyResponseHeadReceived + case bodySentWaitingResponseHead + case bodySentResponseHeadReceived case redirected(HTTPResponseHead, URL) - case body case endOrError } @@ -794,7 +797,8 @@ extension TaskHandler: ChannelDuplexHandler { typealias OutboundOut = HTTPClientRequestPart func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise?) { - self.state = .idle + self.state = .sendingBodyWaitingResponseHead + let request = self.unwrapOutboundIn(data) var head = HTTPRequestHead(version: HTTPVersion(major: 1, minor: 1), @@ -840,11 +844,25 @@ extension TaskHandler: ChannelDuplexHandler { self.writeBody(request: request, context: context) }.flatMap { context.eventLoop.assertInEventLoop() - if case .endOrError = self.state { + switch self.state { + case .idle: + // since this code path is called from `write` and write sets state to sendingBody + preconditionFailure("should not happen") + case .sendingBodyWaitingResponseHead: + self.state = .bodySentWaitingResponseHead + case .sendingBodyResponseHeadReceived: + self.state = .bodySentResponseHeadReceived + case .bodySentWaitingResponseHead, .bodySentResponseHeadReceived: + preconditionFailure("should not happen, state is \(self.state)") + case .redirected: + break + case .endOrError: + // If the state is .endOrError, it means that request was failed and there is nothing to do here: + // we cannot write .end since channel is most likely closed, and we should not fail the future, + // since the task would already be failed, no need to fail the writer too. return context.eventLoop.makeSucceededFuture(()) } - self.state = .bodySent if let expectedBodyLength = self.expectedBodyLength, expectedBodyLength != self.actualBodyLength { let error = HTTPClientError.bodyLengthMismatch return context.eventLoop.makeFailedFuture(error) @@ -852,11 +870,11 @@ extension TaskHandler: ChannelDuplexHandler { return context.writeAndFlush(self.wrapOutboundOut(.end(nil))) }.map { context.eventLoop.assertInEventLoop() + if case .endOrError = self.state { return } - self.state = .sent self.callOutToDelegateFireAndForget(self.delegate.didSendRequest) }.flatMapErrorThrowing { error in context.eventLoop.assertInEventLoop() @@ -903,6 +921,9 @@ extension TaskHandler: ChannelDuplexHandler { private func writeBodyPart(context: ChannelHandlerContext, part: IOData, promise: EventLoopPromise) { switch self.state { case .idle: + // this function is called on the codepath starting with write, so it cannot be in state .idle + preconditionFailure("should not happen") + case .sendingBodyWaitingResponseHead, .sendingBodyResponseHeadReceived, .redirected: if let limit = self.expectedBodyLength, self.actualBodyLength + part.readableBytes > limit { let error = HTTPClientError.bodyLengthMismatch self.errorCaught(context: context, error: error) @@ -911,7 +932,7 @@ extension TaskHandler: ChannelDuplexHandler { } self.actualBodyLength += part.readableBytes context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) - default: + case .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .endOrError: let error = HTTPClientError.writeAfterRequestSent self.errorCaught(context: context, error: error) promise.fail(error) @@ -931,7 +952,18 @@ extension TaskHandler: ChannelDuplexHandler { let response = self.unwrapInboundIn(data) switch response { case .head(let head): - if case .endOrError = self.state { + switch self.state { + case .idle: + // should be prevented by NIO HTTP1 pipeline, see testHTTPResponseHeadBeforeRequestHead + preconditionFailure("should not happen") + case .sendingBodyWaitingResponseHead: + self.state = .sendingBodyResponseHeadReceived + case .bodySentWaitingResponseHead: + self.state = .bodySentResponseHeadReceived + case .sendingBodyResponseHeadReceived, .bodySentResponseHeadReceived, .redirected: + // should be prevented by NIO HTTP1 pipeline, aee testHTTPResponseDoubleHead + preconditionFailure("should not happen") + case .endOrError: return } @@ -942,7 +974,6 @@ extension TaskHandler: ChannelDuplexHandler { if let redirectURL = self.redirectHandler?.redirectTarget(status: head.status, headers: head.headers) { self.state = .redirected(head, redirectURL) } else { - self.state = .head self.mayRead = false self.callOutToDelegate(value: head, channelEventLoop: context.eventLoop, self.delegate.didReceiveHead) .whenComplete { result in @@ -954,7 +985,6 @@ extension TaskHandler: ChannelDuplexHandler { case .redirected, .endOrError: break default: - self.state = .body self.mayRead = false self.callOutToDelegate(value: body, channelEventLoop: context.eventLoop, self.delegate.didReceiveBodyPart) .whenComplete { result in @@ -1009,10 +1039,10 @@ extension TaskHandler: ChannelDuplexHandler { func channelInactive(context: ChannelHandlerContext) { switch self.state { + case .idle, .sendingBodyWaitingResponseHead, .sendingBodyResponseHeadReceived, .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .redirected: + self.errorCaught(context: context, error: HTTPClientError.remoteConnectionClosed) case .endOrError: break - case .body, .head, .idle, .redirected, .sent, .bodySent: - self.errorCaught(context: context, error: HTTPClientError.remoteConnectionClosed) } context.fireChannelInactive() } @@ -1025,8 +1055,8 @@ extension TaskHandler: ChannelDuplexHandler { /// Some HTTP Servers can 'forget' to respond with CloseNotify when client is closing connection, /// this could lead to incomplete SSL shutdown. But since request is already processed, we can ignore this error. break - case .head where self.ignoreUncleanSSLShutdown, - .body where self.ignoreUncleanSSLShutdown: + case .sendingBodyResponseHeadReceived where self.ignoreUncleanSSLShutdown, + .bodySentResponseHeadReceived where self.ignoreUncleanSSLShutdown: /// We can also ignore this error like `.end`. break default: @@ -1035,7 +1065,7 @@ extension TaskHandler: ChannelDuplexHandler { } default: switch self.state { - case .idle, .bodySent, .sent, .head, .redirected, .body: + case .idle, .sendingBodyWaitingResponseHead, .sendingBodyResponseHeadReceived, .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .redirected: self.state = .endOrError self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError) case .endOrError: diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift index 839a68460..64f691af9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift @@ -29,6 +29,9 @@ extension HTTPClientInternalTests { ("testBadHTTPRequest", testBadHTTPRequest), ("testHostPort", testHostPort), ("testHTTPPartsHandlerMultiBody", testHTTPPartsHandlerMultiBody), + ("testHTTPResponseHeadBeforeRequestHead", testHTTPResponseHeadBeforeRequestHead), + ("testHTTPResponseDoubleHead", testHTTPResponseDoubleHead), + ("testRequestFinishesAfterRedirectIfServerRespondsBeforeClientFinishes", testRequestFinishesAfterRedirectIfServerRespondsBeforeClientFinishes), ("testProxyStreaming", testProxyStreaming), ("testProxyStreamingFailure", testProxyStreamingFailure), ("testUploadStreamingBackpressure", testUploadStreamingBackpressure), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 24daf6c94..89648c726 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -145,7 +145,7 @@ class HTTPClientInternalTests: XCTestCase { try channel.pipeline.addHandler(handler).wait() - handler.state = .sent + handler.state = .bodySentWaitingResponseHead var body = channel.allocator.buffer(capacity: 4) body.writeStaticString("1234") @@ -161,6 +161,100 @@ class HTTPClientInternalTests: XCTestCase { } } + func testHTTPResponseHeadBeforeRequestHead() throws { + let channel = EmbeddedChannel() + XCTAssertNoThrow(try channel.connect(to: try SocketAddress(unixDomainSocketPath: "/fake")).wait()) + + let delegate = TestHTTPDelegate() + let task = Task(eventLoop: channel.eventLoop, logger: HTTPClient.loggingDisabled) + let handler = TaskHandler(task: task, + kind: .host, + delegate: delegate, + redirectHandler: nil, + ignoreUncleanSSLShutdown: false, + logger: HTTPClient.loggingDisabled) + + XCTAssertNoThrow(try channel.pipeline.addHTTPClientHandlers().wait()) + XCTAssertNoThrow(try channel.pipeline.addHandler(handler).wait()) + + XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "HTTP/1.0 200 OK\r\n\r\n"))) + + XCTAssertThrowsError(try task.futureResult.wait()) { error in + XCTAssertEqual(error as? NIOHTTPDecoderError, NIOHTTPDecoderError.unsolicitedResponse) + } + } + + func testHTTPResponseDoubleHead() throws { + let channel = EmbeddedChannel() + XCTAssertNoThrow(try channel.connect(to: try SocketAddress(unixDomainSocketPath: "/fake")).wait()) + + let delegate = TestHTTPDelegate() + let task = Task(eventLoop: channel.eventLoop, logger: HTTPClient.loggingDisabled) + let handler = TaskHandler(task: task, + kind: .host, + delegate: delegate, + redirectHandler: nil, + ignoreUncleanSSLShutdown: false, + logger: HTTPClient.loggingDisabled) + + XCTAssertNoThrow(try channel.pipeline.addHTTPClientHandlers().wait()) + XCTAssertNoThrow(try channel.pipeline.addHandler(handler).wait()) + + let request = try HTTPClient.Request(url: "http://localhost/get") + XCTAssertNoThrow(try channel.writeOutbound(request)) + + XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "HTTP/1.0 200 OK\r\nHTTP/1.0 200 OK\r\n\r\n"))) + + XCTAssertThrowsError(try task.futureResult.wait()) { error in + XCTAssertEqual((error as? HTTPParserError)?.debugDescription, "invalid character in header") + } + } + + func testRequestFinishesAfterRedirectIfServerRespondsBeforeClientFinishes() throws { + let channel = EmbeddedChannel() + + var request = try Request(url: "http://localhost:8080/get") + // This promise is needed to force task handler to process incoming redirecting head before finishing sending the request + let promise = channel.eventLoop.makePromise(of: Void.self) + request.body = .stream(length: 6) { writer in + promise.futureResult.flatMap { + writer.write(.byteBuffer(ByteBuffer(string: "helllo"))) + } + } + + let task = Task(eventLoop: channel.eventLoop, logger: HTTPClient.loggingDisabled) + let redirecter = RedirectHandler(request: request) { _ in + task + } + + let handler = TaskHandler(task: task, + kind: .host, + delegate: TestHTTPDelegate(), + redirectHandler: redirecter, + ignoreUncleanSSLShutdown: false, + logger: HTTPClient.loggingDisabled) + + XCTAssertNoThrow(try channel.pipeline.addHTTPClientHandlers().wait()) + XCTAssertNoThrow(try channel.pipeline.addHandler(handler).wait()) + + let future = channel.write(request) + channel.flush() + + XCTAssertNoThrow(XCTAssertNotNil(try channel.readOutbound(as: ByteBuffer.self))) // expecting to read head from the client + // sending redirect before client finishesh processing the request + XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "HTTP/1.1 302 Found\r\nLocation: /follow\r\n\r\n"))) + channel.flush() + + promise.succeed(()) + + // we expect client to fully send us all bytes + XCTAssertEqual(try channel.readOutbound(as: ByteBuffer.self), ByteBuffer(string: "helllo")) + XCTAssertEqual(try channel.readOutbound(as: ByteBuffer.self), ByteBuffer(string: "")) + XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "\r\n"))) + + XCTAssertNoThrow(try future.wait()) + } + func testProxyStreaming() throws { let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 9962c7209..df0816b49 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -896,6 +896,30 @@ struct CollectEverythingLogHandler: LogHandler { } } +class HTTPEchoHandler: ChannelInboundHandler { + typealias InboundIn = HTTPServerRequestPart + typealias OutboundOut = HTTPServerResponsePart + + var promises: CircularBuffer> = CircularBuffer() + + func channelRead(context: ChannelHandlerContext, data: NIOAny) { + let request = self.unwrapInboundIn(data) + switch request { + case .head: + context.writeAndFlush(self.wrapOutboundOut(.head(.init(version: .init(major: 1, minor: 1), status: .ok))), promise: nil) + case .body(let bytes): + context.writeAndFlush(self.wrapOutboundOut(.body(.byteBuffer(bytes)))).whenSuccess { + if let promise = self.promises.popFirst() { + promise.succeed(()) + } + } + case .end: + context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) + context.close(promise: nil) + } + } +} + private let cert = """ -----BEGIN CERTIFICATE----- MIICmDCCAYACCQCPC8JDqMh1zzANBgkqhkiG9w0BAQsFADANMQswCQYDVQQGEwJ1 diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 87859ad95..263aed5a9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -128,6 +128,7 @@ extension HTTPClientTests { ("testSSLHandshakeErrorPropagation", testSSLHandshakeErrorPropagation), ("testSSLHandshakeErrorPropagationDelayedClose", testSSLHandshakeErrorPropagationDelayedClose), ("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer), + ("testBiDirectionalStreaming", testBiDirectionalStreaming), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 6a62f9eef..b3e28744c 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2708,15 +2708,11 @@ class HTTPClientTests: XCTestCase { let task = client.execute(request: request, delegate: TestHTTPDelegate()) XCTAssertThrowsError(try task.wait()) { error in - #if os(Linux) + if isTestingNIOTS() { + XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(100))) + } else { XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown) - #else - if isTestingNIOTS() { - XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(100))) - } else { - XCTAssertEqual((error as? IOError).map { $0.errnoCode }, ECONNRESET) - } - #endif + } } } @@ -2756,15 +2752,11 @@ class HTTPClientTests: XCTestCase { let task = client.execute(request: request, delegate: TestHTTPDelegate()) XCTAssertThrowsError(try task.wait()) { error in - #if os(Linux) + if isTestingNIOTS() { + XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(200))) + } else { XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown) - #else - if isTestingNIOTS() { - XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(200))) - } else { - XCTAssertEqual((error as? IOError).map { $0.errnoCode }, ECONNRESET) - } - #endif + } } } @@ -2793,4 +2785,40 @@ class HTTPClientTests: XCTestCase { let result = group.wait(timeout: DispatchTime.now() + DispatchTimeInterval.milliseconds(100)) XCTAssertEqual(result, .success, "we never closed the connection!") } + + func testBiDirectionalStreaming() throws { + let handler = HTTPEchoHandler() + + let server = try ServerBootstrap(group: self.serverGroup) + .serverChannelOption(ChannelOptions.socketOption(.so_reuseaddr), value: 1) + .childChannelInitializer { channel in + channel.pipeline.configureHTTPServerPipeline().flatMap { + channel.pipeline.addHandler(handler) + } + } + .bind(host: "localhost", port: 0) + .wait() + + defer { + server.close(promise: nil) + } + + let body: HTTPClient.Body = .stream { writer in + let promise = self.clientGroup.next().makePromise(of: Void.self) + handler.promises.append(promise) + return writer.write(.byteBuffer(ByteBuffer(string: "hello"))).flatMap { + promise.futureResult + }.flatMap { + let promise = self.clientGroup.next().makePromise(of: Void.self) + handler.promises.append(promise) + return writer.write(.byteBuffer(ByteBuffer(string: "hello2"))).flatMap { + promise.futureResult + } + } + } + + let future = self.defaultClient.execute(url: "http://localhost:\(server.localAddress!.port!)", body: body) + + XCTAssertNoThrow(try future.wait()) + } } From 0dda95cffcdf3d96ca551e5701efd1661cf31374 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Mon, 8 Mar 2021 10:52:57 +0000 Subject: [PATCH 08/23] Fix CoW in HTTPResponseAggregator (#345) Motivation: HTTPResponseAggregator attempts to build a single, complete response object. This necessarily means it loads the entire response payload into memory. It wants to provide this payload as a single contiguous buffer of data, and it does so by aggregating the data into a single contiguous buffer as it goes. Because ByteBuffer does exponential reallocation, the cost of doing this should be amortised constant-time, even though we do have to copy some data sometimes. However, if this operation triggers a copy-on-write then the operation will become quadratic. For large buffers this will rapidly come to dominate the runtime. Unfortunately in at least Swift 5.3 Swift cannot safely see that during the body stanza the state variable is dead. Swift is not necessarily wrong about this: there's a cross-module call to ByteBuffer.writeBuffer in place and Swift cannot easily prove that that call will not lead to a re-entrant access of the `HTTPResponseAggregator` object. For this reason, during the call to `didReceiveBodyPart` there will be two copies of the body buffer alive, and so the write will CoW. This quadratic behaviour is a nasty performance trap that can become highly apparent even at quite small body sizes. Modifications: While Swift can't prove that the `self.state` variable is dead, we can! To that end, we temporarily set it to a different value that does not store the buffer in question. This will force Swift to drop the ref on the buffer, making it uniquely owned and avoiding the CoW. Sadly, it's extremely difficult to test for "does not CoW", so this patch does not currently come with any tests. I have experimentally verified the behaviour. Result: No copy-on-write in the HTTPResponseAggregator during body aggregation. --- Sources/AsyncHTTPClient/HTTPHandler.swift | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index e79ceffbd..c15b64a1e 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -380,6 +380,11 @@ public class ResponseAccumulator: HTTPClientResponseDelegate { case .head(let head): self.state = .body(head, part) case .body(let head, var body): + // The compiler can't prove that `self.state` is dead here (and it kinda isn't, there's + // a cross-module call in the way) so we need to drop the original reference to `body` in + // `self.state` or we'll get a CoW. To fix that we temporarily set the state to `.end` (which + // has no associated data). We'll fix it at the bottom of this block. + self.state = .end var part = part body.writeBuffer(&part) self.state = .body(head, body) From ae5f18590765f6c47c042f0db0894a2dd3d57c07 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Tue, 16 Mar 2021 09:41:55 +0000 Subject: [PATCH 09/23] Use synchronous pipeline hops to remove windows. (#346) Motivation: There is an awkward timing window in the TLSEventsHandler flow where it is possible for the NIOSSLClientHandler to fail the handshake on handlerAdded. If this happens, the TLSEventsHandler will not be in the pipeline, and so the handshake failure error will be lost and we'll get a generic one instead. This window can be resolved without performance penalty if we use the new synchronous pipeline operations view to add the two handlers backwards. If this is done then we can ensure that the TLSEventsHandler is always in the pipeline before the NIOSSLClientHandler, and so there is no risk of event loss. While I'm here, AHC does a lot of pipeline modification. This has led to lengthy future chains with lots of event loop hops for no particularly good reason. I've therefore replaced all pipeline operations with their synchronous counterparts. All but one sequence was happening on the correct event loop, and for the one that may not I've added a fast-path dispatch that should tolerate being on the wrong one. The result is cleaner, more linear code that also reduces the allocations and event loop hops. Modifications: - Use synchronous pipeline operations everywhere - Change the order of adding TLSEventsHandler and NIOSSLClientHandler Result: Faster, safer, fewer timing windows. --- Package.swift | 2 +- Sources/AsyncHTTPClient/HTTPClient.swift | 64 +++++++++++++++--------- Sources/AsyncHTTPClient/Utils.swift | 37 +++++++------- 3 files changed, 60 insertions(+), 43 deletions(-) diff --git a/Package.swift b/Package.swift index 261d3f019..01912ec09 100644 --- a/Package.swift +++ b/Package.swift @@ -21,7 +21,7 @@ let package = Package( .library(name: "AsyncHTTPClient", targets: ["AsyncHTTPClient"]), ], dependencies: [ - .package(url: "https://github.com/apple/swift-nio.git", from: "2.19.0"), + .package(url: "https://github.com/apple/swift-nio.git", from: "2.27.0"), .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.8.0"), .package(url: "https://github.com/apple/swift-nio-extras.git", from: "1.3.0"), .package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.5.1"), diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 409d16cfe..10ff09a8f 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -561,16 +561,21 @@ public class HTTPClient { "ahc-task-el": "\(taskEL)"]) let channel = connection.channel - let future: EventLoopFuture - if let timeout = self.resolve(timeout: self.configuration.timeout.read, deadline: deadline) { - future = channel.pipeline.addHandler(IdleStateHandler(readTimeout: timeout)) - } else { - future = channel.eventLoop.makeSucceededFuture(()) - } - return future.flatMap { - return channel.pipeline.addHandler(taskHandler) - }.flatMap { + func prepareChannelForTask0() -> EventLoopFuture { + do { + let syncPipelineOperations = channel.pipeline.syncOperations + + if let timeout = self.resolve(timeout: self.configuration.timeout.read, deadline: deadline) { + try syncPipelineOperations.addHandler(IdleStateHandler(readTimeout: timeout)) + } + + try syncPipelineOperations.addHandler(taskHandler) + } catch { + connection.release(closing: true, logger: logger) + return channel.eventLoop.makeFailedFuture(error) + } + task.setConnection(connection) let isCancelled = task.lock.withLock { @@ -581,14 +586,19 @@ public class HTTPClient { return channel.writeAndFlush(request).flatMapError { _ in // At this point the `TaskHandler` will already be present // to handle the failure and pass it to the `promise` - channel.eventLoop.makeSucceededFuture(()) + channel.eventLoop.makeSucceededVoidFuture() } } else { - return channel.eventLoop.makeSucceededFuture(()) + return channel.eventLoop.makeSucceededVoidFuture() + } + } + + if channel.eventLoop.inEventLoop { + return prepareChannelForTask0() + } else { + return channel.eventLoop.flatSubmit { + return prepareChannelForTask0() } - }.flatMapError { error in - connection.release(closing: true, logger: logger) - return channel.eventLoop.makeFailedFuture(error) } }.always { _ in setupComplete.succeed(()) @@ -873,7 +883,7 @@ extension HTTPClient.Configuration { } extension ChannelPipeline { - func addProxyHandler(host: String, port: Int, authorization: HTTPClient.Authorization?) -> EventLoopFuture { + func syncAddProxyHandler(host: String, port: Int, authorization: HTTPClient.Authorization?) throws { let encoder = HTTPRequestEncoder() let decoder = ByteToMessageHandler(HTTPResponseDecoder(leftOverBytesStrategy: .forwardBytes)) let handler = HTTPClientProxyHandler(host: host, port: port, authorization: authorization) { channel in @@ -883,28 +893,34 @@ extension ChannelPipeline { channel.pipeline.removeHandler(decoder) } } - return addHandlers([encoder, decoder, handler]) + + let sync = self.syncOperations + try sync.addHandler(encoder) + try sync.addHandler(decoder) + try sync.addHandler(handler) } - func addSSLHandlerIfNeeded(for key: ConnectionPool.Key, tlsConfiguration: TLSConfiguration?, addSSLClient: Bool, handshakePromise: EventLoopPromise) { + func syncAddSSLHandlerIfNeeded(for key: ConnectionPool.Key, tlsConfiguration: TLSConfiguration?, addSSLClient: Bool, handshakePromise: EventLoopPromise) { guard key.scheme.requiresTLS else { handshakePromise.succeed(()) return } do { - let handlers: [ChannelHandler] + let synchronousPipelineView = self.syncOperations + + // We add the TLSEventsHandler first so that it's always in the pipeline before any other TLS handler we add. + let eventsHandler = TLSEventsHandler(completionPromise: handshakePromise) + try synchronousPipelineView.addHandler(eventsHandler) + if addSSLClient { let tlsConfiguration = tlsConfiguration ?? TLSConfiguration.forClient() let context = try NIOSSLContext(configuration: tlsConfiguration) - handlers = [ + try synchronousPipelineView.addHandler( try NIOSSLClientHandler(context: context, serverHostname: (key.host.isIPAddress || key.host.isEmpty) ? nil : key.host), - TLSEventsHandler(completionPromise: handshakePromise), - ] - } else { - handlers = [TLSEventsHandler(completionPromise: handshakePromise)] + position: .before(eventsHandler) + ) } - self.addHandlers(handlers).cascadeFailure(to: handshakePromise) } catch { handshakePromise.fail(error) } diff --git a/Sources/AsyncHTTPClient/Utils.swift b/Sources/AsyncHTTPClient/Utils.swift index 30fbd8107..2f1bf0b40 100644 --- a/Sources/AsyncHTTPClient/Utils.swift +++ b/Sources/AsyncHTTPClient/Utils.swift @@ -128,14 +128,14 @@ extension NIOClientTCPBootstrap { return try self.makeBootstrap(on: eventLoop, host: host, requiresTLS: requiresTLS, configuration: configuration) .channelOption(ChannelOptions.socket(SocketOptionLevel(IPPROTO_TCP), TCP_NODELAY), value: 1) .channelInitializer { channel in - let channelAddedFuture: EventLoopFuture - switch configuration.proxy { - case .none: - channelAddedFuture = eventLoop.makeSucceededFuture(()) - case .some: - channelAddedFuture = channel.pipeline.addProxyHandler(host: host, port: port, authorization: configuration.proxy?.authorization) + do { + if let proxy = configuration.proxy { + try channel.pipeline.syncAddProxyHandler(host: host, port: port, authorization: proxy.authorization) + } + return channel.eventLoop.makeSucceededVoidFuture() + } catch { + return channel.eventLoop.makeFailedFuture(error) } - return channelAddedFuture } } @@ -165,27 +165,28 @@ extension NIOClientTCPBootstrap { let requiresSSLHandler = configuration.proxy != nil && key.scheme.requiresTLS let handshakePromise = channel.eventLoop.makePromise(of: Void.self) - channel.pipeline.addSSLHandlerIfNeeded(for: key, tlsConfiguration: configuration.tlsConfiguration, addSSLClient: requiresSSLHandler, handshakePromise: handshakePromise) + channel.pipeline.syncAddSSLHandlerIfNeeded(for: key, tlsConfiguration: configuration.tlsConfiguration, addSSLClient: requiresSSLHandler, handshakePromise: handshakePromise) + + return handshakePromise.futureResult.flatMapThrowing { + let syncOperations = channel.pipeline.syncOperations + + try syncOperations.addHTTPClientHandlers(leftOverBytesStrategy: .forwardBytes) - return handshakePromise.futureResult.flatMap { - channel.pipeline.addHTTPClientHandlers(leftOverBytesStrategy: .forwardBytes) - }.flatMap { #if canImport(Network) if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), bootstrap.underlyingBootstrap is NIOTSConnectionBootstrap { - return channel.pipeline.addHandler(HTTPClient.NWErrorHandler(), position: .first) + try syncOperations.addHandler(HTTPClient.NWErrorHandler(), position: .first) } #endif - return channel.eventLoop.makeSucceededFuture(()) - }.flatMap { + switch configuration.decompression { case .disabled: - return channel.eventLoop.makeSucceededFuture(()) + () case .enabled(let limit): let decompressHandler = NIOHTTPResponseDecompressor(limit: limit) - return channel.pipeline.addHandler(decompressHandler) + try syncOperations.addHandler(decompressHandler) } - }.map { - channel + + return channel } }.flatMapError { error in #if canImport(Network) From b075d190070891b7e4131752d295a9362bbaa809 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Thu, 18 Mar 2021 12:21:23 +0000 Subject: [PATCH 10/23] Unconditionally insert TLSEventsHandler (#349) Motivation: AsyncHTTPClient attempts to avoid the problem of Happy Eyeballs making it hard to know which Channel will be returned by only inserting the TLSEventsHandler upon completion of the connect promise. Unfortunately, as this may involve event loop hops, there are some awkward timing windows in play where the connect may complete before this handler gets added. We should remove that timing window by ensuring that all channels always have this handler in place, and instead of trying to wait until we know which Channel will win, we can find the TLSEventsHandler that belongs to the winning channel after the fact. Modifications: - TLSEventsHandler no longer removes itself from the pipeline or throws away its promise. - makeHTTP1Channel now searches for the TLSEventsHandler from the pipeline that was created and is also responsible for removing it. - Better sanity checking that the proxy TLS case does not overlap with the connection-level TLS case. Results: Further shrinking windows for pipeline management issues. --- Sources/AsyncHTTPClient/HTTPClient.swift | 42 +++++++++---------- Sources/AsyncHTTPClient/Utils.swift | 33 ++++++++++++--- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 23 ++++++++++ 4 files changed, 71 insertions(+), 28 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 10ff09a8f..40b07f67b 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -900,27 +900,25 @@ extension ChannelPipeline { try sync.addHandler(handler) } - func syncAddSSLHandlerIfNeeded(for key: ConnectionPool.Key, tlsConfiguration: TLSConfiguration?, addSSLClient: Bool, handshakePromise: EventLoopPromise) { - guard key.scheme.requiresTLS else { - handshakePromise.succeed(()) - return - } + func syncAddLateSSLHandlerIfNeeded(for key: ConnectionPool.Key, tlsConfiguration: TLSConfiguration?, handshakePromise: EventLoopPromise) { + precondition(key.scheme.requiresTLS) do { let synchronousPipelineView = self.syncOperations // We add the TLSEventsHandler first so that it's always in the pipeline before any other TLS handler we add. + // If we're here, we must not have one in the channel already. + assert((try? synchronousPipelineView.context(name: TLSEventsHandler.handlerName)) == nil) let eventsHandler = TLSEventsHandler(completionPromise: handshakePromise) - try synchronousPipelineView.addHandler(eventsHandler) - - if addSSLClient { - let tlsConfiguration = tlsConfiguration ?? TLSConfiguration.forClient() - let context = try NIOSSLContext(configuration: tlsConfiguration) - try synchronousPipelineView.addHandler( - try NIOSSLClientHandler(context: context, serverHostname: (key.host.isIPAddress || key.host.isEmpty) ? nil : key.host), - position: .before(eventsHandler) - ) - } + try synchronousPipelineView.addHandler(eventsHandler, name: TLSEventsHandler.handlerName) + + // Then we add the SSL handler. + let tlsConfiguration = tlsConfiguration ?? TLSConfiguration.forClient() + let context = try NIOSSLContext(configuration: tlsConfiguration) + try synchronousPipelineView.addHandler( + try NIOSSLClientHandler(context: context, serverHostname: (key.host.isIPAddress || key.host.isEmpty) ? nil : key.host), + position: .before(eventsHandler) + ) } catch { handshakePromise.fail(error) } @@ -930,7 +928,9 @@ extension ChannelPipeline { class TLSEventsHandler: ChannelInboundHandler, RemovableChannelHandler { typealias InboundIn = NIOAny - var completionPromise: EventLoopPromise? + static let handlerName: String = "AsyncHTTPClient.HTTPClient.TLSEventsHandler" + + var completionPromise: EventLoopPromise init(completionPromise: EventLoopPromise) { self.completionPromise = completionPromise @@ -940,9 +940,7 @@ class TLSEventsHandler: ChannelInboundHandler, RemovableChannelHandler { if let tlsEvent = event as? TLSUserEvent { switch tlsEvent { case .handshakeCompleted: - self.completionPromise?.succeed(()) - self.completionPromise = nil - context.pipeline.removeHandler(self, promise: nil) + self.completionPromise.succeed(()) case .shutdownCompleted: break } @@ -951,15 +949,13 @@ class TLSEventsHandler: ChannelInboundHandler, RemovableChannelHandler { } func errorCaught(context: ChannelHandlerContext, error: Error) { - self.completionPromise?.fail(error) - self.completionPromise = nil - context.pipeline.removeHandler(self, promise: nil) + self.completionPromise.fail(error) context.fireErrorCaught(error) } func handlerRemoved(context: ChannelHandlerContext) { struct NoResult: Error {} - self.completionPromise?.fail(NoResult()) + self.completionPromise.fail(NoResult()) } } diff --git a/Sources/AsyncHTTPClient/Utils.swift b/Sources/AsyncHTTPClient/Utils.swift index 2f1bf0b40..24ec338ab 100644 --- a/Sources/AsyncHTTPClient/Utils.swift +++ b/Sources/AsyncHTTPClient/Utils.swift @@ -131,6 +131,11 @@ extension NIOClientTCPBootstrap { do { if let proxy = configuration.proxy { try channel.pipeline.syncAddProxyHandler(host: host, port: port, authorization: proxy.authorization) + } else if requiresTLS { + // We only add the handshake verifier if we need TLS and we're not going through a proxy. If we're going + // through a proxy we add it later. + let completionPromise = channel.eventLoop.makePromise(of: Void.self) + try channel.pipeline.syncOperations.addHandler(TLSEventsHandler(completionPromise: completionPromise), name: TLSEventsHandler.handlerName) } return channel.eventLoop.makeSucceededVoidFuture() } catch { @@ -162,14 +167,32 @@ extension NIOClientTCPBootstrap { } return channel.flatMap { channel in - let requiresSSLHandler = configuration.proxy != nil && key.scheme.requiresTLS - let handshakePromise = channel.eventLoop.makePromise(of: Void.self) - - channel.pipeline.syncAddSSLHandlerIfNeeded(for: key, tlsConfiguration: configuration.tlsConfiguration, addSSLClient: requiresSSLHandler, handshakePromise: handshakePromise) + let requiresTLS = key.scheme.requiresTLS + let requiresLateSSLHandler = configuration.proxy != nil && requiresTLS + let handshakeFuture: EventLoopFuture + + if requiresLateSSLHandler { + let handshakePromise = channel.eventLoop.makePromise(of: Void.self) + channel.pipeline.syncAddLateSSLHandlerIfNeeded(for: key, tlsConfiguration: configuration.tlsConfiguration, handshakePromise: handshakePromise) + handshakeFuture = handshakePromise.futureResult + } else if requiresTLS { + do { + handshakeFuture = try channel.pipeline.syncOperations.handler(type: TLSEventsHandler.self).completionPromise.futureResult + } catch { + return channel.eventLoop.makeFailedFuture(error) + } + } else { + handshakeFuture = channel.eventLoop.makeSucceededVoidFuture() + } - return handshakePromise.futureResult.flatMapThrowing { + return handshakeFuture.flatMapThrowing { let syncOperations = channel.pipeline.syncOperations + // If we got here and we had a TLSEventsHandler in the pipeline, we can remove it ow. + if requiresTLS { + channel.pipeline.removeHandler(name: TLSEventsHandler.handlerName, promise: nil) + } + try syncOperations.addHTTPClientHandlers(leftOverBytesStrategy: .forwardBytes) #if canImport(Network) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 263aed5a9..46b6d2bbf 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -129,6 +129,7 @@ extension HTTPClientTests { ("testSSLHandshakeErrorPropagationDelayedClose", testSSLHandshakeErrorPropagationDelayedClose), ("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer), ("testBiDirectionalStreaming", testBiDirectionalStreaming), + ("testSynchronousHandshakeErrorReporting", testSynchronousHandshakeErrorReporting), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index b3e28744c..ec9a798a9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2821,4 +2821,27 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(try future.wait()) } + + func testSynchronousHandshakeErrorReporting() throws { + // This only affects cases where we use NIOSSL. + guard !isTestingNIOTS() else { return } + + // We use a specially crafted client that has no cipher suites to offer. To do this we ask + // only for cipher suites incompatible with our TLS version. + let tlsConfig = TLSConfiguration.forClient(minimumTLSVersion: .tlsv13, maximumTLSVersion: .tlsv12, certificateVerification: .none) + let localHTTPBin = HTTPBin(ssl: true) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration(tlsConfiguration: tlsConfig)) + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localHTTPBin.shutdown()) + } + + XCTAssertThrowsError(try localClient.get(url: "https://localhost:\(localHTTPBin.port)/").wait()) { error in + guard let clientError = error as? NIOSSLError, case NIOSSLError.handshakeFailed = clientError else { + XCTFail("Unexpected error: \(error)") + return + } + } + } } From e4fded76acd97d78b70f350b74f5151dae703b3e Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Tue, 30 Mar 2021 11:39:35 +0100 Subject: [PATCH 11/23] Better backpressure management. (#352) Motivation: Users of the HTTPClientResponseDelegate expect that the event loop futures returned from didReceiveHead and didReceiveBodyPart can be used to exert backpressure. To be fair to them, they somewhat can. However, the TaskHandler has a bit of a misunderstanding about how NIO backpressure works, and does not correctly manage the buffer of inbound data. The result of this misunderstanding is that multiple calls to didReceiveBodyPart and didReceiveHead can be outstanding at once. This would likely lead to severe bugs in most delegates, as they do not expect it. We should make things work the way delegate implementers believe it works. Modifications: - Added a buffer to the TaskHandler to avoid delivering data that the delegate is not ready for. - Added a new "pending close" state that keeps track of a state where the TaskHandler has received .end but not yet delivered it to the delegate. This allows better error management. - Added some more tests. - Documented our backpressure commitments. Result: Better respect for backpressure. Resolves #348 --- Sources/AsyncHTTPClient/HTTPHandler.swift | 132 ++++++++++++++---- .../AsyncHTTPClient/ResponseReadBuffer.swift | 32 +++++ .../HTTPClientTestUtils.swift | 75 ++++++++++ .../HTTPClientTests+XCTest.swift | 3 + .../HTTPClientTests.swift | 66 +++++++++ scripts/soundness.sh | 2 +- 6 files changed, 281 insertions(+), 29 deletions(-) create mode 100644 Sources/AsyncHTTPClient/ResponseReadBuffer.swift diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index c15b64a1e..3cdfeeee7 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -419,7 +419,26 @@ public class ResponseAccumulator: HTTPClientResponseDelegate { /// `HTTPClientResponseDelegate` allows an implementation to receive notifications about request processing and to control how response parts are processed. /// You can implement this protocol if you need fine-grained control over an HTTP request/response, for example, if you want to inspect the response /// headers before deciding whether to accept a response body, or if you want to stream your request body. Pass an instance of your conforming -/// class to the `HTTPClient.execute()` method and this package will call each delegate method appropriately as the request takes place. +/// class to the `HTTPClient.execute()` method and this package will call each delegate method appropriately as the request takes place./ +/// +/// ### Backpressure +/// +/// A `HTTPClientResponseDelegate` can be used to exert backpressure on the server response. This is achieved by way of the futures returned from +/// `didReceiveHead` and `didReceiveBodyPart`. The following functions are part of the "backpressure system" in the delegate: +/// +/// - `didReceiveHead` +/// - `didReceiveBodyPart` +/// - `didFinishRequest` +/// - `didReceiveError` +/// +/// The first three methods are strictly _exclusive_, with that exclusivity managed by the futures returned by `didReceiveHead` and +/// `didReceiveBodyPart`. What this means is that until the returned future is completed, none of these three methods will be called +/// again. This allows delegates to rate limit the server to a capacity it can manage. `didFinishRequest` does not return a future, +/// as we are expecting no more data from the server at this time. +/// +/// `didReceiveError` is somewhat special: it signals the end of this regime. `didRecieveError` is not exclusive: it may be called at +/// any time, even if a returned future is not yet completed. `didReceiveError` is terminal, meaning that once it has been called none +/// of these four methods will be called again. This can be used as a signal to abandon all outstanding work. /// /// - note: This delegate is strongly held by the `HTTPTaskHandler` /// for the duration of the `Request` processing and will be @@ -463,6 +482,11 @@ public protocol HTTPClientResponseDelegate: AnyObject { /// You must return an `EventLoopFuture` that you complete when you have finished processing the body part. /// You can create an already succeeded future by calling `task.eventLoop.makeSucceededFuture(())`. /// + /// This function will not be called until the future returned by `didReceiveHead` has completed. + /// + /// This function will not be called for subsequent body parts until the previous future returned by a + /// call to this function completes. + /// /// - parameters: /// - task: Current request context. /// - buffer: Received body `Part`. @@ -471,6 +495,10 @@ public protocol HTTPClientResponseDelegate: AnyObject { /// Called when error was thrown during request execution. Will be called zero or one time only. Request processing will be stopped after that. /// + /// This function may be called at any time: it does not respect the backpressure exerted by `didReceiveHead` and `didReceiveBodyPart`. + /// All outstanding work may be cancelled when this is received. Once called, no further calls will be made to `didReceiveHead`, `didReceiveBodyPart`, + /// or `didFinishRequest`. + /// /// - parameters: /// - task: Current request context. /// - error: Error that occured during response processing. @@ -478,6 +506,9 @@ public protocol HTTPClientResponseDelegate: AnyObject { /// Called when the complete HTTP request is finished. You must return an instance of your `Response` associated type. Will be called once, except if an error occurred. /// + /// This function will not be called until all futures returned by `didReceiveHead` and `didReceiveBodyPart` have completed. Once called, + /// no further calls will be made to `didReceiveHead`, `didReceiveBodyPart`, or `didReceiveError`. + /// /// - parameters: /// - task: Current request context. /// - returns: Result of processing. @@ -678,6 +709,7 @@ internal class TaskHandler: RemovableChann case bodySentWaitingResponseHead case bodySentResponseHeadReceived case redirected(HTTPResponseHead, URL) + case bufferedEnd case endOrError } @@ -688,10 +720,11 @@ internal class TaskHandler: RemovableChann let logger: Logger // We are okay to store the logger here because a TaskHandler is just for one request. var state: State = .idle + var responseReadBuffer: ResponseReadBuffer = ResponseReadBuffer() var expectedBodyLength: Int? var actualBodyLength: Int = 0 var pendingRead = false - var mayRead = true + var outstandingDelegateRead = false var closing = false { didSet { assert(self.closing || !oldValue, @@ -861,10 +894,12 @@ extension TaskHandler: ChannelDuplexHandler { preconditionFailure("should not happen, state is \(self.state)") case .redirected: break - case .endOrError: + case .bufferedEnd, .endOrError: // If the state is .endOrError, it means that request was failed and there is nothing to do here: // we cannot write .end since channel is most likely closed, and we should not fail the future, // since the task would already be failed, no need to fail the writer too. + // If the state is .bufferedEnd the issue is the same, we just haven't fully delivered the response to + // the user yet. return context.eventLoop.makeSucceededFuture(()) } @@ -937,7 +972,7 @@ extension TaskHandler: ChannelDuplexHandler { } self.actualBodyLength += part.readableBytes context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) - case .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .endOrError: + case .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .bufferedEnd, .endOrError: let error = HTTPClientError.writeAfterRequestSent self.errorCaught(context: context, error: error) promise.fail(error) @@ -945,11 +980,11 @@ extension TaskHandler: ChannelDuplexHandler { } public func read(context: ChannelHandlerContext) { - if self.mayRead { + if self.outstandingDelegateRead { + self.pendingRead = true + } else { self.pendingRead = false context.read() - } else { - self.pendingRead = true } } @@ -968,7 +1003,7 @@ extension TaskHandler: ChannelDuplexHandler { case .sendingBodyResponseHeadReceived, .bodySentResponseHeadReceived, .redirected: // should be prevented by NIO HTTP1 pipeline, aee testHTTPResponseDoubleHead preconditionFailure("should not happen") - case .endOrError: + case .bufferedEnd, .endOrError: return } @@ -979,26 +1014,18 @@ extension TaskHandler: ChannelDuplexHandler { if let redirectURL = self.redirectHandler?.redirectTarget(status: head.status, headers: head.headers) { self.state = .redirected(head, redirectURL) } else { - self.mayRead = false - self.callOutToDelegate(value: head, channelEventLoop: context.eventLoop, self.delegate.didReceiveHead) - .whenComplete { result in - self.handleBackpressureResult(context: context, result: result) - } + self.handleReadForDelegate(response, context: context) } - case .body(let body): + case .body: switch self.state { - case .redirected, .endOrError: + case .redirected, .bufferedEnd, .endOrError: break default: - self.mayRead = false - self.callOutToDelegate(value: body, channelEventLoop: context.eventLoop, self.delegate.didReceiveBodyPart) - .whenComplete { result in - self.handleBackpressureResult(context: context, result: result) - } + self.handleReadForDelegate(response, context: context) } case .end: switch self.state { - case .endOrError: + case .bufferedEnd, .endOrError: break case .redirected(let head, let redirectURL): self.state = .endOrError @@ -1006,18 +1033,67 @@ extension TaskHandler: ChannelDuplexHandler { self.redirectHandler?.redirect(status: head.status, to: redirectURL, promise: self.task.promise) } default: - self.state = .endOrError - self.callOutToDelegate(promise: self.task.promise, self.delegate.didFinishRequest) + self.state = .bufferedEnd + self.handleReadForDelegate(response, context: context) + } + } + } + + private func handleReadForDelegate(_ read: HTTPClientResponsePart, context: ChannelHandlerContext) { + if self.outstandingDelegateRead { + self.responseReadBuffer.appendPart(read) + return + } + + // No outstanding delegate read, so we can deliver this directly. + self.deliverReadToDelegate(read, context: context) + } + + private func deliverReadToDelegate(_ read: HTTPClientResponsePart, context: ChannelHandlerContext) { + precondition(!self.outstandingDelegateRead) + self.outstandingDelegateRead = true + + if case .endOrError = self.state { + // No further read delivery should occur, we already delivered an error. + return + } + + switch read { + case .head(let head): + self.callOutToDelegate(value: head, channelEventLoop: context.eventLoop, self.delegate.didReceiveHead) + .whenComplete { result in + self.handleBackpressureResult(context: context, result: result) + } + case .body(let body): + self.callOutToDelegate(value: body, channelEventLoop: context.eventLoop, self.delegate.didReceiveBodyPart) + .whenComplete { result in + self.handleBackpressureResult(context: context, result: result) + } + case .end: + self.state = .endOrError + self.outstandingDelegateRead = false + + if self.pendingRead { + // We must read here, as we will be removed from the channel shortly. + self.pendingRead = false + context.read() } + + self.callOutToDelegate(promise: self.task.promise, self.delegate.didFinishRequest) } } private func handleBackpressureResult(context: ChannelHandlerContext, result: Result) { context.eventLoop.assertInEventLoop() + self.outstandingDelegateRead = false + switch result { case .success: - self.mayRead = true - if self.pendingRead { + if let nextRead = self.responseReadBuffer.nextRead() { + // We can deliver this directly. + self.deliverReadToDelegate(nextRead, context: context) + } else if self.pendingRead { + self.pendingRead = false context.read() } case .failure(let error): @@ -1046,7 +1122,7 @@ extension TaskHandler: ChannelDuplexHandler { switch self.state { case .idle, .sendingBodyWaitingResponseHead, .sendingBodyResponseHeadReceived, .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .redirected: self.errorCaught(context: context, error: HTTPClientError.remoteConnectionClosed) - case .endOrError: + case .bufferedEnd, .endOrError: break } context.fireChannelInactive() @@ -1056,7 +1132,7 @@ extension TaskHandler: ChannelDuplexHandler { switch error { case NIOSSLError.uncleanShutdown: switch self.state { - case .endOrError: + case .bufferedEnd, .endOrError: /// Some HTTP Servers can 'forget' to respond with CloseNotify when client is closing connection, /// this could lead to incomplete SSL shutdown. But since request is already processed, we can ignore this error. break @@ -1070,7 +1146,7 @@ extension TaskHandler: ChannelDuplexHandler { } default: switch self.state { - case .idle, .sendingBodyWaitingResponseHead, .sendingBodyResponseHeadReceived, .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .redirected: + case .idle, .sendingBodyWaitingResponseHead, .sendingBodyResponseHeadReceived, .bodySentWaitingResponseHead, .bodySentResponseHeadReceived, .redirected, .bufferedEnd: self.state = .endOrError self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError) case .endOrError: diff --git a/Sources/AsyncHTTPClient/ResponseReadBuffer.swift b/Sources/AsyncHTTPClient/ResponseReadBuffer.swift new file mode 100644 index 000000000..37cf11665 --- /dev/null +++ b/Sources/AsyncHTTPClient/ResponseReadBuffer.swift @@ -0,0 +1,32 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +import NIO +import NIOHTTP1 + +struct ResponseReadBuffer { + private var responseParts: CircularBuffer + + init() { + self.responseParts = CircularBuffer(initialCapacity: 16) + } + + mutating func appendPart(_ part: HTTPClientResponsePart) { + self.responseParts.append(part) + } + + mutating func nextRead() -> HTTPClientResponsePart? { + return self.responseParts.popFirst() + } +} diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index df0816b49..f10d99677 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -21,6 +21,7 @@ import NIOHTTP1 import NIOHTTPCompression import NIOSSL import NIOTransportServices +import XCTest /// Are we testing NIO Transport services func isTestingNIOTS() -> Bool { @@ -100,6 +101,52 @@ class CountingDelegate: HTTPClientResponseDelegate { } } +class DelayOnHeadDelegate: HTTPClientResponseDelegate { + typealias Response = ByteBuffer + + let promise: EventLoopPromise + + private var data: ByteBuffer + + private var mayReceiveData = false + + private var expectError = false + + init(promise: EventLoopPromise) { + self.promise = promise + self.data = ByteBuffer() + + self.promise.futureResult.whenSuccess { + self.mayReceiveData = true + } + self.promise.futureResult.whenFailure { (_: Error) in + self.expectError = true + } + } + + func didReceiveHead(task: HTTPClient.Task, _ head: HTTPResponseHead) -> EventLoopFuture { + XCTAssertFalse(self.expectError) + return self.promise.futureResult.hop(to: task.eventLoop) + } + + func didReceiveBodyPart(task: HTTPClient.Task, _ buffer: ByteBuffer) -> EventLoopFuture { + XCTAssertTrue(self.mayReceiveData) + XCTAssertFalse(self.expectError) + self.data.writeImmutableBuffer(buffer) + return self.promise.futureResult.hop(to: task.eventLoop) + } + + func didFinishRequest(task: HTTPClient.Task) throws -> Response { + XCTAssertTrue(self.mayReceiveData) + XCTAssertFalse(self.expectError) + return self.data + } + + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + XCTAssertTrue(self.expectError) + } +} + internal final class RecordingHandler: ChannelDuplexHandler { typealias InboundIn = Input typealias OutboundIn = Output @@ -464,6 +511,21 @@ internal final class HttpBinHandler: ChannelInboundHandler { context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) } + func writeChunked(context: ChannelHandlerContext) { + // This tests receiving chunks very fast: please do not insert delays here! + let headers = HTTPHeaders([("Transfer-Encoding", "chunked")]) + + context.write(self.wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok, headers: headers))), promise: nil) + for i in 0..<10 { + let msg = "id: \(i)" + var buf = context.channel.allocator.buffer(capacity: msg.count) + buf.writeString(msg) + context.write(wrapOutboundOut(.body(.byteBuffer(buf))), promise: nil) + } + + context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) + } + func channelRead(context: ChannelHandlerContext, data: NIOAny) { self.isServingRequest = true switch self.unwrapInboundIn(data) { @@ -579,6 +641,19 @@ internal final class HttpBinHandler: ChannelInboundHandler { return case "/events/10/content-length": self.writeEvents(context: context, isContentLengthRequired: true) + case "/chunked": + self.writeChunked(context: context) + return + case "/close-on-response": + var headers = self.responseHeaders + headers.replaceOrAdd(name: "connection", value: "close") + + var builder = HTTPResponseBuilder(.http1_1, status: .ok, headers: headers) + builder.body = ByteBuffer(string: "some body content") + + // We're forcing this closed now. + self.shouldClose = true + self.resps.append(builder) default: context.write(wrapOutboundOut(.head(HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .notFound))), promise: nil) context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 46b6d2bbf..4344fd72e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -130,6 +130,9 @@ extension HTTPClientTests { ("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer), ("testBiDirectionalStreaming", testBiDirectionalStreaming), ("testSynchronousHandshakeErrorReporting", testSynchronousHandshakeErrorReporting), + ("testFileDownloadChunked", testFileDownloadChunked), + ("testCloseWhileBackpressureIsExertedIsFine", testCloseWhileBackpressureIsExertedIsFine), + ("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index ec9a798a9..7e0948aef 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2844,4 +2844,70 @@ class HTTPClientTests: XCTestCase { } } } + + func testFileDownloadChunked() throws { + var request = try Request(url: self.defaultHTTPBinURLPrefix + "chunked") + request.headers.add(name: "Accept", value: "text/event-stream") + + let progress = + try TemporaryFileHelpers.withTemporaryFilePath { path -> FileDownloadDelegate.Progress in + let delegate = try FileDownloadDelegate(path: path) + + let progress = try self.defaultClient.execute( + request: request, + delegate: delegate + ) + .wait() + + try XCTAssertEqual(50, TemporaryFileHelpers.fileSize(path: path)) + + return progress + } + + XCTAssertEqual(nil, progress.totalBytes) + XCTAssertEqual(50, progress.receivedBytes) + } + + func testCloseWhileBackpressureIsExertedIsFine() throws { + let request = try Request(url: self.defaultHTTPBinURLPrefix + "close-on-response") + let backpressurePromise = self.defaultClient.eventLoopGroup.next().makePromise(of: Void.self) + + let resultFuture = self.defaultClient.execute( + request: request, delegate: DelayOnHeadDelegate(promise: backpressurePromise) + ) + + self.defaultClient.eventLoopGroup.next().scheduleTask(in: .milliseconds(50)) { + backpressurePromise.succeed(()) + } + + // The full response must be correctly delivered. + var data = try resultFuture.wait() + guard let info = try data.readJSONDecodable(RequestInfo.self, length: data.readableBytes) else { + XCTFail("Could not parse response") + return + } + XCTAssertEqual(info.data, "some body content") + } + + func testErrorAfterCloseWhileBackpressureExerted() throws { + enum ExpectedError: Error { + case expected + } + + let request = try Request(url: self.defaultHTTPBinURLPrefix + "close-on-response") + let backpressurePromise = self.defaultClient.eventLoopGroup.next().makePromise(of: Void.self) + + let resultFuture = self.defaultClient.execute( + request: request, delegate: DelayOnHeadDelegate(promise: backpressurePromise) + ) + + self.defaultClient.eventLoopGroup.next().scheduleTask(in: .milliseconds(50)) { + backpressurePromise.fail(ExpectedError.expected) + } + + // The task must be failed. + XCTAssertThrowsError(try resultFuture.wait()) { error in + XCTAssertEqual(error as? ExpectedError, .expected) + } + } } diff --git a/scripts/soundness.sh b/scripts/soundness.sh index bc972b760..0e0c5221c 100755 --- a/scripts/soundness.sh +++ b/scripts/soundness.sh @@ -18,7 +18,7 @@ here="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" function replace_acceptable_years() { # this needs to replace all acceptable forms with 'YEARS' - sed -e 's/20[12][0-9]-20[12][0-9]/YEARS/' -e 's/2019/YEARS/' -e 's/2020/YEARS/' + sed -e 's/20[12][0-9]-20[12][0-9]/YEARS/' -e 's/2019/YEARS/' -e 's/2020/YEARS/' -e 's/2021/YEARS/' } printf "=> Checking linux tests... " From f3521033efcf02027367197986afbbf6808a1ed8 Mon Sep 17 00:00:00 2001 From: David Evans Date: Tue, 27 Apr 2021 15:43:07 +0100 Subject: [PATCH 12/23] Fix tests (#356) --- Package.swift | 2 +- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/Package.swift b/Package.swift index 01912ec09..a14c1db8b 100644 --- a/Package.swift +++ b/Package.swift @@ -22,7 +22,7 @@ let package = Package( ], dependencies: [ .package(url: "https://github.com/apple/swift-nio.git", from: "2.27.0"), - .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.8.0"), + .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.12.0"), .package(url: "https://github.com/apple/swift-nio-extras.git", from: "1.3.0"), .package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.5.1"), .package(url: "https://github.com/apple/swift-log.git", from: "1.4.0"), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 7e0948aef..34e95924e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2711,7 +2711,10 @@ class HTTPClientTests: XCTestCase { if isTestingNIOTS() { XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(100))) } else { - XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown) + switch error as? NIOSSLError { + case .some(.handshakeFailed(.sslError(_))): break + default: XCTFail("Handshake failed with unexpected error: \(String(describing: error))") + } } } } @@ -2755,7 +2758,10 @@ class HTTPClientTests: XCTestCase { if isTestingNIOTS() { XCTAssertEqual(error as? ChannelError, .connectTimeout(.milliseconds(200))) } else { - XCTAssertEqual(error as? NIOSSLError, NIOSSLError.uncleanShutdown) + switch error as? NIOSSLError { + case .some(.handshakeFailed(.sslError(_))): break + default: XCTFail("Handshake failed with unexpected error: \(String(describing: error))") + } } } } From abac00add8f9cdee6271a7f38fc6e626ef2694c0 Mon Sep 17 00:00:00 2001 From: tomer doron Date: Thu, 6 May 2021 18:03:11 -0700 Subject: [PATCH 13/23] add 5.4 docker setup for CI (#360) motivation: 5.4 is out! changes: * update Dockerfile handling of rubygems * add docker compose setup for ubuntu 20.04 and 5.4 toolchain --- docker/Dockerfile | 7 ++++--- docker/docker-compose.2004.54.yaml | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 docker/docker-compose.2004.54.yaml diff --git a/docker/Dockerfile b/docker/Dockerfile index 2f15de800..6caf07109 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -17,9 +17,10 @@ RUN apt-get update && apt-get install -y wget RUN apt-get update && apt-get install -y lsof dnsutils netcat-openbsd net-tools libz-dev curl jq # used by integration tests # ruby and jazzy for docs generation -RUN apt-get update && apt-get install -y ruby ruby-dev libsqlite3-dev -# jazzy no longer works on xenial as ruby is too old. -RUN if [ "${ubuntu_version}" != "xenial" ] ; then gem install jazzy --no-ri --no-rdoc ; fi +RUN apt-get update && apt-get install -y ruby ruby-dev libsqlite3-dev build-essential +# switch of gem docs building +RUN echo "gem: --no-document" > ~/.gemrc +RUN if [ "${ubuntu_version}" != "xenial" ] ; then gem install jazzy ; fi # tools RUN mkdir -p $HOME/.tools diff --git a/docker/docker-compose.2004.54.yaml b/docker/docker-compose.2004.54.yaml new file mode 100644 index 000000000..154540ccb --- /dev/null +++ b/docker/docker-compose.2004.54.yaml @@ -0,0 +1,18 @@ +version: "3" + +services: + + runtime-setup: + image: async-http-client:20.04-5.4 + build: + args: + ubuntu_version: "focal" + swift_version: "5.4" + + test: + image: async-http-client:20.04-5.4 + environment: [] + #- SANITIZER_ARG=--sanitize=thread + + shell: + image: async-http-client:20.04-5.4 From b5b04acccd9f1412194c050e76c3dad160894cbe Mon Sep 17 00:00:00 2001 From: tomer doron Date: Thu, 6 May 2021 18:24:43 -0700 Subject: [PATCH 14/23] add 5.4 docker setup for CI (#361) motivation: test with nightly toolchain changes: add docker compose setup for ubuntu 20.04 and nightly toolchain --- docker/docker-compose.2004.main.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 docker/docker-compose.2004.main.yaml diff --git a/docker/docker-compose.2004.main.yaml b/docker/docker-compose.2004.main.yaml new file mode 100644 index 000000000..55715ab72 --- /dev/null +++ b/docker/docker-compose.2004.main.yaml @@ -0,0 +1,18 @@ +version: "3" + +services: + + runtime-setup: + image: async-http-client:20.04-main + build: + args: + base_image: "swiftlang/swift:nightly-main-focal" + + + test: + image: async-http-client:20.04-main + environment: [] + #- SANITIZER_ARG=--sanitize=thread + + shell: + image: async-http-client:20.04-main From ca722d83378d3b6d405f25a672c995bca677873b Mon Sep 17 00:00:00 2001 From: Mads Odgaard Date: Fri, 7 May 2021 14:47:02 +0200 Subject: [PATCH 15/23] Support request specific TLS configuration (#358) Adds support for request-specific TLS configuration: Request(url: "https://webserver.com", tlsConfiguration: .forClient()) --- Package.swift | 2 +- .../BestEffortHashableTLSConfiguration.swift | 32 +++++++++++++ Sources/AsyncHTTPClient/ConnectionPool.swift | 15 +++++- Sources/AsyncHTTPClient/HTTPHandler.swift | 40 +++++++++++++++- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 48 +++++++++++++++++++ 6 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 Sources/AsyncHTTPClient/BestEffortHashableTLSConfiguration.swift diff --git a/Package.swift b/Package.swift index a14c1db8b..f2e606a93 100644 --- a/Package.swift +++ b/Package.swift @@ -22,7 +22,7 @@ let package = Package( ], dependencies: [ .package(url: "https://github.com/apple/swift-nio.git", from: "2.27.0"), - .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.12.0"), + .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.13.0"), .package(url: "https://github.com/apple/swift-nio-extras.git", from: "1.3.0"), .package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.5.1"), .package(url: "https://github.com/apple/swift-log.git", from: "1.4.0"), diff --git a/Sources/AsyncHTTPClient/BestEffortHashableTLSConfiguration.swift b/Sources/AsyncHTTPClient/BestEffortHashableTLSConfiguration.swift new file mode 100644 index 000000000..58169f645 --- /dev/null +++ b/Sources/AsyncHTTPClient/BestEffortHashableTLSConfiguration.swift @@ -0,0 +1,32 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +import NIOSSL + +/// Wrapper around `TLSConfiguration` from NIOSSL to provide a best effort implementation of `Hashable` +struct BestEffortHashableTLSConfiguration: Hashable { + let base: TLSConfiguration + + init(wrapping base: TLSConfiguration) { + self.base = base + } + + func hash(into hasher: inout Hasher) { + self.base.bestEffortHash(into: &hasher) + } + + static func == (lhs: BestEffortHashableTLSConfiguration, rhs: BestEffortHashableTLSConfiguration) -> Bool { + return lhs.base.bestEffortEquals(rhs.base) + } +} diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index e7c68aba4..63be3aa37 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -82,7 +82,7 @@ final class ConnectionPool { } else { let provider = HTTP1ConnectionProvider(key: key, eventLoop: taskEventLoop, - configuration: self.configuration, + configuration: key.config(overriding: self.configuration), pool: self, backgroundActivityLogger: self.backgroundActivityLogger) let enqueued = provider.enqueue() @@ -139,12 +139,16 @@ final class ConnectionPool { self.port = request.port self.host = request.host self.unixPath = request.socketPath + if let tls = request.tlsConfiguration { + self.tlsConfiguration = BestEffortHashableTLSConfiguration(wrapping: tls) + } } var scheme: Scheme var host: String var port: Int var unixPath: String + var tlsConfiguration: BestEffortHashableTLSConfiguration? enum Scheme: Hashable { case http @@ -162,6 +166,15 @@ final class ConnectionPool { } } } + + /// Returns a key-specific `HTTPClient.Configuration` by overriding the properties of `base` + func config(overriding base: HTTPClient.Configuration) -> HTTPClient.Configuration { + var config = base + if let tlsConfiguration = self.tlsConfiguration { + config.tlsConfiguration = tlsConfiguration.base + } + return config + } } } diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 3cdfeeee7..4850c51d8 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -186,6 +186,8 @@ extension HTTPClient { public var headers: HTTPHeaders /// Request body, defaults to no body. public var body: Body? + /// Request-specific TLS configuration, defaults to no request-specific TLS configuration. + public var tlsConfiguration: TLSConfiguration? struct RedirectState { var count: Int @@ -209,11 +211,29 @@ extension HTTPClient { /// - `unsupportedScheme` if URL does contains unsupported HTTP scheme. /// - `emptyHost` if URL does not contains a host. public init(url: String, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { + try self.init(url: url, method: method, headers: headers, body: body, tlsConfiguration: nil) + } + + /// Create HTTP request. + /// + /// - parameters: + /// - url: Remote `URL`. + /// - version: HTTP version. + /// - method: HTTP method. + /// - headers: Custom HTTP headers. + /// - body: Request body. + /// - tlsConfiguration: Request TLS configuration + /// - throws: + /// - `invalidURL` if URL cannot be parsed. + /// - `emptyScheme` if URL does not contain HTTP scheme. + /// - `unsupportedScheme` if URL does contains unsupported HTTP scheme. + /// - `emptyHost` if URL does not contains a host. + public init(url: String, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil, tlsConfiguration: TLSConfiguration?) throws { guard let url = URL(string: url) else { throw HTTPClientError.invalidURL } - try self.init(url: url, method: method, headers: headers, body: body) + try self.init(url: url, method: method, headers: headers, body: body, tlsConfiguration: tlsConfiguration) } /// Create an HTTP `Request`. @@ -229,6 +249,23 @@ extension HTTPClient { /// - `emptyHost` if URL does not contains a host. /// - `missingSocketPath` if URL does not contains a socketPath as an encoded host. public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { + try self.init(url: url, method: method, headers: headers, body: body, tlsConfiguration: nil) + } + + /// Create an HTTP `Request`. + /// + /// - parameters: + /// - url: Remote `URL`. + /// - method: HTTP method. + /// - headers: Custom HTTP headers. + /// - body: Request body. + /// - tlsConfiguration: Request TLS configuration + /// - throws: + /// - `emptyScheme` if URL does not contain HTTP scheme. + /// - `unsupportedScheme` if URL does contains unsupported HTTP scheme. + /// - `emptyHost` if URL does not contains a host. + /// - `missingSocketPath` if URL does not contains a socketPath as an encoded host. + public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil, tlsConfiguration: TLSConfiguration?) throws { guard let scheme = url.scheme?.lowercased() else { throw HTTPClientError.emptyScheme } @@ -244,6 +281,7 @@ extension HTTPClient { self.scheme = scheme self.headers = headers self.body = body + self.tlsConfiguration = tlsConfiguration } /// Whether request will be executed using secure socket. diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 4344fd72e..7e9e2b5d6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -133,6 +133,7 @@ extension HTTPClientTests { ("testFileDownloadChunked", testFileDownloadChunked), ("testCloseWhileBackpressureIsExertedIsFine", testCloseWhileBackpressureIsExertedIsFine), ("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted), + ("testRequestSpecificTLS", testRequestSpecificTLS), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 34e95924e..ed8c1acc5 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2916,4 +2916,52 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(error as? ExpectedError, .expected) } } + + func testRequestSpecificTLS() throws { + let configuration = HTTPClient.Configuration(tlsConfiguration: nil, + timeout: .init(), + ignoreUncleanSSLShutdown: false, + decompression: .disabled) + let localHTTPBin = HTTPBin(ssl: true) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), + configuration: configuration) + let decoder = JSONDecoder() + + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localHTTPBin.shutdown()) + } + + // First two requests use identical TLS configurations. + let firstRequest = try HTTPClient.Request(url: "https://localhost:\(localHTTPBin.port)/get", method: .GET, tlsConfiguration: .forClient(certificateVerification: .none)) + let firstResponse = try localClient.execute(request: firstRequest).wait() + guard let firstBody = firstResponse.body else { + XCTFail("No request body found") + return + } + let firstConnectionNumber = try decoder.decode(RequestInfo.self, from: firstBody).connectionNumber + + let secondRequest = try HTTPClient.Request(url: "https://localhost:\(localHTTPBin.port)/get", method: .GET, tlsConfiguration: .forClient(certificateVerification: .none)) + let secondResponse = try localClient.execute(request: secondRequest).wait() + guard let secondBody = secondResponse.body else { + XCTFail("No request body found") + return + } + let secondConnectionNumber = try decoder.decode(RequestInfo.self, from: secondBody).connectionNumber + + // Uses a differrent TLS config. + let thirdRequest = try HTTPClient.Request(url: "https://localhost:\(localHTTPBin.port)/get", method: .GET, tlsConfiguration: .forClient(maximumTLSVersion: .tlsv1, certificateVerification: .none)) + let thirdResponse = try localClient.execute(request: thirdRequest).wait() + guard let thirdBody = thirdResponse.body else { + XCTFail("No request body found") + return + } + let thirdConnectionNumber = try decoder.decode(RequestInfo.self, from: thirdBody).connectionNumber + + XCTAssertEqual(firstResponse.status, .ok) + XCTAssertEqual(secondResponse.status, .ok) + XCTAssertEqual(thirdResponse.status, .ok) + XCTAssertEqual(firstConnectionNumber, secondConnectionNumber, "Identical TLS configurations did not use the same connection") + XCTAssertNotEqual(thirdConnectionNumber, firstConnectionNumber, "Different TLS configurations did not use different connections.") + } } From e2d03ffb32880269e548dc702bac19c9978160fb Mon Sep 17 00:00:00 2001 From: Johannes Weiss Date: Thu, 13 May 2021 12:16:52 +0100 Subject: [PATCH 16/23] cache NIOSSLContext (saves 27k allocs per conn) (#362) Motivation: At the moment, AHC assumes that creating a `NIOSSLContext` is both cheap and doesn't block. Neither of these two assumptions are true. To create a `NIOSSLContext`, BoringSSL will have to read a lot of certificates in the trust store (on disk) which require a lot of ASN1 parsing and much much more. On my Ubuntu test machine, creating one `NIOSSLContext` is about 27,000 allocations!!! To make it worse, AHC allocates a fresh `NIOSSLContext` for _every single connection_, whether HTTP or HTTPS. Yes, correct. Modification: - Cache NIOSSLContexts per TLSConfiguration in a LRU cache - Don't get an NIOSSLContext for HTTP (plain text) connections Result: New connections should be _much_ faster in general assuming that you're not using a different TLSConfiguration for every connection. --- Sources/AsyncHTTPClient/ConnectionPool.swift | 24 +- Sources/AsyncHTTPClient/HTTPClient.swift | 9 +- Sources/AsyncHTTPClient/LRUCache.swift | 104 ++++++ .../NIOTransportServices/NWErrorHandler.swift | 54 +-- Sources/AsyncHTTPClient/SSLContextCache.swift | 104 ++++++ Sources/AsyncHTTPClient/Utils.swift | 326 ++++++++++-------- .../ConnectionTests.swift | 21 +- .../HTTPClientNIOTSTests.swift | 17 +- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 19 + .../LRUCacheTests+XCTest.swift | 33 ++ .../AsyncHTTPClientTests/LRUCacheTests.swift | 83 +++++ .../SSLContextCacheTests+XCTest.swift | 34 ++ .../SSLContextCacheTests.swift | 75 ++++ Tests/LinuxMain.swift | 2 + 15 files changed, 720 insertions(+), 186 deletions(-) create mode 100644 Sources/AsyncHTTPClient/LRUCache.swift create mode 100644 Sources/AsyncHTTPClient/SSLContextCache.swift create mode 100644 Tests/AsyncHTTPClientTests/LRUCacheTests+XCTest.swift create mode 100644 Tests/AsyncHTTPClientTests/LRUCacheTests.swift create mode 100644 Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift create mode 100644 Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index 63be3aa37..4aecf6d17 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -18,6 +18,7 @@ import NIO import NIOConcurrencyHelpers import NIOHTTP1 import NIOHTTPCompression +import NIOSSL import NIOTLS import NIOTransportServices @@ -41,6 +42,8 @@ final class ConnectionPool { private let backgroundActivityLogger: Logger + let sslContextCache = SSLContextCache() + init(configuration: HTTPClient.Configuration, backgroundActivityLogger: Logger) { self.configuration = configuration self.backgroundActivityLogger = backgroundActivityLogger @@ -106,6 +109,8 @@ final class ConnectionPool { self.providers.values } + self.sslContextCache.shutdown() + return EventLoopFuture.reduce(true, providers.map { $0.close() }, on: eventLoop) { $0 && $1 } } @@ -148,7 +153,7 @@ final class ConnectionPool { var host: String var port: Int var unixPath: String - var tlsConfiguration: BestEffortHashableTLSConfiguration? + private var tlsConfiguration: BestEffortHashableTLSConfiguration? enum Scheme: Hashable { case http @@ -249,14 +254,15 @@ class HTTP1ConnectionProvider { } else { logger.trace("opening fresh connection (found matching but inactive connection)", metadata: ["ahc-dead-connection": "\(connection)"]) - self.makeChannel(preference: waiter.preference).whenComplete { result in + self.makeChannel(preference: waiter.preference, + logger: logger).whenComplete { result in self.connect(result, waiter: waiter, logger: logger) } } } case .create(let waiter): logger.trace("opening fresh connection (no connections to reuse available)") - self.makeChannel(preference: waiter.preference).whenComplete { result in + self.makeChannel(preference: waiter.preference, logger: logger).whenComplete { result in self.connect(result, waiter: waiter, logger: logger) } case .replace(let connection, let waiter): @@ -266,7 +272,7 @@ class HTTP1ConnectionProvider { logger.trace("opening fresh connection (replacing exising connection)", metadata: ["ahc-old-connection": "\(connection)", "ahc-waiter": "\(waiter)"]) - self.makeChannel(preference: waiter.preference).whenComplete { result in + self.makeChannel(preference: waiter.preference, logger: logger).whenComplete { result in self.connect(result, waiter: waiter, logger: logger) } } @@ -434,8 +440,14 @@ class HTTP1ConnectionProvider { return self.closePromise.futureResult.map { true } } - private func makeChannel(preference: HTTPClient.EventLoopPreference) -> EventLoopFuture { - return NIOClientTCPBootstrap.makeHTTP1Channel(destination: self.key, eventLoop: self.eventLoop, configuration: self.configuration, preference: preference) + private func makeChannel(preference: HTTPClient.EventLoopPreference, + logger: Logger) -> EventLoopFuture { + return NIOClientTCPBootstrap.makeHTTP1Channel(destination: self.key, + eventLoop: self.eventLoop, + configuration: self.configuration, + sslContextCache: self.pool.sslContextCache, + preference: preference, + logger: logger) } /// A `Waiter` represents a request that waits for a connection when none is diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 40b07f67b..ec549d993 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -900,7 +900,9 @@ extension ChannelPipeline { try sync.addHandler(handler) } - func syncAddLateSSLHandlerIfNeeded(for key: ConnectionPool.Key, tlsConfiguration: TLSConfiguration?, handshakePromise: EventLoopPromise) { + func syncAddLateSSLHandlerIfNeeded(for key: ConnectionPool.Key, + sslContext: NIOSSLContext, + handshakePromise: EventLoopPromise) { precondition(key.scheme.requiresTLS) do { @@ -913,10 +915,9 @@ extension ChannelPipeline { try synchronousPipelineView.addHandler(eventsHandler, name: TLSEventsHandler.handlerName) // Then we add the SSL handler. - let tlsConfiguration = tlsConfiguration ?? TLSConfiguration.forClient() - let context = try NIOSSLContext(configuration: tlsConfiguration) try synchronousPipelineView.addHandler( - try NIOSSLClientHandler(context: context, serverHostname: (key.host.isIPAddress || key.host.isEmpty) ? nil : key.host), + try NIOSSLClientHandler(context: sslContext, + serverHostname: (key.host.isIPAddress || key.host.isEmpty) ? nil : key.host), position: .before(eventsHandler) ) } catch { diff --git a/Sources/AsyncHTTPClient/LRUCache.swift b/Sources/AsyncHTTPClient/LRUCache.swift new file mode 100644 index 000000000..0a01da0d2 --- /dev/null +++ b/Sources/AsyncHTTPClient/LRUCache.swift @@ -0,0 +1,104 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +struct LRUCache { + private typealias Generation = UInt64 + private struct Element { + var generation: Generation + var key: Key + var value: Value + } + + private let capacity: Int + private var generation: Generation = 0 + private var elements: [Element] + + init(capacity: Int = 8) { + precondition(capacity > 0, "capacity needs to be > 0") + self.capacity = capacity + self.elements = [] + self.elements.reserveCapacity(capacity) + } + + private mutating func bumpGenerationAndFindIndex(key: Key) -> Int? { + self.generation += 1 + + let found = self.elements.firstIndex { element in + element.key == key + } + + return found + } + + mutating func find(key: Key) -> Value? { + if let found = self.bumpGenerationAndFindIndex(key: key) { + self.elements[found].generation = self.generation + return self.elements[found].value + } else { + return nil + } + } + + @discardableResult + mutating func append(key: Key, value: Value) -> Value { + let newElement = Element(generation: self.generation, + key: key, + value: value) + if let found = self.bumpGenerationAndFindIndex(key: key) { + self.elements[found] = newElement + return value + } + + if self.elements.count < self.capacity { + self.elements.append(newElement) + return value + } + assert(self.elements.count == self.capacity) + assert(self.elements.count > 0) + + let minIndex = self.elements.minIndex { l, r in + l.generation < r.generation + }! + + self.elements.swapAt(minIndex, self.elements.endIndex - 1) + self.elements.removeLast() + self.elements.append(newElement) + + return value + } + + mutating func findOrAppend(key: Key, _ valueGenerator: (Key) -> Value) -> Value { + if let found = self.find(key: key) { + return found + } + + return self.append(key: key, value: valueGenerator(key)) + } +} + +extension Array { + func minIndex(by areInIncreasingOrder: (Element, Element) throws -> Bool) rethrows -> Index? { + guard var minSoFar: (Index, Element) = self.first.map({ (0, $0) }) else { + return nil + } + + for indexElement in self.enumerated() { + if try areInIncreasingOrder(indexElement.1, minSoFar.1) { + minSoFar = indexElement + } + } + + return minSoFar.0 + } +} diff --git a/Sources/AsyncHTTPClient/NIOTransportServices/NWErrorHandler.swift b/Sources/AsyncHTTPClient/NIOTransportServices/NWErrorHandler.swift index 1f9dceb88..16e9d4717 100644 --- a/Sources/AsyncHTTPClient/NIOTransportServices/NWErrorHandler.swift +++ b/Sources/AsyncHTTPClient/NIOTransportServices/NWErrorHandler.swift @@ -13,13 +13,14 @@ //===----------------------------------------------------------------------===// #if canImport(Network) - import Network - import NIO - import NIOHTTP1 - import NIOTransportServices +#endif +import NIO +import NIOHTTP1 +import NIOTransportServices - extension HTTPClient { +extension HTTPClient { + #if canImport(Network) public struct NWPOSIXError: Error, CustomStringConvertible { /// POSIX error code (enum) public let errorCode: POSIXErrorCode @@ -57,28 +58,35 @@ public var description: String { return self.reason } } + #endif - @available(macOS 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) - class NWErrorHandler: ChannelInboundHandler { - typealias InboundIn = HTTPClientResponsePart + class NWErrorHandler: ChannelInboundHandler { + typealias InboundIn = HTTPClientResponsePart - func errorCaught(context: ChannelHandlerContext, error: Error) { - context.fireErrorCaught(NWErrorHandler.translateError(error)) - } + func errorCaught(context: ChannelHandlerContext, error: Error) { + context.fireErrorCaught(NWErrorHandler.translateError(error)) + } - static func translateError(_ error: Error) -> Error { - if let error = error as? NWError { - switch error { - case .tls(let status): - return NWTLSError(status, reason: error.localizedDescription) - case .posix(let errorCode): - return NWPOSIXError(errorCode, reason: error.localizedDescription) - default: - return error + static func translateError(_ error: Error) -> Error { + #if canImport(Network) + if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) { + if let error = error as? NWError { + switch error { + case .tls(let status): + return NWTLSError(status, reason: error.localizedDescription) + case .posix(let errorCode): + return NWPOSIXError(errorCode, reason: error.localizedDescription) + default: + return error + } } + return error + } else { + preconditionFailure("\(self) used on a non-NIOTS Channel") } - return error - } + #else + preconditionFailure("\(self) used on a non-NIOTS Channel") + #endif } } -#endif +} diff --git a/Sources/AsyncHTTPClient/SSLContextCache.swift b/Sources/AsyncHTTPClient/SSLContextCache.swift new file mode 100644 index 000000000..582d2cee8 --- /dev/null +++ b/Sources/AsyncHTTPClient/SSLContextCache.swift @@ -0,0 +1,104 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +import Logging +import NIO +import NIOConcurrencyHelpers +import NIOSSL + +class SSLContextCache { + private var state = State.activeNoThread + private let lock = Lock() + private var sslContextCache = LRUCache() + private let threadPool = NIOThreadPool(numberOfThreads: 1) + + enum State { + case activeNoThread + case active + case shutDown + } + + init() {} + + func shutdown() { + self.lock.withLock { () -> Void in + switch self.state { + case .activeNoThread: + self.state = .shutDown + case .active: + self.state = .shutDown + self.threadPool.shutdownGracefully { maybeError in + precondition(maybeError == nil, "\(maybeError!)") + } + case .shutDown: + preconditionFailure("SSLContextCache shut down twice") + } + } + } + + deinit { + assert(self.state == .shutDown) + } +} + +extension SSLContextCache { + private struct SSLContextCacheShutdownError: Error {} + + func sslContext(tlsConfiguration: TLSConfiguration, + eventLoop: EventLoop, + logger: Logger) -> EventLoopFuture { + let earlyExitError: Error? = self.lock.withLock { () -> Error? in + switch self.state { + case .activeNoThread: + self.state = .active + self.threadPool.start() + return nil + case .active: + return nil + case .shutDown: + return SSLContextCacheShutdownError() + } + } + + if let error = earlyExitError { + return eventLoop.makeFailedFuture(error) + } + + let eqTLSConfiguration = BestEffortHashableTLSConfiguration(wrapping: tlsConfiguration) + let sslContext = self.lock.withLock { + self.sslContextCache.find(key: eqTLSConfiguration) + } + + if let sslContext = sslContext { + logger.debug("found SSL context in cache", + metadata: ["ahc-tls-config": "\(tlsConfiguration)"]) + return eventLoop.makeSucceededFuture(sslContext) + } + + logger.debug("creating new SSL context", + metadata: ["ahc-tls-config": "\(tlsConfiguration)"]) + let newSSLContext = self.threadPool.runIfActive(eventLoop: eventLoop) { + try NIOSSLContext(configuration: tlsConfiguration) + } + + newSSLContext.whenSuccess { (newSSLContext: NIOSSLContext) -> Void in + self.lock.withLock { () -> Void in + self.sslContextCache.append(key: eqTLSConfiguration, + value: newSSLContext) + } + } + + return newSSLContext + } +} diff --git a/Sources/AsyncHTTPClient/Utils.swift b/Sources/AsyncHTTPClient/Utils.swift index 24ec338ab..1af7899b2 100644 --- a/Sources/AsyncHTTPClient/Utils.swift +++ b/Sources/AsyncHTTPClient/Utils.swift @@ -16,6 +16,7 @@ import Foundation #if canImport(Network) import Network #endif +import Logging import NIO import NIOHTTP1 import NIOHTTPCompression @@ -52,174 +53,207 @@ public final class HTTPClientCopyingDelegate: HTTPClientResponseDelegate { } } -extension ClientBootstrap { - fileprivate func makeClientTCPBootstrap( - host: String, - requiresTLS: Bool, - configuration: HTTPClient.Configuration - ) throws -> NIOClientTCPBootstrap { - // if there is a proxy don't create TLS provider as it will be added at a later point - if configuration.proxy != nil { - return NIOClientTCPBootstrap(self, tls: NIOInsecureNoTLS()) +extension NIOClientTCPBootstrap { + static func makeHTTP1Channel(destination: ConnectionPool.Key, + eventLoop: EventLoop, + configuration: HTTPClient.Configuration, + sslContextCache: SSLContextCache, + preference: HTTPClient.EventLoopPreference, + logger: Logger) -> EventLoopFuture { + let channelEventLoop = preference.bestEventLoop ?? eventLoop + + let key = destination + let requiresTLS = key.scheme.requiresTLS + let sslContext: EventLoopFuture + if key.scheme.requiresTLS, configuration.proxy != nil { + // If we use a proxy & also require TLS, then we always use NIOSSL (and not Network.framework TLS because + // it can't be added later) and therefore require a `NIOSSLContext`. + // In this case, `makeAndConfigureBootstrap` will not create another `NIOSSLContext`. + // + // Note that TLS proxies are not supported at the moment. This means that we will always speak + // plaintext to the proxy but we do support sending HTTPS traffic through the proxy. + sslContext = sslContextCache.sslContext(tlsConfiguration: configuration.tlsConfiguration ?? .forClient(), + eventLoop: eventLoop, + logger: logger).map { $0 } } else { - let tlsConfiguration = configuration.tlsConfiguration ?? TLSConfiguration.forClient() - let sslContext = try NIOSSLContext(configuration: tlsConfiguration) - let hostname = (!requiresTLS || host.isIPAddress || host.isEmpty) ? nil : host - let tlsProvider = try NIOSSLClientTLSProvider(context: sslContext, serverHostname: hostname) - return NIOClientTCPBootstrap(self, tls: tlsProvider) + sslContext = eventLoop.makeSucceededFuture(nil) } - } -} -extension NIOClientTCPBootstrap { - /// create a TCP Bootstrap based off what type of `EventLoop` has been passed to the function. - fileprivate static func makeBootstrap( - on eventLoop: EventLoop, - host: String, - requiresTLS: Bool, - configuration: HTTPClient.Configuration - ) throws -> NIOClientTCPBootstrap { - var bootstrap: NIOClientTCPBootstrap - #if canImport(Network) - // if eventLoop is compatible with NIOTransportServices create a NIOTSConnectionBootstrap - if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) { - // if there is a proxy don't create TLS provider as it will be added at a later point - if configuration.proxy != nil { - bootstrap = NIOClientTCPBootstrap(tsBootstrap, tls: NIOInsecureNoTLS()) + let bootstrap = NIOClientTCPBootstrap.makeAndConfigureBootstrap(on: channelEventLoop, + host: key.host, + port: key.port, + requiresTLS: requiresTLS, + configuration: configuration, + sslContextCache: sslContextCache, + logger: logger) + return bootstrap.flatMap { bootstrap -> EventLoopFuture in + let channel: EventLoopFuture + switch key.scheme { + case .http, .https: + let address = HTTPClient.resolveAddress(host: key.host, port: key.port, proxy: configuration.proxy) + channel = bootstrap.connect(host: address.host, port: address.port) + case .unix, .http_unix, .https_unix: + channel = bootstrap.connect(unixDomainSocketPath: key.unixPath) + } + + return channel.flatMap { channel -> EventLoopFuture<(Channel, NIOSSLContext?)> in + sslContext.map { sslContext -> (Channel, NIOSSLContext?) in + (channel, sslContext) + } + }.flatMap { channel, sslContext in + configureChannelPipeline(channel, + isNIOTS: bootstrap.isNIOTS, + sslContext: sslContext, + configuration: configuration, + key: key) + }.flatMapErrorThrowing { error in + if bootstrap.isNIOTS { + throw HTTPClient.NWErrorHandler.translateError(error) } else { - // create NIOClientTCPBootstrap with NIOTS TLS provider - let tlsConfiguration = configuration.tlsConfiguration ?? TLSConfiguration.forClient() - let parameters = tlsConfiguration.getNWProtocolTLSOptions() - let tlsProvider = NIOTSClientTLSProvider(tlsOptions: parameters) - bootstrap = NIOClientTCPBootstrap(tsBootstrap, tls: tlsProvider) + throw error } - } else if let clientBootstrap = ClientBootstrap(validatingGroup: eventLoop) { - bootstrap = try clientBootstrap.makeClientTCPBootstrap(host: host, requiresTLS: requiresTLS, configuration: configuration) - } else { - preconditionFailure("Cannot create bootstrap for the supplied EventLoop") } - #else - if let clientBootstrap = ClientBootstrap(validatingGroup: eventLoop) { - bootstrap = try clientBootstrap.makeClientTCPBootstrap(host: host, requiresTLS: requiresTLS, configuration: configuration) - } else { - preconditionFailure("Cannot create bootstrap for the supplied EventLoop") - } - #endif - - if let timeout = configuration.timeout.connect { - bootstrap = bootstrap.connectTimeout(timeout) - } - - // don't enable TLS if we have a proxy, this will be enabled later on - if requiresTLS, configuration.proxy == nil { - return bootstrap.enableTLS() } - - return bootstrap } - static func makeHTTPClientBootstrapBase( + /// Creates and configures a bootstrap given the `eventLoop`, if TLS/a proxy is being used. + private static func makeAndConfigureBootstrap( on eventLoop: EventLoop, host: String, port: Int, requiresTLS: Bool, - configuration: HTTPClient.Configuration - ) throws -> NIOClientTCPBootstrap { - return try self.makeBootstrap(on: eventLoop, host: host, requiresTLS: requiresTLS, configuration: configuration) - .channelOption(ChannelOptions.socket(SocketOptionLevel(IPPROTO_TCP), TCP_NODELAY), value: 1) - .channelInitializer { channel in - do { - if let proxy = configuration.proxy { - try channel.pipeline.syncAddProxyHandler(host: host, port: port, authorization: proxy.authorization) - } else if requiresTLS { - // We only add the handshake verifier if we need TLS and we're not going through a proxy. If we're going - // through a proxy we add it later. - let completionPromise = channel.eventLoop.makePromise(of: Void.self) - try channel.pipeline.syncOperations.addHandler(TLSEventsHandler(completionPromise: completionPromise), name: TLSEventsHandler.handlerName) + configuration: HTTPClient.Configuration, + sslContextCache: SSLContextCache, + logger: Logger + ) -> EventLoopFuture { + return self.makeBestBootstrap(host: host, + eventLoop: eventLoop, + requiresTLS: requiresTLS, + sslContextCache: sslContextCache, + tlsConfiguration: configuration.tlsConfiguration ?? .forClient(), + useProxy: configuration.proxy != nil, + logger: logger) + .map { bootstrap -> NIOClientTCPBootstrap in + var bootstrap = bootstrap + + if let timeout = configuration.timeout.connect { + bootstrap = bootstrap.connectTimeout(timeout) + } + + // Don't enable TLS if we have a proxy, this will be enabled later on (outside of this method). + if requiresTLS, configuration.proxy == nil { + bootstrap = bootstrap.enableTLS() + } + + return bootstrap.channelInitializer { channel in + do { + if let proxy = configuration.proxy { + try channel.pipeline.syncAddProxyHandler(host: host, + port: port, + authorization: proxy.authorization) + } else if requiresTLS { + // We only add the handshake verifier if we need TLS and we're not going through a proxy. + // If we're going through a proxy we add it later (outside of this method). + let completionPromise = channel.eventLoop.makePromise(of: Void.self) + try channel.pipeline.syncOperations.addHandler(TLSEventsHandler(completionPromise: completionPromise), + name: TLSEventsHandler.handlerName) + } + return channel.eventLoop.makeSucceededVoidFuture() + } catch { + return channel.eventLoop.makeFailedFuture(error) } - return channel.eventLoop.makeSucceededVoidFuture() - } catch { - return channel.eventLoop.makeFailedFuture(error) } } } - static func makeHTTP1Channel(destination: ConnectionPool.Key, eventLoop: EventLoop, configuration: HTTPClient.Configuration, preference: HTTPClient.EventLoopPreference) -> EventLoopFuture { - let channelEventLoop = preference.bestEventLoop ?? eventLoop + /// Creates the best-suited bootstrap given an `EventLoop` and pairs it with an appropriate TLS provider. + private static func makeBestBootstrap( + host: String, + eventLoop: EventLoop, + requiresTLS: Bool, + sslContextCache: SSLContextCache, + tlsConfiguration: TLSConfiguration, + useProxy: Bool, + logger: Logger + ) -> EventLoopFuture { + #if canImport(Network) + // if eventLoop is compatible with NIOTransportServices create a NIOTSConnectionBootstrap + if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) { + // create NIOClientTCPBootstrap with NIOTS TLS provider + let parameters = tlsConfiguration.getNWProtocolTLSOptions() + let tlsProvider = NIOTSClientTLSProvider(tlsOptions: parameters) + return eventLoop.makeSucceededFuture(NIOClientTCPBootstrap(tsBootstrap, tls: tlsProvider)) + } + #endif - let key = destination + if let clientBootstrap = ClientBootstrap(validatingGroup: eventLoop) { + // If there is a proxy don't create TLS provider as it will be added at a later point. + if !requiresTLS || useProxy { + return eventLoop.makeSucceededFuture(NIOClientTCPBootstrap(clientBootstrap, + tls: NIOInsecureNoTLS())) + } else { + return sslContextCache.sslContext(tlsConfiguration: tlsConfiguration, + eventLoop: eventLoop, + logger: logger) + .flatMapThrowing { sslContext in + let hostname = (host.isIPAddress || host.isEmpty) ? nil : host + let tlsProvider = try NIOSSLClientTLSProvider(context: sslContext, serverHostname: hostname) + return NIOClientTCPBootstrap(clientBootstrap, tls: tlsProvider) + } + } + } - let requiresTLS = key.scheme.requiresTLS - let bootstrap: NIOClientTCPBootstrap + preconditionFailure("Cannot create bootstrap for event loop \(eventLoop)") + } +} + +private func configureChannelPipeline(_ channel: Channel, + isNIOTS: Bool, + sslContext: NIOSSLContext?, + configuration: HTTPClient.Configuration, + key: ConnectionPool.Key) -> EventLoopFuture { + let requiresTLS = key.scheme.requiresTLS + let handshakeFuture: EventLoopFuture + + if requiresTLS, configuration.proxy != nil { + let handshakePromise = channel.eventLoop.makePromise(of: Void.self) + channel.pipeline.syncAddLateSSLHandlerIfNeeded(for: key, + sslContext: sslContext!, + handshakePromise: handshakePromise) + handshakeFuture = handshakePromise.futureResult + } else if requiresTLS { do { - bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: channelEventLoop, host: key.host, port: key.port, requiresTLS: requiresTLS, configuration: configuration) + handshakeFuture = try channel.pipeline.syncOperations.handler(type: TLSEventsHandler.self).completionPromise.futureResult } catch { - return channelEventLoop.makeFailedFuture(error) - } - - let channel: EventLoopFuture - switch key.scheme { - case .http, .https: - let address = HTTPClient.resolveAddress(host: key.host, port: key.port, proxy: configuration.proxy) - channel = bootstrap.connect(host: address.host, port: address.port) - case .unix, .http_unix, .https_unix: - channel = bootstrap.connect(unixDomainSocketPath: key.unixPath) + return channel.eventLoop.makeFailedFuture(error) } + } else { + handshakeFuture = channel.eventLoop.makeSucceededVoidFuture() + } - return channel.flatMap { channel in - let requiresTLS = key.scheme.requiresTLS - let requiresLateSSLHandler = configuration.proxy != nil && requiresTLS - let handshakeFuture: EventLoopFuture - - if requiresLateSSLHandler { - let handshakePromise = channel.eventLoop.makePromise(of: Void.self) - channel.pipeline.syncAddLateSSLHandlerIfNeeded(for: key, tlsConfiguration: configuration.tlsConfiguration, handshakePromise: handshakePromise) - handshakeFuture = handshakePromise.futureResult - } else if requiresTLS { - do { - handshakeFuture = try channel.pipeline.syncOperations.handler(type: TLSEventsHandler.self).completionPromise.futureResult - } catch { - return channel.eventLoop.makeFailedFuture(error) - } - } else { - handshakeFuture = channel.eventLoop.makeSucceededVoidFuture() - } - - return handshakeFuture.flatMapThrowing { - let syncOperations = channel.pipeline.syncOperations + return handshakeFuture.flatMapThrowing { + let syncOperations = channel.pipeline.syncOperations - // If we got here and we had a TLSEventsHandler in the pipeline, we can remove it ow. - if requiresTLS { - channel.pipeline.removeHandler(name: TLSEventsHandler.handlerName, promise: nil) - } + // If we got here and we had a TLSEventsHandler in the pipeline, we can remove it ow. + if requiresTLS { + channel.pipeline.removeHandler(name: TLSEventsHandler.handlerName, promise: nil) + } - try syncOperations.addHTTPClientHandlers(leftOverBytesStrategy: .forwardBytes) + try syncOperations.addHTTPClientHandlers(leftOverBytesStrategy: .forwardBytes) - #if canImport(Network) - if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), bootstrap.underlyingBootstrap is NIOTSConnectionBootstrap { - try syncOperations.addHandler(HTTPClient.NWErrorHandler(), position: .first) - } - #endif - - switch configuration.decompression { - case .disabled: - () - case .enabled(let limit): - let decompressHandler = NIOHTTPResponseDecompressor(limit: limit) - try syncOperations.addHandler(decompressHandler) - } + if isNIOTS { + try syncOperations.addHandler(HTTPClient.NWErrorHandler(), position: .first) + } - return channel - } - }.flatMapError { error in - #if canImport(Network) - var error = error - if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), bootstrap.underlyingBootstrap is NIOTSConnectionBootstrap { - error = HTTPClient.NWErrorHandler.translateError(error) - } - #endif - return channelEventLoop.makeFailedFuture(error) + switch configuration.decompression { + case .disabled: + () + case .enabled(let limit): + let decompressHandler = NIOHTTPResponseDecompressor(limit: limit) + try syncOperations.addHandler(decompressHandler) } + + return channel } } @@ -230,3 +264,17 @@ extension Connection { }.recover { _ in } } } + +extension NIOClientTCPBootstrap { + var isNIOTS: Bool { + #if canImport(Network) + if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) { + return self.underlyingBootstrap is NIOTSConnectionBootstrap + } else { + return false + } + #else + return false + #endif + } +} diff --git a/Tests/AsyncHTTPClientTests/ConnectionTests.swift b/Tests/AsyncHTTPClientTests/ConnectionTests.swift index e01cefc99..c1191124c 100644 --- a/Tests/AsyncHTTPClientTests/ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/ConnectionTests.swift @@ -19,6 +19,7 @@ import XCTest class ConnectionTests: XCTestCase { var eventLoop: EmbeddedEventLoop! var http1ConnectionProvider: HTTP1ConnectionProvider! + var pool: ConnectionPool! func buildState(connection: Connection, release: Bool) { XCTAssertTrue(self.http1ConnectionProvider.state.enqueue()) @@ -131,24 +132,30 @@ class ConnectionTests: XCTestCase { } override func setUp() { + XCTAssertNil(self.pool) XCTAssertNil(self.eventLoop) XCTAssertNil(self.http1ConnectionProvider) self.eventLoop = EmbeddedEventLoop() - XCTAssertNoThrow(self.http1ConnectionProvider = try HTTP1ConnectionProvider(key: .init(.init(url: "http://some.test")), - eventLoop: self.eventLoop, - configuration: .init(), - pool: .init(configuration: .init(), - backgroundActivityLogger: HTTPClient.loggingDisabled), - backgroundActivityLogger: HTTPClient.loggingDisabled)) + self.pool = ConnectionPool(configuration: .init(), + backgroundActivityLogger: HTTPClient.loggingDisabled) + XCTAssertNoThrow(self.http1ConnectionProvider = + try HTTP1ConnectionProvider(key: .init(.init(url: "http://some.test")), + eventLoop: self.eventLoop, + configuration: .init(), + pool: self.pool, + backgroundActivityLogger: HTTPClient.loggingDisabled)) } override func tearDown() { + XCTAssertNotNil(self.pool) XCTAssertNotNil(self.eventLoop) XCTAssertNotNil(self.http1ConnectionProvider) XCTAssertNoThrow(try self.http1ConnectionProvider.close().wait()) XCTAssertNoThrow(try self.eventLoop.syncShutdownGracefully()) - self.eventLoop = nil self.http1ConnectionProvider = nil + XCTAssertTrue(try self.pool.close(on: self.eventLoop).wait()) + self.eventLoop = nil + self.pool = nil } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index ce71c5fab..a8d2088d7 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -51,14 +51,15 @@ class HTTPClientNIOTSTests: XCTestCase { func testTLSFailError() { guard isTestingNIOTS() else { return } - #if canImport(Network) - let httpBin = HTTPBin(ssl: true) - let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) - defer { - XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true)) - XCTAssertNoThrow(try httpBin.shutdown()) - } + let httpBin = HTTPBin(ssl: true) + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) + defer { + XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true)) + XCTAssertNoThrow(try httpBin.shutdown()) + } + + #if canImport(Network) do { _ = try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait() XCTFail("This should have failed") @@ -68,6 +69,8 @@ class HTTPClientNIOTSTests: XCTestCase { } catch { XCTFail("Error should have been NWTLSError not \(type(of: error))") } + #else + XCTFail("wrong OS") #endif } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 7e9e2b5d6..98bfb0b54 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -37,6 +37,7 @@ extension HTTPClientTests { ("testPost", testPost), ("testGetHttps", testGetHttps), ("testGetHttpsWithIP", testGetHttpsWithIP), + ("testGetHTTPSWorksOnMTELGWithIP", testGetHTTPSWorksOnMTELGWithIP), ("testPostHttps", testPostHttps), ("testHttpRedirect", testHttpRedirect), ("testHttpHostRedirect", testHttpHostRedirect), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index ed8c1acc5..3e313088d 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -313,6 +313,25 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(.ok, response.status) } + func testGetHTTPSWorksOnMTELGWithIP() throws { + // Same test as above but this one will use NIO on Sockets even on Apple platforms, just to make sure + // this works. + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + } + let localHTTPBin = HTTPBin(ssl: true) + let localClient = HTTPClient(eventLoopGroupProvider: .shared(group), + configuration: HTTPClient.Configuration(certificateVerification: .none)) + defer { + XCTAssertNoThrow(try localClient.syncShutdown()) + XCTAssertNoThrow(try localHTTPBin.shutdown()) + } + + let response = try localClient.get(url: "https://127.0.0.1:\(localHTTPBin.port)/get").wait() + XCTAssertEqual(.ok, response.status) + } + func testPostHttps() throws { let localHTTPBin = HTTPBin(ssl: true) let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), diff --git a/Tests/AsyncHTTPClientTests/LRUCacheTests+XCTest.swift b/Tests/AsyncHTTPClientTests/LRUCacheTests+XCTest.swift new file mode 100644 index 000000000..a0231bf0d --- /dev/null +++ b/Tests/AsyncHTTPClientTests/LRUCacheTests+XCTest.swift @@ -0,0 +1,33 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2018-2019 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// +// +// LRUCacheTests+XCTest.swift +// +import XCTest + +/// +/// NOTE: This file was generated by generate_linux_tests.rb +/// +/// Do NOT edit this file directly as it will be regenerated automatically when needed. +/// + +extension LRUCacheTests { + static var allTests: [(String, (LRUCacheTests) -> () throws -> Void)] { + return [ + ("testBasicsWork", testBasicsWork), + ("testCachesTheRightThings", testCachesTheRightThings), + ("testAppendingTheSameDoesNotEvictButUpdates", testAppendingTheSameDoesNotEvictButUpdates), + ] + } +} diff --git a/Tests/AsyncHTTPClientTests/LRUCacheTests.swift b/Tests/AsyncHTTPClientTests/LRUCacheTests.swift new file mode 100644 index 000000000..6392bcebe --- /dev/null +++ b/Tests/AsyncHTTPClientTests/LRUCacheTests.swift @@ -0,0 +1,83 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +@testable import AsyncHTTPClient +import XCTest + +class LRUCacheTests: XCTestCase { + func testBasicsWork() { + var cache = LRUCache(capacity: 1) + var requestedValueGens = 0 + for i in 0..<10 { + let actual = cache.findOrAppend(key: i) { i in + requestedValueGens += 1 + return i + } + XCTAssertEqual(i, actual) + } + XCTAssertEqual(10, requestedValueGens) + + let nine = cache.findOrAppend(key: 9) { i in + XCTAssertEqual(9, i) + XCTFail("9 should be in the cache") + return -1 + } + XCTAssertEqual(9, nine) + } + + func testCachesTheRightThings() { + var cache = LRUCache(capacity: 3) + + for i in 0..<10 { + let actual = cache.findOrAppend(key: i) { i in + i + } + XCTAssertEqual(i, actual) + + let zero = cache.find(key: 0) + XCTAssertEqual(0, zero, "at \(i), couldn't find 0") + + cache.append(key: -1, value: -1) + XCTAssertEqual(-1, cache.find(key: -1)) + } + + XCTAssertEqual(0, cache.find(key: 0)) + XCTAssertEqual(9, cache.find(key: 9)) + + for i in 1..<9 { + XCTAssertNil(cache.find(key: i)) + } + } + + func testAppendingTheSameDoesNotEvictButUpdates() { + var cache = LRUCache(capacity: 3) + + cache.append(key: 1, value: 1) + cache.append(key: 3, value: 3) + for i in (2...100).reversed() { + cache.append(key: 2, value: i) + XCTAssertEqual(i, cache.find(key: 2)) + } + + for i in 1...3 { + XCTAssertEqual(i, cache.find(key: i)) + } + + cache.append(key: 4, value: 4) + XCTAssertNil(cache.find(key: 1)) + for i in 2...4 { + XCTAssertEqual(i, cache.find(key: i)) + } + } +} diff --git a/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift b/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift new file mode 100644 index 000000000..78a2d5e6f --- /dev/null +++ b/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift @@ -0,0 +1,34 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2018-2019 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// +// +// SSLContextCacheTests+XCTest.swift +// +import XCTest + +/// +/// NOTE: This file was generated by generate_linux_tests.rb +/// +/// Do NOT edit this file directly as it will be regenerated automatically when needed. +/// + +extension SSLContextCacheTests { + static var allTests: [(String, (SSLContextCacheTests) -> () throws -> Void)] { + return [ + ("testJustStartingAndStoppingAContextCacheWorks", testJustStartingAndStoppingAContextCacheWorks), + ("testRequestingSSLContextWorks", testRequestingSSLContextWorks), + ("testRequestingSSLContextAfterShutdownThrows", testRequestingSSLContextAfterShutdownThrows), + ("testCacheWorks", testCacheWorks), + ] + } +} diff --git a/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift b/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift new file mode 100644 index 000000000..980e7f94a --- /dev/null +++ b/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift @@ -0,0 +1,75 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +@testable import AsyncHTTPClient +import NIO +import NIOSSL +import XCTest + +final class SSLContextCacheTests: XCTestCase { + func testJustStartingAndStoppingAContextCacheWorks() { + SSLContextCache().shutdown() + } + + func testRequestingSSLContextWorks() { + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + let eventLoop = group.next() + let cache = SSLContextCache() + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + cache.shutdown() + } + + XCTAssertNoThrow(try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + } + + func testRequestingSSLContextAfterShutdownThrows() { + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + let eventLoop = group.next() + let cache = SSLContextCache() + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + } + + cache.shutdown() + XCTAssertThrowsError(try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + } + + func testCacheWorks() { + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + let eventLoop = group.next() + let cache = SSLContextCache() + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + cache.shutdown() + } + + var firstContext: NIOSSLContext? + var secondContext: NIOSSLContext? + + XCTAssertNoThrow(firstContext = try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + XCTAssertNoThrow(secondContext = try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + XCTAssertNotNil(firstContext) + XCTAssertNotNil(secondContext) + XCTAssert(firstContext === secondContext) + } +} diff --git a/Tests/LinuxMain.swift b/Tests/LinuxMain.swift index c6dfc8291..0db0dd9ce 100644 --- a/Tests/LinuxMain.swift +++ b/Tests/LinuxMain.swift @@ -32,6 +32,8 @@ import XCTest testCase(HTTPClientInternalTests.allTests), testCase(HTTPClientNIOTSTests.allTests), testCase(HTTPClientTests.allTests), + testCase(LRUCacheTests.allTests), testCase(RequestValidationTests.allTests), + testCase(SSLContextCacheTests.allTests), ]) #endif From 06daedfbbd53863f2179c6e3d65eae97dada207b Mon Sep 17 00:00:00 2001 From: Johannes Weiss Date: Wed, 12 May 2021 17:29:30 +0100 Subject: [PATCH 17/23] TLS on Darwin: Add explainer that MTELG supports all options --- .../TLSConfiguration.swift | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift index e1003dd93..6dcebb52a 100644 --- a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift +++ b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift @@ -63,6 +63,13 @@ func getNWProtocolTLSOptions() -> NWProtocolTLS.Options { let options = NWProtocolTLS.Options() + let useMTELGExplainer = """ + You can still use this configuration option on macOS if you initialize HTTPClient \ + with a MultiThreadedEventLoopGroup. Please note that using MultiThreadedEventLoopGroup \ + will make AsyncHTTPClient use NIO on BSD Sockets and not Network.framework (which is the preferred \ + platform networking stack). + """ + // minimum TLS protocol if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { sec_protocol_options_set_min_tls_protocol_version(options.securityProtocolOptions, self.minimumTLSVersion.nwTLSProtocolVersion) @@ -88,7 +95,7 @@ // the certificate chain if self.certificateChain.count > 0 { - preconditionFailure("TLSConfiguration.certificateChain is not supported") + preconditionFailure("TLSConfiguration.certificateChain is not supported. \(useMTELGExplainer)") } // cipher suites @@ -99,12 +106,12 @@ // key log callback if self.keyLogCallback != nil { - preconditionFailure("TLSConfiguration.keyLogCallback is not supported") + preconditionFailure("TLSConfiguration.keyLogCallback is not supported. \(useMTELGExplainer)") } // private key if self.privateKey != nil { - preconditionFailure("TLSConfiguration.privateKey is not supported") + preconditionFailure("TLSConfiguration.privateKey is not supported. \(useMTELGExplainer)") } // renegotiation support key is unsupported @@ -112,7 +119,7 @@ // trust roots if let trustRoots = self.trustRoots { guard case .default = trustRoots else { - preconditionFailure("TLSConfiguration.trustRoots != .default is not supported") + preconditionFailure("TLSConfiguration.trustRoots != .default is not supported. \(useMTELGExplainer)") } } @@ -127,7 +134,8 @@ ) case .noHostnameVerification: - precondition(self.certificateVerification != .noHostnameVerification, "TLSConfiguration.certificateVerification = .noHostnameVerification is not supported") + precondition(self.certificateVerification != .noHostnameVerification, + "TLSConfiguration.certificateVerification = .noHostnameVerification is not supported. \(useMTELGExplainer)") case .fullVerification: break From 9cdf8a01e55a8a66b2210d0a1248060f0964dcd9 Mon Sep 17 00:00:00 2001 From: Adam Fowler Date: Thu, 13 May 2021 13:59:18 +0100 Subject: [PATCH 18/23] Generate trust roots SecCertificate for Transport Services (#350) This PR is a result of another #321. In that PR I provided an alternative structure to TLSConfiguration for when connecting with Transport Services. In this one I construct the NWProtocolTLS.Options from TLSConfiguration. It does mean a little more work for whenever we make a connection, but having spoken to @weissi he doesn't seem to think that is an issue. Also there is no method to create a SecIdentity at the moment. We need to generate a pkcs#12 from the certificate chain and private key, which can then be used to create the SecIdentity. This should resolve #292 --- .dockerignore | 2 + .../TLSConfiguration.swift | 88 +++++++++++++++---- Sources/AsyncHTTPClient/Utils.swift | 8 +- .../HTTPClientNIOTSTests+XCTest.swift | 1 + .../HTTPClientNIOTSTests.swift | 15 ++++ 5 files changed, 93 insertions(+), 21 deletions(-) create mode 100644 .dockerignore diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 000000000..2fb33437e --- /dev/null +++ b/.dockerignore @@ -0,0 +1,2 @@ +.build +.git \ No newline at end of file diff --git a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift index 6dcebb52a..86867c6f9 100644 --- a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift +++ b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift @@ -16,6 +16,7 @@ import Foundation import Network + import NIO import NIOSSL import NIOTransportServices @@ -58,9 +59,25 @@ /// create NWProtocolTLS.Options for use with NIOTransportServices from the NIOSSL TLSConfiguration /// - /// - Parameter queue: Dispatch queue to run `sec_protocol_options_set_verify_block` on. + /// - Parameter eventLoop: EventLoop to wait for creation of options on + /// - Returns: Future holding NWProtocolTLS Options + func getNWProtocolTLSOptions(on eventLoop: EventLoop) -> EventLoopFuture { + let promise = eventLoop.makePromise(of: NWProtocolTLS.Options.self) + Self.tlsDispatchQueue.async { + do { + let options = try self.getNWProtocolTLSOptions() + promise.succeed(options) + } catch { + promise.fail(error) + } + } + return promise.futureResult + } + + /// create NWProtocolTLS.Options for use with NIOTransportServices from the NIOSSL TLSConfiguration + /// /// - Returns: Equivalent NWProtocolTLS Options - func getNWProtocolTLSOptions() -> NWProtocolTLS.Options { + func getNWProtocolTLSOptions() throws -> NWProtocolTLS.Options { let options = NWProtocolTLS.Options() let useMTELGExplainer = """ @@ -109,6 +126,11 @@ preconditionFailure("TLSConfiguration.keyLogCallback is not supported. \(useMTELGExplainer)") } + // the certificate chain + if self.certificateChain.count > 0 { + preconditionFailure("TLSConfiguration.certificateChain is not supported. \(useMTELGExplainer)") + } + // private key if self.privateKey != nil { preconditionFailure("TLSConfiguration.privateKey is not supported. \(useMTELGExplainer)") @@ -117,30 +139,60 @@ // renegotiation support key is unsupported // trust roots - if let trustRoots = self.trustRoots { - guard case .default = trustRoots else { - preconditionFailure("TLSConfiguration.trustRoots != .default is not supported. \(useMTELGExplainer)") + var secTrustRoots: [SecCertificate]? + switch trustRoots { + case .some(.certificates(let certificates)): + secTrustRoots = try certificates.compactMap { certificate in + try SecCertificateCreateWithData(nil, Data(certificate.toDERBytes()) as CFData) + } + case .some(.file(let file)): + let certificates = try NIOSSLCertificate.fromPEMFile(file) + secTrustRoots = try certificates.compactMap { certificate in + try SecCertificateCreateWithData(nil, Data(certificate.toDERBytes()) as CFData) } + + case .some(.default), .none: + break } - switch self.certificateVerification { - case .none: + precondition(self.certificateVerification != .noHostnameVerification, + "TLSConfiguration.certificateVerification = .noHostnameVerification is not supported. \(useMTELGExplainer)") + + if certificateVerification != .fullVerification || trustRoots != nil { // add verify block to control certificate verification sec_protocol_options_set_verify_block( options.securityProtocolOptions, - { _, _, sec_protocol_verify_complete in - sec_protocol_verify_complete(true) - }, TLSConfiguration.tlsDispatchQueue + { _, sec_trust, sec_protocol_verify_complete in + guard self.certificateVerification != .none else { + sec_protocol_verify_complete(true) + return + } + + let trust = sec_trust_copy_ref(sec_trust).takeRetainedValue() + if let trustRootCertificates = secTrustRoots { + SecTrustSetAnchorCertificates(trust, trustRootCertificates as CFArray) + } + if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { + dispatchPrecondition(condition: .onQueue(Self.tlsDispatchQueue)) + SecTrustEvaluateAsyncWithError(trust, Self.tlsDispatchQueue) { _, result, error in + if let error = error { + print("Trust failed: \(error.localizedDescription)") + } + sec_protocol_verify_complete(result) + } + } else { + SecTrustEvaluateAsync(trust, Self.tlsDispatchQueue) { _, result in + switch result { + case .proceed, .unspecified: + sec_protocol_verify_complete(true) + default: + sec_protocol_verify_complete(false) + } + } + } + }, Self.tlsDispatchQueue ) - - case .noHostnameVerification: - precondition(self.certificateVerification != .noHostnameVerification, - "TLSConfiguration.certificateVerification = .noHostnameVerification is not supported. \(useMTELGExplainer)") - - case .fullVerification: - break } - return options } } diff --git a/Sources/AsyncHTTPClient/Utils.swift b/Sources/AsyncHTTPClient/Utils.swift index 1af7899b2..6069222b1 100644 --- a/Sources/AsyncHTTPClient/Utils.swift +++ b/Sources/AsyncHTTPClient/Utils.swift @@ -180,9 +180,11 @@ extension NIOClientTCPBootstrap { // if eventLoop is compatible with NIOTransportServices create a NIOTSConnectionBootstrap if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *), let tsBootstrap = NIOTSConnectionBootstrap(validatingGroup: eventLoop) { // create NIOClientTCPBootstrap with NIOTS TLS provider - let parameters = tlsConfiguration.getNWProtocolTLSOptions() - let tlsProvider = NIOTSClientTLSProvider(tlsOptions: parameters) - return eventLoop.makeSucceededFuture(NIOClientTCPBootstrap(tsBootstrap, tls: tlsProvider)) + return tlsConfiguration.getNWProtocolTLSOptions(on: eventLoop) + .map { parameters in + let tlsProvider = NIOTSClientTLSProvider(tlsOptions: parameters) + return NIOClientTCPBootstrap(tsBootstrap, tls: tlsProvider) + } } #endif diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests+XCTest.swift index c0d86085f..cc33f6aee 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests+XCTest.swift @@ -29,6 +29,7 @@ extension HTTPClientNIOTSTests { ("testTLSFailError", testTLSFailError), ("testConnectionFailError", testConnectionFailError), ("testTLSVersionError", testTLSVersionError), + ("testTrustRootCertificateLoadFail", testTrustRootCertificateLoadFail), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index a8d2088d7..9a0070229 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -111,4 +111,19 @@ class HTTPClientNIOTSTests: XCTestCase { } #endif } + + func testTrustRootCertificateLoadFail() { + guard isTestingNIOTS() else { return } + #if canImport(Network) + let tlsConfig = TLSConfiguration.forClient(trustRoots: .file("not/a/certificate")) + XCTAssertThrowsError(try tlsConfig.getNWProtocolTLSOptions()) { error in + switch error { + case let error as NIOSSL.NIOSSLError where error == .failedToLoadCertificate: + break + default: + XCTFail("\(error)") + } + } + #endif + } } From 8ccba7328d178ac05a1a9803cf3f2c6660d2f826 Mon Sep 17 00:00:00 2001 From: Johannes Weiss Date: Thu, 13 May 2021 15:11:49 +0100 Subject: [PATCH 19/23] SSLContextCache: use DispatchQueue instead of NIOThreadPool (#368) Motivation: In the vast majority of cases, we'll only ever create one and only one `NIOSSLContext`. It's therefore wasteful to keep around a whole thread doing nothing just for that. A `DispatchQueue` is absolutely fine here. Modification: Run the `NIOSSLContext` creation on a `DispatchQueue` instead. Result: Fewer threads hanging around. --- Sources/AsyncHTTPClient/ConnectionPool.swift | 2 - Sources/AsyncHTTPClient/SSLContextCache.swift | 53 ++----------------- .../SSLContextCacheTests+XCTest.swift | 3 +- .../SSLContextCacheTests.swift | 32 ++++++----- 4 files changed, 22 insertions(+), 68 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index 4aecf6d17..8845f9709 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -109,8 +109,6 @@ final class ConnectionPool { self.providers.values } - self.sslContextCache.shutdown() - return EventLoopFuture.reduce(true, providers.map { $0.close() }, on: eventLoop) { $0 && $1 } } diff --git a/Sources/AsyncHTTPClient/SSLContextCache.swift b/Sources/AsyncHTTPClient/SSLContextCache.swift index 582d2cee8..005c475de 100644 --- a/Sources/AsyncHTTPClient/SSLContextCache.swift +++ b/Sources/AsyncHTTPClient/SSLContextCache.swift @@ -12,69 +12,22 @@ // //===----------------------------------------------------------------------===// +import Dispatch import Logging import NIO import NIOConcurrencyHelpers import NIOSSL class SSLContextCache { - private var state = State.activeNoThread private let lock = Lock() private var sslContextCache = LRUCache() - private let threadPool = NIOThreadPool(numberOfThreads: 1) - - enum State { - case activeNoThread - case active - case shutDown - } - - init() {} - - func shutdown() { - self.lock.withLock { () -> Void in - switch self.state { - case .activeNoThread: - self.state = .shutDown - case .active: - self.state = .shutDown - self.threadPool.shutdownGracefully { maybeError in - precondition(maybeError == nil, "\(maybeError!)") - } - case .shutDown: - preconditionFailure("SSLContextCache shut down twice") - } - } - } - - deinit { - assert(self.state == .shutDown) - } + private let offloadQueue = DispatchQueue(label: "io.github.swift-server.AsyncHTTPClient.SSLContextCache") } extension SSLContextCache { - private struct SSLContextCacheShutdownError: Error {} - func sslContext(tlsConfiguration: TLSConfiguration, eventLoop: EventLoop, logger: Logger) -> EventLoopFuture { - let earlyExitError: Error? = self.lock.withLock { () -> Error? in - switch self.state { - case .activeNoThread: - self.state = .active - self.threadPool.start() - return nil - case .active: - return nil - case .shutDown: - return SSLContextCacheShutdownError() - } - } - - if let error = earlyExitError { - return eventLoop.makeFailedFuture(error) - } - let eqTLSConfiguration = BestEffortHashableTLSConfiguration(wrapping: tlsConfiguration) let sslContext = self.lock.withLock { self.sslContextCache.find(key: eqTLSConfiguration) @@ -88,7 +41,7 @@ extension SSLContextCache { logger.debug("creating new SSL context", metadata: ["ahc-tls-config": "\(tlsConfiguration)"]) - let newSSLContext = self.threadPool.runIfActive(eventLoop: eventLoop) { + let newSSLContext = self.offloadQueue.asyncWithFuture(eventLoop: eventLoop) { try NIOSSLContext(configuration: tlsConfiguration) } diff --git a/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift b/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift index 78a2d5e6f..d98f5a853 100644 --- a/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/SSLContextCacheTests+XCTest.swift @@ -25,10 +25,9 @@ import XCTest extension SSLContextCacheTests { static var allTests: [(String, (SSLContextCacheTests) -> () throws -> Void)] { return [ - ("testJustStartingAndStoppingAContextCacheWorks", testJustStartingAndStoppingAContextCacheWorks), ("testRequestingSSLContextWorks", testRequestingSSLContextWorks), - ("testRequestingSSLContextAfterShutdownThrows", testRequestingSSLContextAfterShutdownThrows), ("testCacheWorks", testCacheWorks), + ("testCacheDoesNotReturnWrongEntry", testCacheDoesNotReturnWrongEntry), ] } } diff --git a/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift b/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift index 980e7f94a..d44d65432 100644 --- a/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift +++ b/Tests/AsyncHTTPClientTests/SSLContextCacheTests.swift @@ -18,17 +18,12 @@ import NIOSSL import XCTest final class SSLContextCacheTests: XCTestCase { - func testJustStartingAndStoppingAContextCacheWorks() { - SSLContextCache().shutdown() - } - func testRequestingSSLContextWorks() { let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) let eventLoop = group.next() let cache = SSLContextCache() defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) - cache.shutdown() } XCTAssertNoThrow(try cache.sslContext(tlsConfiguration: .forClient(), @@ -36,7 +31,7 @@ final class SSLContextCacheTests: XCTestCase { logger: HTTPClient.loggingDisabled).wait()) } - func testRequestingSSLContextAfterShutdownThrows() { + func testCacheWorks() { let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) let eventLoop = group.next() let cache = SSLContextCache() @@ -44,19 +39,26 @@ final class SSLContextCacheTests: XCTestCase { XCTAssertNoThrow(try group.syncShutdownGracefully()) } - cache.shutdown() - XCTAssertThrowsError(try cache.sslContext(tlsConfiguration: .forClient(), - eventLoop: eventLoop, - logger: HTTPClient.loggingDisabled).wait()) + var firstContext: NIOSSLContext? + var secondContext: NIOSSLContext? + + XCTAssertNoThrow(firstContext = try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + XCTAssertNoThrow(secondContext = try cache.sslContext(tlsConfiguration: .forClient(), + eventLoop: eventLoop, + logger: HTTPClient.loggingDisabled).wait()) + XCTAssertNotNil(firstContext) + XCTAssertNotNil(secondContext) + XCTAssert(firstContext === secondContext) } - func testCacheWorks() { + func testCacheDoesNotReturnWrongEntry() { let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) let eventLoop = group.next() let cache = SSLContextCache() defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) - cache.shutdown() } var firstContext: NIOSSLContext? @@ -65,11 +67,13 @@ final class SSLContextCacheTests: XCTestCase { XCTAssertNoThrow(firstContext = try cache.sslContext(tlsConfiguration: .forClient(), eventLoop: eventLoop, logger: HTTPClient.loggingDisabled).wait()) - XCTAssertNoThrow(secondContext = try cache.sslContext(tlsConfiguration: .forClient(), + + // Second one has a _different_ TLSConfiguration. + XCTAssertNoThrow(secondContext = try cache.sslContext(tlsConfiguration: .forClient(certificateVerification: .none), eventLoop: eventLoop, logger: HTTPClient.loggingDisabled).wait()) XCTAssertNotNil(firstContext) XCTAssertNotNil(secondContext) - XCTAssert(firstContext === secondContext) + XCTAssert(firstContext !== secondContext) } } From cddb69d7d4980c428ea6d5eafb32fb76c151cda8 Mon Sep 17 00:00:00 2001 From: Johannes Weiss Date: Fri, 14 May 2021 16:31:08 +0100 Subject: [PATCH 20/23] fix Swift Package Index build (#369) Co-authored-by: Johannes Weiss --- .../HTTPClientNIOTSTests.swift | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index 9a0070229..71d2b312f 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -115,14 +115,19 @@ class HTTPClientNIOTSTests: XCTestCase { func testTrustRootCertificateLoadFail() { guard isTestingNIOTS() else { return } #if canImport(Network) - let tlsConfig = TLSConfiguration.forClient(trustRoots: .file("not/a/certificate")) - XCTAssertThrowsError(try tlsConfig.getNWProtocolTLSOptions()) { error in - switch error { - case let error as NIOSSL.NIOSSLError where error == .failedToLoadCertificate: - break - default: - XCTFail("\(error)") + if #available(macOS 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) { + let tlsConfig = TLSConfiguration.forClient(trustRoots: .file("not/a/certificate")) + + XCTAssertThrowsError(try tlsConfig.getNWProtocolTLSOptions()) { error in + switch error { + case let error as NIOSSL.NIOSSLError where error == .failedToLoadCertificate: + break + default: + XCTFail("\(error)") + } } + } else { + XCTFail("should be impossible") } #endif } From ddc53041d2d2232295a5022a671fb47440719eae Mon Sep 17 00:00:00 2001 From: Moritz Lang Date: Sat, 5 Dec 2020 17:35:48 +0100 Subject: [PATCH 21/23] Inject instrumentation metadata into HTTP headers Motivation: In order to instrument distributed systems, metadata such as trace ids must be propagated across network boundaries. As HTTPClient operates at one such boundary, it should take care of injecting metadata into HTTP headers automatically using the configured instrument. Modifications: HTTPClient gains new method overloads accepting LoggingContext. Result: - New HTTPClient method overloads accepting LoggingContext - Existing overloads accepting Logger construct a DefaultLoggingContext - Existing methods that neither take Logger nor LoggingContext construct a DefaultLoggingContext --- Package.swift | 3 +- Sources/AsyncHTTPClient/HTTPClient.swift | 157 ++++++++++++++++-- .../AsyncHTTPClient/HTTPHeadersInjector.swift | 25 +++ .../HTTPClientTestUtils.swift | 48 ++++++ .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 26 +++ 6 files changed, 249 insertions(+), 11 deletions(-) create mode 100644 Sources/AsyncHTTPClient/HTTPHeadersInjector.swift diff --git a/Package.swift b/Package.swift index f2e606a93..d0325bdb5 100644 --- a/Package.swift +++ b/Package.swift @@ -26,12 +26,13 @@ let package = Package( .package(url: "https://github.com/apple/swift-nio-extras.git", from: "1.3.0"), .package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.5.1"), .package(url: "https://github.com/apple/swift-log.git", from: "1.4.0"), + .package(url: "https://github.com/apple/swift-distributed-tracing.git", from: "0.1.1"), ], targets: [ .target( name: "AsyncHTTPClient", dependencies: ["NIO", "NIOHTTP1", "NIOSSL", "NIOConcurrencyHelpers", "NIOHTTPCompression", - "NIOFoundationCompat", "NIOTransportServices", "Logging"] + "NIOFoundationCompat", "NIOTransportServices", "Logging", "Instrumentation"] ), .testTarget( name: "AsyncHTTPClientTests", diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index ec549d993..602fcbbad 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -12,7 +12,9 @@ // //===----------------------------------------------------------------------===// +import Baggage import Foundation +import Instrumentation import Logging import NIO import NIOConcurrencyHelpers @@ -71,6 +73,7 @@ public class HTTPClient { private let stateLock = Lock() internal static let loggingDisabled = Logger(label: "AHC-do-not-log", factory: { _ in SwiftLogNoOpLogHandler() }) + internal static let loggingDisabledTopLevelContext = DefaultLoggingContext.topLevel(logger: loggingDisabled) /// Create an `HTTPClient` with specified `EventLoopGroup` provider and configuration. /// @@ -227,7 +230,17 @@ public class HTTPClient { /// - url: Remote URL. /// - deadline: Point in time by which the request must complete. public func get(url: String, deadline: NIODeadline? = nil) -> EventLoopFuture { - return self.get(url: url, deadline: deadline, logger: HTTPClient.loggingDisabled) + return self.get(url: url, context: HTTPClient.loggingDisabledTopLevelContext, deadline: deadline) + } + + /// Execute `GET` request using specified URL. + /// + /// - parameters: + /// - url: Remote URL. + /// - context: The logging context carrying a logger and instrumentation metadata. + /// - deadline: Point in time by which the request must complete. + public func get(url: String, context: LoggingContext, deadline: NIODeadline? = nil) -> EventLoopFuture { + return self.execute(.GET, url: url, context: context, deadline: deadline) } /// Execute `GET` request using specified URL. @@ -237,7 +250,7 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func get(url: String, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - return self.execute(.GET, url: url, deadline: deadline, logger: logger) + return self.get(url: url, context: DefaultLoggingContext.topLevel(logger: logger), deadline: deadline) } /// Execute `POST` request using specified URL. @@ -247,7 +260,18 @@ public class HTTPClient { /// - body: Request body. /// - deadline: Point in time by which the request must complete. public func post(url: String, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { - return self.post(url: url, body: body, deadline: deadline, logger: HTTPClient.loggingDisabled) + return self.post(url: url, context: HTTPClient.loggingDisabledTopLevelContext, body: body, deadline: deadline) + } + + /// Execute `POST` request using specified URL. + /// + /// - parameters: + /// - url: Remote URL. + /// - context: The logging context carrying a logger and instrumentation metadata. + /// - body: Request body. + /// - deadline: Point in time by which the request must complete. + public func post(url: String, context: LoggingContext, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { + return self.execute(.POST, url: url, context: context, body: body, deadline: deadline) } /// Execute `POST` request using specified URL. @@ -258,7 +282,12 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func post(url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - return self.execute(.POST, url: url, body: body, deadline: deadline, logger: logger) + return self.post( + url: url, + context: DefaultLoggingContext.topLevel(logger: logger), + body: body, + deadline: deadline + ) } /// Execute `PATCH` request using specified URL. @@ -268,7 +297,18 @@ public class HTTPClient { /// - body: Request body. /// - deadline: Point in time by which the request must complete. public func patch(url: String, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { - return self.patch(url: url, body: body, deadline: deadline, logger: HTTPClient.loggingDisabled) + return self.patch(url: url, context: HTTPClient.loggingDisabledTopLevelContext, body: body, deadline: deadline) + } + + /// Execute `PATCH` request using specified URL. + /// + /// - parameters: + /// - url: Remote URL. + /// - context: The logging context carrying a logger and instrumentation metadata. + /// - body: Request body. + /// - deadline: Point in time by which the request must complete. + public func patch(url: String, context: LoggingContext, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { + return self.execute(.PATCH, url: url, context: context, body: body, deadline: deadline) } /// Execute `PATCH` request using specified URL. @@ -279,7 +319,12 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func patch(url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - return self.execute(.PATCH, url: url, body: body, deadline: deadline, logger: logger) + return self.patch( + url: url, + context: DefaultLoggingContext.topLevel(logger: logger), + body: body, + deadline: deadline + ) } /// Execute `PUT` request using specified URL. @@ -289,7 +334,18 @@ public class HTTPClient { /// - body: Request body. /// - deadline: Point in time by which the request must complete. public func put(url: String, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { - return self.put(url: url, body: body, deadline: deadline, logger: HTTPClient.loggingDisabled) + return self.put(url: url, context: HTTPClient.loggingDisabledTopLevelContext, body: body, deadline: deadline) + } + + /// Execute `PUT` request using specified URL. + /// + /// - parameters: + /// - url: Remote URL. + /// - context: The logging context carrying a logger and instrumentation metadata. + /// - body: Request body. + /// - deadline: Point in time by which the request must complete. + public func put(url: String, context: LoggingContext, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { + return self.execute(.PUT, url: url, context: context, body: body, deadline: deadline) } /// Execute `PUT` request using specified URL. @@ -300,7 +356,12 @@ public class HTTPClient { /// - deadline: Point in time by which the request must complete. /// - logger: The logger to use for this request. public func put(url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - return self.execute(.PUT, url: url, body: body, deadline: deadline, logger: logger) + return self.put( + url: url, + context: DefaultLoggingContext.topLevel(logger: logger), + body: body, + deadline: deadline + ) } /// Execute `DELETE` request using specified URL. @@ -309,7 +370,17 @@ public class HTTPClient { /// - url: Remote URL. /// - deadline: The time when the request must have been completed by. public func delete(url: String, deadline: NIODeadline? = nil) -> EventLoopFuture { - return self.delete(url: url, deadline: deadline, logger: HTTPClient.loggingDisabled) + return self.delete(url: url, context: HTTPClient.loggingDisabledTopLevelContext, deadline: deadline) + } + + /// Execute `DELETE` request using specified URL. + /// + /// - parameters: + /// - url: Remote URL. + /// - context: The logging context carrying a logger and instrumentation metadata. + /// - deadline: Point in time by which the request must complete. + public func delete(url: String, context: LoggingContext, deadline: NIODeadline? = nil) -> EventLoopFuture { + return self.execute(.DELETE, url: url, context: context, deadline: deadline) } /// Execute `DELETE` request using specified URL. @@ -319,7 +390,24 @@ public class HTTPClient { /// - deadline: The time when the request must have been completed by. /// - logger: The logger to use for this request. public func delete(url: String, deadline: NIODeadline? = nil, logger: Logger) -> EventLoopFuture { - return self.execute(.DELETE, url: url, deadline: deadline, logger: logger) + return self.delete(url: url, context: DefaultLoggingContext.topLevel(logger: logger), deadline: deadline) + } + + /// Execute arbitrary HTTP request using specified URL. + /// + /// - parameters: + /// - method: Request method. + /// - url: Request url. + /// - body: Request body. + /// - deadline: Point in time by which the request must complete. + /// - context: The logging context carrying a logger and instrumentation metadata. + public func execute(_ method: HTTPMethod = .GET, url: String, context: LoggingContext?, body: Body? = nil, deadline: NIODeadline? = nil) -> EventLoopFuture { + do { + let request = try Request(url: url, method: method, body: body) + return self.execute(request: request, context: context, deadline: deadline) + } catch { + return self.eventLoopGroup.next().makeFailedFuture(error) + } } /// Execute arbitrary HTTP request using specified URL. @@ -390,6 +478,17 @@ public class HTTPClient { return self.execute(request: request, deadline: deadline, logger: HTTPClient.loggingDisabled) } + /// Execute arbitrary HTTP request using specified URL. + /// + /// - parameters: + /// - request: HTTP request to execute. + /// - context: The logging context carrying a logger and instrumentation metadata. + /// - deadline: Point in time by which the request must complete. + public func execute(request: Request, context: LoggingContext?, deadline: NIODeadline? = nil) -> EventLoopFuture { + let accumulator = ResponseAccumulator(request: request) + return self.execute(request: request, delegate: accumulator, context: context, deadline: deadline).futureResult + } + /// Execute arbitrary HTTP request using specified URL. /// /// - parameters: @@ -441,6 +540,20 @@ public class HTTPClient { return self.execute(request: request, delegate: delegate, deadline: deadline, logger: HTTPClient.loggingDisabled) } + /// Execute arbitrary HTTP request and handle response processing using provided delegate. + /// + /// - parameters: + /// - request: HTTP request to execute. + /// - delegate: Delegate to process response parts. + /// - deadline: Point in time by which the request must complete. + /// - context: The logging context carrying a logger and instrumentation metadata. + public func execute(request: Request, + delegate: Delegate, + context: LoggingContext?, + deadline: NIODeadline? = nil) -> Task { + return self.execute(request: request, delegate: delegate, eventLoop: .indifferent, context: context, deadline: deadline) + } + /// Execute arbitrary HTTP request and handle response processing using provided delegate. /// /// - parameters: @@ -474,6 +587,30 @@ public class HTTPClient { logger: HTTPClient.loggingDisabled) } + /// Execute arbitrary HTTP request and handle response processing using provided delegate. + /// + /// - parameters: + /// - request: HTTP request to execute. + /// - delegate: Delegate to process response parts. + /// - eventLoop: NIO Event Loop preference. + /// - deadline: Point in time by which the request must complete. + /// - context: The logging context carrying a logger and instrumentation metadata. + public func execute(request: Request, + delegate: Delegate, + eventLoop eventLoopPreference: EventLoopPreference, + context: LoggingContext?, + deadline: NIODeadline? = nil) -> Task { + var request = request + if let baggage = context?.baggage { + InstrumentationSystem.instrument.inject(baggage, into: &request.headers, using: HTTPHeadersInjector()) + } + return self.execute(request: request, + delegate: delegate, + eventLoop: eventLoopPreference, + deadline: deadline, + logger: context?.logger) + } + /// Execute arbitrary HTTP request and handle response processing using provided delegate. /// /// - parameters: diff --git a/Sources/AsyncHTTPClient/HTTPHeadersInjector.swift b/Sources/AsyncHTTPClient/HTTPHeadersInjector.swift new file mode 100644 index 000000000..bbc0951ba --- /dev/null +++ b/Sources/AsyncHTTPClient/HTTPHeadersInjector.swift @@ -0,0 +1,25 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2020 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +import Instrumentation +import NIOHTTP1 + +/// Injects values into `NIOHTTP1.HTTPHeaders`. +struct HTTPHeadersInjector: Injector { + init() {} + + func inject(_ value: String, forKey key: String, into headers: inout HTTPHeaders) { + headers.replaceOrAdd(name: key, value: value) + } +} diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index f10d99677..678c947f9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -13,7 +13,9 @@ //===----------------------------------------------------------------------===// import AsyncHTTPClient +import CoreBaggage import Foundation +import Instrumentation import Logging import NIO import NIOConcurrencyHelpers @@ -995,6 +997,52 @@ class HTTPEchoHandler: ChannelInboundHandler { } } +internal enum TestInstrumentIDKey: Baggage.Key { + typealias Value = String + static let nameOverride: String? = "instrumentation-test-id" + static let headerName = "x-instrumentation-test-id" +} + +internal extension Baggage { + var testInstrumentID: String? { + get { + return self[TestInstrumentIDKey.self] + } + set { + self[TestInstrumentIDKey.self] = newValue + } + } +} + +internal final class TestInstrument: Instrument { + private(set) var carrierAfterInjection: Any? + + func inject( + _ baggage: Baggage, + into carrier: inout Carrier, + using injector: Inject + ) where Carrier == Inject.Carrier, Inject: Injector { + if let testID = baggage.testInstrumentID { + injector.inject(testID, forKey: TestInstrumentIDKey.headerName, into: &carrier) + self.carrierAfterInjection = carrier + } + } + + func extract( + _ carrier: Carrier, + into baggage: inout Baggage, + using extractor: Extract + ) where Carrier == Extract.Carrier, Extract: Extractor { + // no-op + } +} + +internal extension InstrumentationSystem { + static var testInstrument: TestInstrument? { + InstrumentationSystem.instrument as? TestInstrument + } +} + private let cert = """ -----BEGIN CERTIFICATE----- MIICmDCCAYACCQCPC8JDqMh1zzANBgkqhkiG9w0BAQsFADANMQswCQYDVQQGEwJ1 diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 98bfb0b54..ba44574e1 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -117,6 +117,7 @@ extension HTTPClientTests { ("testLoggingCorrectlyAttachesRequestInformation", testLoggingCorrectlyAttachesRequestInformation), ("testNothingIsLoggedAtInfoOrHigher", testNothingIsLoggedAtInfoOrHigher), ("testAllMethodsLog", testAllMethodsLog), + ("testRequestWithBaggage", testRequestWithBaggage), ("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground), ("testUploadStreamingNoLength", testUploadStreamingNoLength), ("testConnectErrorPropagatedToDelegate", testConnectErrorPropagatedToDelegate), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 3e313088d..99bc0e083 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -16,6 +16,8 @@ #if canImport(Network) import Network #endif +import Baggage +@testable import Instrumentation // @testable to access `bootstrapInternal` import Logging import NIO import NIOConcurrencyHelpers @@ -79,6 +81,8 @@ class HTTPClientTests: XCTestCase { XCTAssertNotNil(self.backgroundLogStore) self.backgroundLogStore = nil + + InstrumentationSystem.bootstrapInternal(nil) } func testRequestURI() throws { @@ -2435,6 +2439,28 @@ class HTTPClientTests: XCTestCase { }) } + func testRequestWithBaggage() throws { + InstrumentationSystem.bootstrapInternal(TestInstrument()) + let logStore = CollectEverythingLogHandler.LogStore() + var logger = Logger(label: #function, factory: { _ in + CollectEverythingLogHandler(logStore: logStore) + }) + logger.logLevel = .trace + var baggage = Baggage.topLevel + baggage.testInstrumentID = "test" + let context = DefaultLoggingContext(logger: logger, baggage: baggage) + let request = try Request(url: self.defaultHTTPBinURLPrefix + "get") + let response = try self.defaultClient.execute(request: request, context: context).wait() + XCTAssertEqual(.ok, response.status) + XCTAssert( + logStore.allEntries.allSatisfy { entry in + entry.metadata.contains(where: { $0.key == TestInstrumentIDKey.nameOverride && $0.value == "test" }) + } + ) + let headers = try XCTUnwrap(InstrumentationSystem.testInstrument?.carrierAfterInjection as? HTTPHeaders) + XCTAssertEqual(headers, [TestInstrumentIDKey.headerName: "test"]) + } + func testClosingIdleConnectionsInPoolLogsInTheBackground() { XCTAssertNoThrow(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "/get").wait()) From 59b61ebbe7fbf1bf51dbbaa72b60dd4ed749b569 Mon Sep 17 00:00:00 2001 From: Moritz Lang Date: Tue, 25 May 2021 14:33:35 +0200 Subject: [PATCH 22/23] Trace request execution --- Package.swift | 3 +- Sources/AsyncHTTPClient/Connection.swift | 27 +++--- Sources/AsyncHTTPClient/ConnectionPool.swift | 81 ++++++++-------- Sources/AsyncHTTPClient/HTTPClient.swift | 94 +++++++++++++++---- Sources/AsyncHTTPClient/HTTPHandler.swift | 28 ++++-- Sources/AsyncHTTPClient/SSLContextCache.swift | 7 +- Sources/AsyncHTTPClient/Utils.swift | 15 +-- 7 files changed, 161 insertions(+), 94 deletions(-) diff --git a/Package.swift b/Package.swift index d0325bdb5..4b5a297b8 100644 --- a/Package.swift +++ b/Package.swift @@ -32,7 +32,8 @@ let package = Package( .target( name: "AsyncHTTPClient", dependencies: ["NIO", "NIOHTTP1", "NIOSSL", "NIOConcurrencyHelpers", "NIOHTTPCompression", - "NIOFoundationCompat", "NIOTransportServices", "Logging", "Instrumentation"] + "NIOFoundationCompat", "NIOTransportServices", "Logging", "Instrumentation", + "Tracing", "TracingOpenTelemetrySupport"] ), .testTarget( name: "AsyncHTTPClientTests", diff --git a/Sources/AsyncHTTPClient/Connection.swift b/Sources/AsyncHTTPClient/Connection.swift index 6542ca20a..55dc625ee 100644 --- a/Sources/AsyncHTTPClient/Connection.swift +++ b/Sources/AsyncHTTPClient/Connection.swift @@ -12,6 +12,7 @@ // //===----------------------------------------------------------------------===// +import Baggage import Foundation import Logging import NIO @@ -51,21 +52,21 @@ extension Connection { /// Release this `Connection` to its associated `HTTP1ConnectionProvider`. /// /// - Warning: This only releases the connection and doesn't take care of cleaning handlers in the `Channel` pipeline. - func release(closing: Bool, logger: Logger) { + func release(closing: Bool, context: LoggingContext) { self.channel.eventLoop.assertInEventLoop() - self.provider.release(connection: self, closing: closing, logger: logger) + self.provider.release(connection: self, closing: closing, context: context) } /// Called when channel exceeds idle time in pool. - func timeout(logger: Logger) { + func timeout(context: LoggingContext) { self.channel.eventLoop.assertInEventLoop() - self.provider.timeout(connection: self, logger: logger) + self.provider.timeout(connection: self, context: context) } /// Called when channel goes inactive while in the pool. - func remoteClosed(logger: Logger) { + func remoteClosed(context: LoggingContext) { self.channel.eventLoop.assertInEventLoop() - self.provider.remoteClosed(connection: self, logger: logger) + self.provider.remoteClosed(connection: self, context: context) } /// Called from `HTTP1ConnectionProvider.close` when client is shutting down. @@ -104,9 +105,9 @@ extension Connection: PoolManageableConnection { extension Connection { /// Sets idle timeout handler and channel inactivity listener. - func setIdleTimeout(timeout: TimeAmount?, logger: Logger) { + func setIdleTimeout(timeout: TimeAmount?, context: LoggingContext) { _ = self.channel.pipeline.addHandler(IdleStateHandler(writeTimeout: timeout), position: .first).flatMap { _ in - self.channel.pipeline.addHandler(IdlePoolConnectionHandler(connection: self, logger: logger)) + self.channel.pipeline.addHandler(IdlePoolConnectionHandler(connection: self, loggingContext: context)) } } @@ -123,19 +124,19 @@ class IdlePoolConnectionHandler: ChannelInboundHandler, RemovableChannelHandler let connection: Connection var eventSent: Bool - let logger: Logger + let loggingContext: LoggingContext - init(connection: Connection, logger: Logger) { + init(connection: Connection, loggingContext: LoggingContext) { self.connection = connection self.eventSent = false - self.logger = logger + self.loggingContext = loggingContext } // this is needed to detect when remote end closes connection while connection is in the pool idling func channelInactive(context: ChannelHandlerContext) { if !self.eventSent { self.eventSent = true - self.connection.remoteClosed(logger: self.logger) + self.connection.remoteClosed(context: loggingContext) } } @@ -143,7 +144,7 @@ class IdlePoolConnectionHandler: ChannelInboundHandler, RemovableChannelHandler if let idleEvent = event as? IdleStateHandler.IdleStateEvent, idleEvent == .write { if !self.eventSent { self.eventSent = true - self.connection.timeout(logger: self.logger) + self.connection.timeout(context: loggingContext) } } else { context.fireUserInboundEventTriggered(event) diff --git a/Sources/AsyncHTTPClient/ConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool.swift index 8845f9709..994133616 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool.swift @@ -12,6 +12,7 @@ // //===----------------------------------------------------------------------===// +import Baggage import Foundation import Logging import NIO @@ -76,7 +77,7 @@ final class ConnectionPool { taskEventLoop: EventLoop, deadline: NIODeadline?, setupComplete: EventLoopFuture, - logger: Logger) -> EventLoopFuture { + context: LoggingContext) -> EventLoopFuture { let key = Key(request) let provider: HTTP1ConnectionProvider = self.lock.withLock { @@ -95,7 +96,7 @@ final class ConnectionPool { } } - return provider.getConnection(preference: preference, setupComplete: setupComplete, logger: logger) + return provider.getConnection(preference: preference, setupComplete: setupComplete, context: context) } func delete(_ provider: HTTP1ConnectionProvider) { @@ -240,66 +241,65 @@ class HTTP1ConnectionProvider { self.state.assertInvariants() } - func execute(_ action: Action, logger: Logger) { + func execute(_ action: Action, context: LoggingContext) { switch action { case .lease(let connection, let waiter): // if connection is became inactive, we create a new one. connection.cancelIdleTimeout().whenComplete { _ in if connection.isActiveEstimation { - logger.trace("leasing existing connection", + context.logger.trace("leasing existing connection", metadata: ["ahc-connection": "\(connection)"]) waiter.promise.succeed(connection) } else { - logger.trace("opening fresh connection (found matching but inactive connection)", + context.logger.trace("opening fresh connection (found matching but inactive connection)", metadata: ["ahc-dead-connection": "\(connection)"]) - self.makeChannel(preference: waiter.preference, - logger: logger).whenComplete { result in - self.connect(result, waiter: waiter, logger: logger) + self.makeChannel(preference: waiter.preference, context: context).whenComplete { result in + self.connect(result, waiter: waiter, context: context) } } } case .create(let waiter): - logger.trace("opening fresh connection (no connections to reuse available)") - self.makeChannel(preference: waiter.preference, logger: logger).whenComplete { result in - self.connect(result, waiter: waiter, logger: logger) + context.logger.trace("opening fresh connection (no connections to reuse available)") + self.makeChannel(preference: waiter.preference, context: context).whenComplete { result in + self.connect(result, waiter: waiter, context: context) } case .replace(let connection, let waiter): connection.cancelIdleTimeout().flatMap { connection.close() }.whenComplete { _ in - logger.trace("opening fresh connection (replacing exising connection)", + context.logger.trace("opening fresh connection (replacing exising connection)", metadata: ["ahc-old-connection": "\(connection)", "ahc-waiter": "\(waiter)"]) - self.makeChannel(preference: waiter.preference, logger: logger).whenComplete { result in - self.connect(result, waiter: waiter, logger: logger) + self.makeChannel(preference: waiter.preference, context: context).whenComplete { result in + self.connect(result, waiter: waiter, context: context) } } case .park(let connection): - logger.trace("parking connection", + context.logger.trace("parking connection", metadata: ["ahc-connection": "\(connection)"]) connection.setIdleTimeout(timeout: self.configuration.connectionPool.idleTimeout, - logger: self.backgroundActivityLogger) + context: DefaultLoggingContext(context: context).withLogger(backgroundActivityLogger)) case .closeProvider: - logger.debug("closing provider", + context.logger.debug("closing provider", metadata: ["ahc-provider": "\(self)"]) self.closeAndDelete() case .none: break case .parkAnd(let connection, let action): - logger.trace("parking connection & doing further action", + context.logger.trace("parking connection & doing further action", metadata: ["ahc-connection": "\(connection)", "ahc-action": "\(action)"]) connection.setIdleTimeout(timeout: self.configuration.connectionPool.idleTimeout, - logger: self.backgroundActivityLogger) - self.execute(action, logger: logger) + context: DefaultLoggingContext(context: context).withLogger(backgroundActivityLogger)) + self.execute(action, context: context) case .closeAnd(let connection, let action): - logger.trace("closing connection & doing further action", + context.logger.trace("closing connection & doing further action", metadata: ["ahc-connection": "\(connection)", "ahc-action": "\(action)"]) connection.channel.close(promise: nil) - self.execute(action, logger: logger) + self.execute(action, context: context) case .fail(let waiter, let error): - logger.debug("failing connection for waiter", + context.logger.debug("failing connection for waiter", metadata: ["ahc-waiter": "\(waiter)", "ahc-error": "\(error)"]) waiter.promise.fail(error) @@ -315,23 +315,23 @@ class HTTP1ConnectionProvider { func getConnection(preference: HTTPClient.EventLoopPreference, setupComplete: EventLoopFuture, - logger: Logger) -> EventLoopFuture { + context: LoggingContext) -> EventLoopFuture { let waiter = Waiter(promise: self.eventLoop.makePromise(), setupComplete: setupComplete, preference: preference) let action: Action = self.lock.withLock { self.state.acquire(waiter: waiter) } - self.execute(action, logger: logger) + self.execute(action, context: context) return waiter.promise.futureResult } - func connect(_ result: Result, waiter: Waiter, logger: Logger) { + func connect(_ result: Result, waiter: Waiter, context: LoggingContext) { let action: Action switch result { case .success(let channel): - logger.trace("successfully created connection", + context.logger.trace("successfully created connection", metadata: ["ahc-connection": "\(channel)"]) let connection = Connection(channel: channel, provider: self) action = self.lock.withLock { @@ -341,7 +341,7 @@ class HTTP1ConnectionProvider { switch action { case .closeAnd: // This happens when client was shut down during connect - logger.trace("connection cancelled due to client shutdown", + context.logger.trace("connection cancelled due to client shutdown", metadata: ["ahc-connection": "\(channel)"]) connection.channel.close(promise: nil) waiter.promise.fail(HTTPClientError.cancelled) @@ -349,7 +349,7 @@ class HTTP1ConnectionProvider { waiter.promise.succeed(connection) } case .failure(let error): - logger.debug("connection attempt failed", + context.logger.debug("connection attempt failed", metadata: ["ahc-error": "\(error)"]) action = self.lock.withLock { self.state.connectFailed() @@ -358,12 +358,12 @@ class HTTP1ConnectionProvider { } waiter.setupComplete.whenComplete { _ in - self.execute(action, logger: logger) + self.execute(action, context: context) } } - func release(connection: Connection, closing: Bool, logger: Logger) { - logger.debug("releasing connection, request complete", + func release(connection: Connection, closing: Bool, context: LoggingContext) { + context.logger.debug("releasing connection, request complete", metadata: ["ahc-closing": "\(closing)"]) let action: Action = self.lock.withLock { self.state.release(connection: connection, closing: closing) @@ -383,31 +383,31 @@ class HTTP1ConnectionProvider { case .park, .closeProvider: // Since both `.park` and `.deleteProvider` are terminal in terms of execution, // we can execute them immediately - self.execute(action, logger: logger) + self.execute(action, context: context) case .closeAnd, .create, .fail, .lease, .parkAnd, .replace: // This is needed to start a new stack, otherwise, since this is called on a previous // future completion handler chain, it will be growing indefinitely until the connection is closed. // We might revisit this when https://github.com/apple/swift-nio/issues/970 is resolved. connection.eventLoop.execute { - self.execute(action, logger: logger) + self.execute(action, context: context) } } } - func remoteClosed(connection: Connection, logger: Logger) { + func remoteClosed(connection: Connection, context: LoggingContext) { let action: Action = self.lock.withLock { self.state.remoteClosed(connection: connection) } - self.execute(action, logger: logger) + self.execute(action, context: context) } - func timeout(connection: Connection, logger: Logger) { + func timeout(connection: Connection, context: LoggingContext) { let action: Action = self.lock.withLock { self.state.timeout(connection: connection) } - self.execute(action, logger: logger) + self.execute(action, context: context) } private func closeAndDelete() { @@ -438,14 +438,13 @@ class HTTP1ConnectionProvider { return self.closePromise.futureResult.map { true } } - private func makeChannel(preference: HTTPClient.EventLoopPreference, - logger: Logger) -> EventLoopFuture { + private func makeChannel(preference: HTTPClient.EventLoopPreference, context: LoggingContext) -> EventLoopFuture { return NIOClientTCPBootstrap.makeHTTP1Channel(destination: self.key, eventLoop: self.eventLoop, configuration: self.configuration, sslContextCache: self.pool.sslContextCache, preference: preference, - logger: logger) + context: context) } /// A `Waiter` represents a request that waits for a connection when none is diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 602fcbbad..63d0206e1 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -15,6 +15,8 @@ import Baggage import Foundation import Instrumentation +import Tracing +import TracingOpenTelemetrySupport import Logging import NIO import NIOConcurrencyHelpers @@ -528,6 +530,21 @@ public class HTTPClient { return self.execute(request: request, delegate: accumulator, eventLoop: eventLoopPreference, deadline: deadline, logger: logger).futureResult } + /// Execute arbitrary HTTP request and handle response processing using provided delegate. + /// + /// - parameters: + /// - request: HTTP request to execute. + /// - eventLoop: NIO Event Loop preference. + /// - deadline: Point in time by which the request must complete. + /// - logger: The logger to use for this request. + public func execute(request: Request, + eventLoop eventLoopPreference: EventLoopPreference, + deadline: NIODeadline? = nil, + context: LoggingContext) -> EventLoopFuture { + let accumulator = ResponseAccumulator(request: request) + return self.execute(request: request, delegate: accumulator, eventLoop: eventLoopPreference, context: context, deadline: deadline).futureResult + } + /// Execute arbitrary HTTP request and handle response processing using provided delegate. /// /// - parameters: @@ -594,21 +611,16 @@ public class HTTPClient { /// - delegate: Delegate to process response parts. /// - eventLoop: NIO Event Loop preference. /// - deadline: Point in time by which the request must complete. - /// - context: The logging context carrying a logger and instrumentation metadata. public func execute(request: Request, delegate: Delegate, eventLoop eventLoopPreference: EventLoopPreference, - context: LoggingContext?, - deadline: NIODeadline? = nil) -> Task { - var request = request - if let baggage = context?.baggage { - InstrumentationSystem.instrument.inject(baggage, into: &request.headers, using: HTTPHeadersInjector()) - } + deadline: NIODeadline? = nil, + logger originalLogger: Logger?) -> Task { return self.execute(request: request, delegate: delegate, eventLoop: eventLoopPreference, - deadline: deadline, - logger: context?.logger) + context: DefaultLoggingContext.topLevel(logger: originalLogger ?? Self.loggingDisabled), + deadline: deadline) } /// Execute arbitrary HTTP request and handle response processing using provided delegate. @@ -618,12 +630,53 @@ public class HTTPClient { /// - delegate: Delegate to process response parts. /// - eventLoop: NIO Event Loop preference. /// - deadline: Point in time by which the request must complete. + /// - context: The logging context carrying a logger and instrumentation metadata. public func execute(request: Request, delegate: Delegate, eventLoop eventLoopPreference: EventLoopPreference, - deadline: NIODeadline? = nil, - logger originalLogger: Logger?) -> Task { - let logger = (originalLogger ?? HTTPClient.loggingDisabled).attachingRequestInformation(request, requestID: globalRequestID.add(1)) + context: LoggingContext? = nil, + deadline: NIODeadline? = nil) -> Task { + return self.execute(request: request, + delegate: delegate, + eventLoop: eventLoopPreference, + context: context ?? Self.loggingDisabledTopLevelContext, + deadline: deadline) + } + + private func execute(request: Request, + delegate: Delegate, + eventLoop eventLoopPreference: EventLoopPreference, + context: LoggingContext, + deadline: NIODeadline? = nil) -> Task { + let span = InstrumentationSystem.tracer.startSpan("HTTP \(request.method.rawValue)", + context: context, + ofKind: .client) + span.attributes.http.method = request.method.rawValue + span.attributes.http.url = request.url.absoluteString + var context = DefaultLoggingContext(context: context).withBaggage(span.baggage) + context.logger = context.logger.attachingRequestInformation(request, requestID: globalRequestID.add(1)) + let task = self.execute(request: request, + delegate: delegate, + eventLoop: eventLoopPreference, + context: context, + span: span, + deadline: deadline) + task.futureResult.whenComplete { result in + if case .failure(let error) = result { + span.recordError(error) + span.setStatus(SpanStatus(code: .error)) + } + span.end() + } + return task + } + + private func execute(request: Request, + delegate: Delegate, + eventLoop eventLoopPreference: EventLoopPreference, + context: LoggingContext, + span: Span, + deadline: NIODeadline? = nil) -> Task { let taskEL: EventLoop switch eventLoopPreference.preference { case .indifferent: @@ -637,7 +690,8 @@ public class HTTPClient { case .testOnly_exact(_, delegateOn: let delegateEL): taskEL = delegateEL } - logger.trace("selected EventLoop for task given the preference", + + context.logger.trace("selected EventLoop for task given the preference", metadata: ["ahc-eventloop": "\(taskEL)", "ahc-el-preference": "\(eventLoopPreference)"]) @@ -646,10 +700,10 @@ public class HTTPClient { case .upAndRunning: return nil case .shuttingDown, .shutDown: - logger.debug("client is shutting down, failing request") + context.logger.debug("client is shutting down, failing request") return Task.failedTask(eventLoop: taskEL, error: HTTPClientError.alreadyShutdown, - logger: logger) + context: context) } } @@ -674,24 +728,24 @@ public class HTTPClient { redirectHandler = nil } - let task = Task(eventLoop: taskEL, logger: logger) + let task = Task(eventLoop: taskEL, context: context) let setupComplete = taskEL.makePromise(of: Void.self) let connection = self.pool.getConnection(request, preference: eventLoopPreference, taskEventLoop: taskEL, deadline: deadline, setupComplete: setupComplete.futureResult, - logger: logger) + context: context) let taskHandler = TaskHandler(task: task, kind: request.kind, delegate: delegate, redirectHandler: redirectHandler, ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown, - logger: logger) + span: span) connection.flatMap { connection -> EventLoopFuture in - logger.debug("got connection for request", + context.logger.debug("got connection for request", metadata: ["ahc-connection": "\(connection)", "ahc-request": "\(request.method) \(request.url)", "ahc-channel-el": "\(connection.channel.eventLoop)", @@ -709,7 +763,7 @@ public class HTTPClient { try syncPipelineOperations.addHandler(taskHandler) } catch { - connection.release(closing: true, logger: logger) + connection.release(closing: true, context: context) return channel.eventLoop.makeFailedFuture(error) } diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 4850c51d8..83588fc04 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -12,6 +12,7 @@ // //===----------------------------------------------------------------------===// +import Tracing import Foundation import Logging import NIO @@ -636,19 +637,19 @@ extension HTTPClient { var connection: Connection? var cancelled: Bool let lock: Lock - let logger: Logger // We are okay to store the logger here because a Task is for only one request. + let context: LoggingContext // We are okay to store the logging context here because a Task is for only one request. - init(eventLoop: EventLoop, logger: Logger) { + init(eventLoop: EventLoop, context: LoggingContext) { self.eventLoop = eventLoop self.promise = eventLoop.makePromise() self.completion = self.promise.futureResult.map { _ in } self.cancelled = false self.lock = Lock() - self.logger = logger + self.context = context } - static func failedTask(eventLoop: EventLoop, error: Error, logger: Logger) -> Task { - let task = self.init(eventLoop: eventLoop, logger: logger) + static func failedTask(eventLoop: EventLoop, error: Error, context: LoggingContext) -> Task { + let task = self.init(eventLoop: eventLoop, context: context) task.promise.fail(error) return task } @@ -721,7 +722,7 @@ extension HTTPClient { return connection.removeHandler(IdleStateHandler.self).flatMap { connection.removeHandler(TaskHandler.self) }.map { - connection.release(closing: closing, logger: self.logger) + connection.release(closing: closing, context: self.context) }.flatMapError { error in fatalError("Couldn't remove taskHandler: \(error)") } @@ -755,7 +756,7 @@ internal class TaskHandler: RemovableChann let delegate: Delegate let redirectHandler: RedirectHandler? let ignoreUncleanSSLShutdown: Bool - let logger: Logger // We are okay to store the logger here because a TaskHandler is just for one request. + private weak var span: Span? var state: State = .idle var responseReadBuffer: ResponseReadBuffer = ResponseReadBuffer() @@ -777,13 +778,13 @@ internal class TaskHandler: RemovableChann delegate: Delegate, redirectHandler: RedirectHandler?, ignoreUncleanSSLShutdown: Bool, - logger: Logger) { + span: Span) { self.task = task self.delegate = delegate self.redirectHandler = redirectHandler self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown self.kind = kind - self.logger = logger + self.span = span } } @@ -881,6 +882,11 @@ extension TaskHandler: ChannelDuplexHandler { method: request.method, uri: request.uri) var headers = request.headers + if let span = self.span { + span.attributes.net.peer.ip = context.remoteAddress?.ipAddress + span.attributes.http.flavor = "\(head.version.major).\(head.version.minor)" + InstrumentationSystem.instrument.inject(span.baggage, into: &headers, using: HTTPHeadersInjector()) + } if !request.headers.contains(name: "host") { let port = request.port @@ -1030,6 +1036,10 @@ extension TaskHandler: ChannelDuplexHandler { let response = self.unwrapInboundIn(data) switch response { case .head(let head): + span?.attributes.http.statusCode = Int(head.status.code) + if 400 ..< 600 ~= head.status.code { + span?.setStatus(SpanStatus(code: .error)) + } switch self.state { case .idle: // should be prevented by NIO HTTP1 pipeline, see testHTTPResponseHeadBeforeRequestHead diff --git a/Sources/AsyncHTTPClient/SSLContextCache.swift b/Sources/AsyncHTTPClient/SSLContextCache.swift index 005c475de..044253f9a 100644 --- a/Sources/AsyncHTTPClient/SSLContextCache.swift +++ b/Sources/AsyncHTTPClient/SSLContextCache.swift @@ -12,6 +12,7 @@ // //===----------------------------------------------------------------------===// +import Baggage import Dispatch import Logging import NIO @@ -27,19 +28,19 @@ class SSLContextCache { extension SSLContextCache { func sslContext(tlsConfiguration: TLSConfiguration, eventLoop: EventLoop, - logger: Logger) -> EventLoopFuture { + context: LoggingContext) -> EventLoopFuture { let eqTLSConfiguration = BestEffortHashableTLSConfiguration(wrapping: tlsConfiguration) let sslContext = self.lock.withLock { self.sslContextCache.find(key: eqTLSConfiguration) } if let sslContext = sslContext { - logger.debug("found SSL context in cache", + context.logger.debug("found SSL context in cache", metadata: ["ahc-tls-config": "\(tlsConfiguration)"]) return eventLoop.makeSucceededFuture(sslContext) } - logger.debug("creating new SSL context", + context.logger.debug("creating new SSL context", metadata: ["ahc-tls-config": "\(tlsConfiguration)"]) let newSSLContext = self.offloadQueue.asyncWithFuture(eventLoop: eventLoop) { try NIOSSLContext(configuration: tlsConfiguration) diff --git a/Sources/AsyncHTTPClient/Utils.swift b/Sources/AsyncHTTPClient/Utils.swift index 6069222b1..d5fe8f99b 100644 --- a/Sources/AsyncHTTPClient/Utils.swift +++ b/Sources/AsyncHTTPClient/Utils.swift @@ -12,6 +12,7 @@ // //===----------------------------------------------------------------------===// +import Baggage import Foundation #if canImport(Network) import Network @@ -59,7 +60,7 @@ extension NIOClientTCPBootstrap { configuration: HTTPClient.Configuration, sslContextCache: SSLContextCache, preference: HTTPClient.EventLoopPreference, - logger: Logger) -> EventLoopFuture { + context: LoggingContext) -> EventLoopFuture { let channelEventLoop = preference.bestEventLoop ?? eventLoop let key = destination @@ -74,7 +75,7 @@ extension NIOClientTCPBootstrap { // plaintext to the proxy but we do support sending HTTPS traffic through the proxy. sslContext = sslContextCache.sslContext(tlsConfiguration: configuration.tlsConfiguration ?? .forClient(), eventLoop: eventLoop, - logger: logger).map { $0 } + context: context).map { $0 } } else { sslContext = eventLoop.makeSucceededFuture(nil) } @@ -85,7 +86,7 @@ extension NIOClientTCPBootstrap { requiresTLS: requiresTLS, configuration: configuration, sslContextCache: sslContextCache, - logger: logger) + context: context) return bootstrap.flatMap { bootstrap -> EventLoopFuture in let channel: EventLoopFuture switch key.scheme { @@ -124,7 +125,7 @@ extension NIOClientTCPBootstrap { requiresTLS: Bool, configuration: HTTPClient.Configuration, sslContextCache: SSLContextCache, - logger: Logger + context: LoggingContext ) -> EventLoopFuture { return self.makeBestBootstrap(host: host, eventLoop: eventLoop, @@ -132,7 +133,7 @@ extension NIOClientTCPBootstrap { sslContextCache: sslContextCache, tlsConfiguration: configuration.tlsConfiguration ?? .forClient(), useProxy: configuration.proxy != nil, - logger: logger) + context: context) .map { bootstrap -> NIOClientTCPBootstrap in var bootstrap = bootstrap @@ -174,7 +175,7 @@ extension NIOClientTCPBootstrap { sslContextCache: SSLContextCache, tlsConfiguration: TLSConfiguration, useProxy: Bool, - logger: Logger + context: LoggingContext ) -> EventLoopFuture { #if canImport(Network) // if eventLoop is compatible with NIOTransportServices create a NIOTSConnectionBootstrap @@ -196,7 +197,7 @@ extension NIOClientTCPBootstrap { } else { return sslContextCache.sslContext(tlsConfiguration: tlsConfiguration, eventLoop: eventLoop, - logger: logger) + context: context) .flatMapThrowing { sslContext in let hostname = (host.isIPAddress || host.isEmpty) ? nil : host let tlsProvider = try NIOSSLClientTLSProvider(context: sslContext, serverHostname: hostname) From 10f26427a7f413a717c0c740de7aa759272f70f5 Mon Sep 17 00:00:00 2001 From: Tim <0xtimc@gmail.com> Date: Mon, 10 Jan 2022 18:01:45 +0000 Subject: [PATCH 23/23] Fix building on macOS 12 --- Package.swift | 3 ++- .../NIOTransportServices/TLSConfiguration.swift | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Package.swift b/Package.swift index 4b5a297b8..704b7bf04 100644 --- a/Package.swift +++ b/Package.swift @@ -27,13 +27,14 @@ let package = Package( .package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.5.1"), .package(url: "https://github.com/apple/swift-log.git", from: "1.4.0"), .package(url: "https://github.com/apple/swift-distributed-tracing.git", from: "0.1.1"), + .package(url: "https://github.com/apple/swift-distributed-tracing-baggage", .upToNextMinor(from: "0.1.1")), ], targets: [ .target( name: "AsyncHTTPClient", dependencies: ["NIO", "NIOHTTP1", "NIOSSL", "NIOConcurrencyHelpers", "NIOHTTPCompression", "NIOFoundationCompat", "NIOTransportServices", "Logging", "Instrumentation", - "Tracing", "TracingOpenTelemetrySupport"] + "Tracing", "TracingOpenTelemetrySupport", "Baggage"] ), .testTarget( name: "AsyncHTTPClientTests", diff --git a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift index 86867c6f9..c35b95a2a 100644 --- a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift +++ b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift @@ -22,6 +22,7 @@ extension TLSVersion { /// return Network framework TLS protocol version + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) var nwTLSProtocolVersion: tls_protocol_version_t { switch self { case .tlsv1: