From b3dbc08c45b255ac04e110406f244ae25c570f6e Mon Sep 17 00:00:00 2001 From: Johannes Weiss Date: Wed, 11 Sep 2019 23:20:32 +0100 Subject: [PATCH] minor: improve code style --- Package.swift | 2 +- Sources/AsyncHTTPClient/HTTPHandler.swift | 16 ++-- .../HTTPClientInternalTests.swift | 12 +-- .../HTTPClientTestUtils.swift | 37 ++++---- .../HTTPClientTests.swift | 88 +++++++++---------- 5 files changed, 77 insertions(+), 78 deletions(-) diff --git a/Package.swift b/Package.swift index 3f14dedbd..21de3392d 100644 --- a/Package.swift +++ b/Package.swift @@ -31,7 +31,7 @@ let package = Package( ), .testTarget( name: "AsyncHTTPClientTests", - dependencies: ["AsyncHTTPClient", "NIOFoundationCompat"] + dependencies: ["NIO", "NIOConcurrencyHelpers", "NIOSSL", "AsyncHTTPClient", "NIOFoundationCompat"] ), ] ) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 9d6f0e667..e835eeda7 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -551,17 +551,15 @@ internal class TaskHandler: ChannelInbound } private func writeBody(request: HTTPClient.Request, context: ChannelHandlerContext) -> EventLoopFuture { - if let body = request.body { - return body.stream(HTTPClient.Body.StreamWriter { part in - let future = context.writeAndFlush(self.wrapOutboundOut(.body(part))) - future.whenSuccess { _ in - self.delegate.didSendRequestPart(task: self.task, part) - } - return future - }) - } else { + guard let body = request.body else { return context.eventLoop.makeSucceededFuture(()) } + + return body.stream(HTTPClient.Body.StreamWriter { part in + context.writeAndFlush(self.wrapOutboundOut(.body(part))).map { + self.delegate.didSendRequestPart(task: self.task, part) + } + }) } public func read(context: ChannelHandlerContext) { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 0c650482f..6d73caf95 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -74,11 +74,11 @@ class HTTPClientInternalTests: XCTestCase { } func testProxyStreaming() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let body: HTTPClient.Body = .stream(length: 50) { writer in @@ -104,11 +104,11 @@ class HTTPClientInternalTests: XCTestCase { } func testProxyStreamingFailure() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } var body: HTTPClient.Body = .stream(length: 50) { _ in @@ -165,11 +165,11 @@ class HTTPClientInternalTests: XCTestCase { let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) let promise: EventLoopPromise = httpClient.eventLoopGroup.next().makePromise() - let httpBin = HttpBin(channelPromise: promise) + let httpBin = HTTPBin(channelPromise: promise) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let request = try Request(url: "http://localhost:\(httpBin.port)/custom") diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 423ccec61..41b22d41b 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -15,6 +15,7 @@ import AsyncHTTPClient import Foundation import NIO +import NIOConcurrencyHelpers import NIOHTTP1 import NIOSSL @@ -90,9 +91,10 @@ internal final class RecordingHandler: ChannelDuplexHandler { } } -internal class HttpBin { +internal final class HTTPBin { let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) let serverChannel: Channel + let isShutdown: Atomic = .init(value: false) var port: Int { return Int(self.serverChannel.localAddress!.port!) @@ -125,7 +127,7 @@ internal class HttpBin { } }.flatMap { if ssl { - return HttpBin.configureTLS(channel: channel).flatMap { + return HTTPBin.configureTLS(channel: channel).flatMap { channel.pipeline.addHandler(HttpBinHandler(channelPromise: channelPromise)) } } else { @@ -135,8 +137,13 @@ internal class HttpBin { }.bind(host: "127.0.0.1", port: 0).wait() } - func shutdown() { - try! self.group.syncShutdownGracefully() + func shutdown() throws { + self.isShutdown.store(true) + try self.group.syncShutdownGracefully() + } + + deinit { + assert(self.isShutdown.load(), "HTTPBin not shutdown before deinit") } } @@ -186,7 +193,7 @@ final class HTTPProxySimulator: ChannelInboundHandler, RemovableChannelHandler { switch self.option { case .tls: - _ = HttpBin.configureTLS(channel: context.channel) + _ = HTTPBin.configureTLS(channel: context.channel) case .plaintext: break } } @@ -311,20 +318,16 @@ internal final class HttpBinHandler: ChannelInboundHandler { response.add(body) self.resps.prepend(response) case .end: - if let promise = self.channelPromise { - promise.succeed(context.channel) - } + self.channelPromise?.succeed(context.channel) if self.resps.isEmpty { return } let response = self.resps.removeFirst() context.write(wrapOutboundOut(.head(response.head)), promise: nil) - if let body = response.body { - let data = body.withUnsafeReadableBytes { - Data(bytes: $0.baseAddress!, count: $0.count) - } - - let serialized = try! JSONEncoder().encode(RequestInfo(data: String(data: data, encoding: .utf8)!)) + if var body = response.body { + let data = body.readData(length: body.readableBytes)! + let serialized = try! JSONEncoder().encode(RequestInfo(data: String(decoding: data, + as: Unicode.UTF8.self))) var responseBody = context.channel.allocator.buffer(capacity: serialized.count) responseBody.writeBytes(serialized) @@ -395,9 +398,7 @@ internal final class HttpBinForSSLUncleanShutdownHandler: ChannelInboundHandler func channelRead(context: ChannelHandlerContext, data: NIOAny) { switch self.unwrapInboundIn(data) { case .head(let req): - if let promise = self.channelPromise { - promise.succeed(context.channel) - } + self.channelPromise?.succeed(context.channel) let response: String? switch req.uri { @@ -440,7 +441,7 @@ internal final class HttpBinForSSLUncleanShutdownHandler: ChannelInboundHandler context.writeAndFlush(self.wrapOutboundOut(buffer), promise: nil) } - _ = context.channel.pipeline.removeHandler(name: "NIOSSLServerHandler").map { _ in + context.channel.pipeline.removeHandler(name: "NIOSSLServerHandler").whenSuccess { context.close(promise: nil) } case .body: diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 0e549fd9d..0c4e17df6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -51,11 +51,11 @@ class HTTPClientTests: XCTestCase { } func testGet() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let response = try httpClient.get(url: "http://localhost:\(httpBin.port)/get").wait() @@ -63,14 +63,14 @@ class HTTPClientTests: XCTestCase { } func testGetWithDifferentEventLoopBackpressure() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let loopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1) let external = MultiThreadedEventLoopGroup(numberOfThreads: 1) let httpClient = HTTPClient(eventLoopGroupProvider: .shared(loopGroup)) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) XCTAssertNoThrow(try loopGroup.syncShutdownGracefully()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/events/10/1") let delegate = TestHTTPDelegate(backpressureEventLoop: external.next()) @@ -79,11 +79,11 @@ class HTTPClientTests: XCTestCase { } func testPost() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let response = try httpClient.post(url: "http://localhost:\(httpBin.port)/post", body: .string("1234")).wait() @@ -95,12 +95,12 @@ class HTTPClientTests: XCTestCase { } func testGetHttps() throws { - let httpBin = HttpBin(ssl: true) + let httpBin = HTTPBin(ssl: true) let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, configuration: HTTPClient.Configuration(certificateVerification: .none)) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let response = try httpClient.get(url: "https://localhost:\(httpBin.port)/get").wait() @@ -108,12 +108,12 @@ class HTTPClientTests: XCTestCase { } func testPostHttps() throws { - let httpBin = HttpBin(ssl: true) + let httpBin = HTTPBin(ssl: true) let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, configuration: HTTPClient.Configuration(certificateVerification: .none)) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let request = try Request(url: "https://localhost:\(httpBin.port)/post", method: .POST, body: .string("1234")) @@ -127,15 +127,15 @@ class HTTPClientTests: XCTestCase { } func testHttpRedirect() throws { - let httpBin = HttpBin(ssl: false) - let httpsBin = HttpBin(ssl: true) + let httpBin = HTTPBin(ssl: false) + let httpsBin = HTTPBin(ssl: true) let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: true)) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() - httpsBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) + XCTAssertNoThrow(try httpsBin.shutdown()) } var response = try httpClient.get(url: "http://localhost:\(httpBin.port)/redirect/302").wait() @@ -146,13 +146,13 @@ class HTTPClientTests: XCTestCase { } func testHttpHostRedirect() throws { - let httpBin = HttpBin(ssl: false) + let httpBin = HTTPBin(ssl: false) let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: true)) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let response = try httpClient.get(url: "http://localhost:\(httpBin.port)/redirect/loopback?port=\(httpBin.port)").wait() @@ -170,11 +170,11 @@ class HTTPClientTests: XCTestCase { } func testPercentEncoded() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let response = try httpClient.get(url: "http://localhost:\(httpBin.port)/percent%20encoded").wait() @@ -186,9 +186,9 @@ class HTTPClientTests: XCTestCase { defer { XCTAssertNoThrow(try httpClient.syncShutdown()) } - let httpBin = HttpBin() + let httpBin = HTTPBin() defer { - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let body = ByteBuffer.of(string: "hello world!") @@ -202,11 +202,11 @@ class HTTPClientTests: XCTestCase { } func testStreaming() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } var request = try Request(url: "http://localhost:\(httpBin.port)/events/10/1") @@ -219,12 +219,12 @@ class HTTPClientTests: XCTestCase { } func testRemoteClose() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } XCTAssertThrowsError(try httpClient.get(url: "http://localhost:\(httpBin.port)/close").wait(), "Should fail") { error in @@ -235,12 +235,12 @@ class HTTPClientTests: XCTestCase { } func testReadTimeout() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew, configuration: HTTPClient.Configuration(timeout: HTTPClient.Configuration.Timeout(read: .milliseconds(150)))) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } XCTAssertThrowsError(try httpClient.get(url: "http://localhost:\(httpBin.port)/wait").wait(), "Should fail") { error in @@ -251,12 +251,12 @@ class HTTPClientTests: XCTestCase { } func testDeadline() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } XCTAssertThrowsError(try httpClient.get(url: "http://localhost:\(httpBin.port)/wait", deadline: .now() + .milliseconds(150)).wait(), "Should fail") { error in @@ -267,12 +267,12 @@ class HTTPClientTests: XCTestCase { } func testCancel() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let queue = DispatchQueue(label: "nio-test") @@ -299,21 +299,21 @@ class HTTPClientTests: XCTestCase { } func testProxyPlaintext() throws { - let httpBin = HttpBin(simulateProxy: .plaintext) + let httpBin = HTTPBin(simulateProxy: .plaintext) let httpClient = HTTPClient( eventLoopGroupProvider: .createNew, configuration: .init(proxy: .server(host: "localhost", port: httpBin.port)) ) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let res = try httpClient.get(url: "http://test/ok").wait() XCTAssertEqual(res.status, .ok) } func testProxyTLS() throws { - let httpBin = HttpBin(simulateProxy: .tls) + let httpBin = HTTPBin(simulateProxy: .tls) let httpClient = HTTPClient( eventLoopGroupProvider: .createNew, configuration: .init( @@ -323,35 +323,35 @@ class HTTPClientTests: XCTestCase { ) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let res = try httpClient.get(url: "https://test/ok").wait() XCTAssertEqual(res.status, .ok) } func testProxyPlaintextWithCorrectlyAuthorization() throws { - let httpBin = HttpBin(simulateProxy: .plaintext) + let httpBin = HTTPBin(simulateProxy: .plaintext) let httpClient = HTTPClient( eventLoopGroupProvider: .createNew, configuration: .init(proxy: .server(host: "localhost", port: httpBin.port, authorization: .basic(username: "aladdin", password: "opensesame"))) ) defer { - try! httpClient.syncShutdown() - httpBin.shutdown() + XCTAssertNoThrow(try httpClient.syncShutdown()) + XCTAssertNoThrow(try httpBin.shutdown()) } let res = try httpClient.get(url: "http://test/ok").wait() XCTAssertEqual(res.status, .ok) } func testProxyPlaintextWithIncorrectlyAuthorization() throws { - let httpBin = HttpBin(simulateProxy: .plaintext) + let httpBin = HTTPBin(simulateProxy: .plaintext) let httpClient = HTTPClient( eventLoopGroupProvider: .createNew, configuration: .init(proxy: .server(host: "localhost", port: httpBin.port, authorization: .basic(username: "aladdin", password: "opensesamefoo"))) ) defer { - try! httpClient.syncShutdown() - httpBin.shutdown() + XCTAssertNoThrow(try httpClient.syncShutdown()) + XCTAssertNoThrow(try httpBin.shutdown()) } XCTAssertThrowsError(try httpClient.get(url: "http://test/ok").wait(), "Should fail") { error in guard case let error = error as? HTTPClientError, error == .proxyAuthenticationRequired else { @@ -361,11 +361,11 @@ class HTTPClientTests: XCTestCase { } func testUploadStreaming() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } let body: HTTPClient.Body = .stream(length: 8) { writer in @@ -522,14 +522,14 @@ class HTTPClientTests: XCTestCase { } func testEventLoopArgument() throws { - let httpBin = HttpBin() + let httpBin = HTTPBin() let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 5) let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoopGroup), configuration: HTTPClient.Configuration(followRedirects: true)) defer { XCTAssertNoThrow(try httpClient.syncShutdown()) XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully()) - httpBin.shutdown() + XCTAssertNoThrow(try httpBin.shutdown()) } class EventLoopValidatingDelegate: HTTPClientResponseDelegate {