From 0aecc2be52b36387a109543bbbb12361c65ed979 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 24 Jan 2023 20:27:15 +0100 Subject: [PATCH 1/8] Reproducer --- .../HTTP1/HTTP1ClientChannelHandler.swift | 27 +++---- .../HTTP1/HTTP1Connection.swift | 7 +- .../HTTP2/HTTP2Connection.swift | 3 + .../HTTP2/HTTP2IdleHandler.swift | 3 +- .../HTTP1ClientChannelHandlerTests.swift | 76 +++++++++++++++++++ .../HTTPClientTestUtils.swift | 16 +++- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 40 ++++++++++ 8 files changed, 155 insertions(+), 18 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index ac92e4bc8..191a179a5 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -35,8 +35,8 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { didSet { if let newRequest = self.request { var requestLogger = newRequest.logger - requestLogger[metadataKey: "ahc-connection-id"] = "\(self.connection.id)" - requestLogger[metadataKey: "ahc-el"] = "\(self.connection.channel.eventLoop)" + requestLogger[metadataKey: "ahc-connection-id"] = connectionIdLoggerMetadata + requestLogger[metadataKey: "ahc-el"] = "\(self.eventLoop)" self.logger = requestLogger if let idleReadTimeout = newRequest.requestOptions.idleReadTimeout { @@ -59,15 +59,15 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { private let backgroundLogger: Logger private var logger: Logger - - let connection: HTTP1Connection - let eventLoop: EventLoop - - init(connection: HTTP1Connection, eventLoop: EventLoop, logger: Logger) { - self.connection = connection + private let eventLoop: EventLoop + private let connectionIdLoggerMetadata: Logger.MetadataValue + + var onRequestCompleted: () -> () = {} + init(eventLoop: EventLoop, backgroundLogger: Logger, connectionIdLoggerMetadata: Logger.MetadataValue) { self.eventLoop = eventLoop - self.backgroundLogger = logger - self.logger = self.backgroundLogger + self.backgroundLogger = backgroundLogger + self.logger = backgroundLogger + self.connectionIdLoggerMetadata = connectionIdLoggerMetadata } func handlerAdded(context: ChannelHandlerContext) { @@ -274,7 +274,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { if shouldClose { context.close(promise: nil) } else { - self.connection.taskCompleted() + self.onRequestCompleted() } oldRequest.succeedRequest(buffer) @@ -286,7 +286,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: writePromise) case .informConnectionIsIdle: - self.connection.taskCompleted() + self.onRequestCompleted() oldRequest.succeedRequest(buffer) } @@ -303,7 +303,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { oldRequest.fail(error) case .informConnectionIsIdle: - self.connection.taskCompleted() + self.onRequestCompleted() oldRequest.fail(error) case .failWritePromise(let writePromise): @@ -328,6 +328,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { // we must check if the request is still present here. guard let request = self.request else { return } request.requestHeadSent() + request.resumeRequestBodyStream() } else { context.write(self.wrapOutboundOut(.head(head)), promise: nil) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift index 3485ada6c..7962e4df7 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift @@ -133,10 +133,13 @@ final class HTTP1Connection { } let channelHandler = HTTP1ClientChannelHandler( - connection: self, eventLoop: channel.eventLoop, - logger: logger + backgroundLogger: logger, + connectionIdLoggerMetadata: "\(self.id)" ) + channelHandler.onRequestCompleted = { + self.taskCompleted() + } try sync.addHandler(channelHandler) } catch { diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift index 5859e619a..c81047bf6 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift @@ -244,6 +244,9 @@ final class HTTP2Connection { self.channel.closeFuture.whenComplete { _ in self.openStreams.remove(box) } + channel.closeFuture.whenComplete { result in + print("H2 closed", result) + } channel.write(request, promise: nil) return channel.eventLoop.makeSucceededVoidFuture() diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift index c522b2425..f32fe734a 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift @@ -62,7 +62,8 @@ final class HTTP2IdleHandler: ChannelDuplexH let frame = self.unwrapInboundIn(data) switch frame.payload { - case .goAway: + case .goAway(_, let errorCode, _): + print(errorCode) let action = self.state.goAwayReceived() self.run(action, context: context) diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index f97580372..6dd4e7c95 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -526,6 +526,82 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { XCTAssertTrue(error is FailEndHandler.Error) } } + + func test() throws { + + final class ChangeWritabilityOnFlush: ChannelOutboundHandler { + typealias OutboundIn = Any + func flush(context: ChannelHandlerContext) { + (context.channel as! EmbeddedChannel).isWritable = false + context.fireChannelWritabilityChanged() + } + } + + final class Request: HTTPExecutableRequest { + var logger: Logging.Logger { Logger(label: "request")} + + var requestHead: NIOHTTP1.HTTPRequestHead + + var requestFramingMetadata: AsyncHTTPClient.RequestFramingMetadata = .init(connectionClose: false, body: .fixedSize(1)) + + var requestOptions: AsyncHTTPClient.RequestOptions = .forTests() + + init(requestHead: NIOHTTP1.HTTPRequestHead = .init(version: .http1_1, method: .GET, uri: "http://localhost/")) { + self.requestHead = requestHead + } + + func willExecuteRequest(_: AsyncHTTPClient.HTTPRequestExecutor) { + print(#function) + } + + func requestHeadSent() { + print(#function) + } + + func resumeRequestBodyStream() { + print(#function) + } + + func pauseRequestBodyStream() { + print(#function) + } + + func receiveResponseHead(_ head: NIOHTTP1.HTTPResponseHead) { + print(#function) + } + + func receiveResponseBodyParts(_ buffer: NIOCore.CircularBuffer) { + print(#function) + } + + func succeedRequest(_ buffer: NIOCore.CircularBuffer?) { + print(#function) + } + + func fail(_ error: Error) { + print(#function) + } + } + let eventLoopGroup = EmbeddedEventLoopGroup(loops: 1) + let eventLoop = eventLoopGroup.next() as! EmbeddedEventLoop + let handler = HTTP1ClientChannelHandler( + eventLoop: eventLoop, + backgroundLogger: Logger(label: "no-op", factory: SwiftLogNoOpLogHandler.init), + connectionIdLoggerMetadata: "test connection" + ) + handler.onRequestCompleted = { + print("onRequestCompleted") + } + let channel = EmbeddedChannel(handlers: [ + ChangeWritabilityOnFlush(), + handler, + ], loop: eventLoop) + try channel.connect(to: .init(ipAddress: "127.0.0.1", port: 80)) + + let request = Request() + try channel.writeOutbound(request) + + } } class TestBackpressureWriter { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 884681123..7b6554cc3 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -375,7 +375,18 @@ internal final class HTTPBin where return "https" } }() - return "\(scheme)://localhost:\(self.port)/" + let host: String = { + switch self.socketAddress { + case .v4: + return self.socketAddress.ipAddress! + case .v6: + return "[\(self.socketAddress.ipAddress!)]" + case .unixDomainSocket: + return self.socketAddress.pathname! + } + }() + + return "\(scheme)://\(host):\(self.port)/" } private let mode: Mode @@ -557,7 +568,8 @@ internal final class HTTPBin where initialSettings: [ // TODO: make max concurrent streams configurable HTTP2Setting(parameter: .maxConcurrentStreams, value: 10), - HTTP2Setting(parameter: .maxHeaderListSize, value: HPACKDecoder.defaultMaxHeaderListSize), + HTTP2Setting(parameter: .maxHeaderListSize, value: 1024 * 1024 * 16), + HTTP2Setting(parameter: .maxFrameSize, value: 1024 * 1024 * 8), ] ) let multiplexer = HTTP2StreamMultiplexer( diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index ef81b1dde..337e235a6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -143,6 +143,7 @@ extension HTTPClientTests { ("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail), ("testMassiveDownload", testMassiveDownload), ("testShutdownWithFutures", testShutdownWithFutures), + ("testMassiveHeader", testMassiveHeader), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 8f4126c43..de1b0481d 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3363,4 +3363,44 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) XCTAssertNoThrow(try httpClient.shutdown().wait()) } + + func testMassiveHeader() throws { + //let httpBin = HTTPBin(.http2(compress: false)) + let httpBin = HTTPBin(.http1_1()) + defer { + print("bin shutdown started") + XCTAssertNoThrow(try httpBin.shutdown()) + print("bin shutdown complete") + } + let factory = { (label: String) -> LogHandler in StreamLogHandler.standardOutput(label: label) } + var bgLogger = Logger(label: "BG", factory: factory) + bgLogger.logLevel = .trace + let localClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup), + configuration: .init( + certificateVerification: .none + ), + backgroundActivityLogger: bgLogger + ) + defer { + print("client shutdown started") + XCTAssertNoThrow(try localClient.syncShutdown()) + print("client shutdown complete") + } + var rqLogger = Logger(label: "RQ", factory: factory) + rqLogger.logLevel = .trace + + var request = try HTTPClient.Request(url: httpBin.baseURL, method: .POST) + // add 4 Megabyte header + let headerValue = String(repeating: "0", count: 1024 * 4) + // non empty body is important to trigger this bug as we otherwise finish the request in a single flush + request.body = .byteBuffer(ByteBuffer(bytes: [0])) + for headerID in 0..<(1024) { + request.headers.replaceOrAdd(name: "larg-header-\(headerID)", value: headerValue) + } + let requests = (0..<1).map { _ in + localClient.execute(request: request, deadline: .now() + .seconds(10), logger: rqLogger) + } + XCTAssertNoThrow(try EventLoopFuture.whenAllSucceed(requests, on: clientGroup.any()).wait()) + } } From c43bccc309150a7a72c00f308df75aa47c9c7841 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 12:03:56 +0100 Subject: [PATCH 2/8] Refactor test case --- .../HTTP1/HTTP1ClientChannelHandler.swift | 7 + .../HTTP1ClientChannelHandlerTests.swift | 193 +++++++++++++----- .../HTTPClientTests.swift | 42 +--- 3 files changed, 159 insertions(+), 83 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index 191a179a5..dc5003b90 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -108,6 +108,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { let action = self.state.writabilityChanged(writable: context.channel.isWritable) self.run(action, context: context) + context.fireChannelWritabilityChanged() } func channelRead(context: ChannelHandlerContext, data: NIOAny) { @@ -156,6 +157,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { metadata: req.requestFramingMetadata ) self.run(action, context: context) + promise?.succeed(()) } func read(context: ChannelHandlerContext) { @@ -435,6 +437,11 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { } } +#if swift(>=5.6) +@available(*, unavailable) +extension HTTP1ClientChannelHandler: Sendable {} +#endif + extension HTTP1ClientChannelHandler: HTTPRequestExecutor { func writeRequestBodyPart(_ data: IOData, request: HTTPExecutableRequest, promise: EventLoopPromise?) { if self.eventLoop.inEventLoop { diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index 6dd4e7c95..975e998b7 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -528,60 +528,14 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { } func test() throws { - final class ChangeWritabilityOnFlush: ChannelOutboundHandler { typealias OutboundIn = Any func flush(context: ChannelHandlerContext) { + context.flush() (context.channel as! EmbeddedChannel).isWritable = false context.fireChannelWritabilityChanged() } } - - final class Request: HTTPExecutableRequest { - var logger: Logging.Logger { Logger(label: "request")} - - var requestHead: NIOHTTP1.HTTPRequestHead - - var requestFramingMetadata: AsyncHTTPClient.RequestFramingMetadata = .init(connectionClose: false, body: .fixedSize(1)) - - var requestOptions: AsyncHTTPClient.RequestOptions = .forTests() - - init(requestHead: NIOHTTP1.HTTPRequestHead = .init(version: .http1_1, method: .GET, uri: "http://localhost/")) { - self.requestHead = requestHead - } - - func willExecuteRequest(_: AsyncHTTPClient.HTTPRequestExecutor) { - print(#function) - } - - func requestHeadSent() { - print(#function) - } - - func resumeRequestBodyStream() { - print(#function) - } - - func pauseRequestBodyStream() { - print(#function) - } - - func receiveResponseHead(_ head: NIOHTTP1.HTTPResponseHead) { - print(#function) - } - - func receiveResponseBodyParts(_ buffer: NIOCore.CircularBuffer) { - print(#function) - } - - func succeedRequest(_ buffer: NIOCore.CircularBuffer?) { - print(#function) - } - - func fail(_ error: Error) { - print(#function) - } - } let eventLoopGroup = EmbeddedEventLoopGroup(loops: 1) let eventLoop = eventLoopGroup.next() as! EmbeddedEventLoop let handler = HTTP1ClientChannelHandler( @@ -596,11 +550,152 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { ChangeWritabilityOnFlush(), handler, ], loop: eventLoop) - try channel.connect(to: .init(ipAddress: "127.0.0.1", port: 80)) + try channel.connect(to: .init(ipAddress: "127.0.0.1", port: 80)).wait() - let request = Request() + + let request = HTTPTestRequest() + // non empty body is important to trigger this bug as we otherwise finish the request in a single flush + request.requestFramingMetadata.body = .fixedSize(1) + request.raiseErrorIfUnimplementedMethodIsCalled = false try channel.writeOutbound(request) + XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent]) + } +} + +final class HTTPTestRequest: HTTPExecutableRequest { + enum Event { + /// ``Event`` without associated values + enum Kind: Hashable { + case willExecuteRequest + case requestHeadSent + case resumeRequestBodyStream + case pauseRequestBodyStream + case receiveResponseHead + case receiveResponseBodyParts + case succeedRequest + case fail + } + case willExecuteRequest(HTTPRequestExecutor) + case requestHeadSent + case resumeRequestBodyStream + case pauseRequestBodyStream + case receiveResponseHead(HTTPResponseHead) + case receiveResponseBodyParts(CircularBuffer) + case succeedRequest(CircularBuffer?) + case fail(Error) + var kind: Kind { + switch self { + case .willExecuteRequest: return .willExecuteRequest + case .requestHeadSent: return .requestHeadSent + case .resumeRequestBodyStream: return .resumeRequestBodyStream + case .pauseRequestBodyStream: return .pauseRequestBodyStream + case .receiveResponseHead: return .receiveResponseHead + case .receiveResponseBodyParts: return .receiveResponseBodyParts + case .succeedRequest: return .succeedRequest + case .fail: return .fail + } + } + } + + var logger: Logging.Logger = Logger(label: "request") + var requestHead: NIOHTTP1.HTTPRequestHead + var requestFramingMetadata: RequestFramingMetadata + var requestOptions: RequestOptions = .forTests() + + /// if true and ``HTTPExecutableRequest`` method is called without setting a corisbonding callback on `self` e.g. + /// If ``HTTPExecutableRequest\.willExecuteRequest(_:)`` is called but ``willExecuteRequestCallback`` is not set, + /// ``XCTestFail(_:)`` will be called to fail the current test. + var raiseErrorIfUnimplementedMethodIsCalled: Bool = true + private var file: StaticString + private var line: UInt + + var willExecuteRequestCallback: ((_: HTTPRequestExecutor) -> ())? + var requestHeadSentCallback: (() -> ())? + var resumeRequestBodyStreamCallback: (() -> ())? + var pauseRequestBodyStreamCallback: (() -> ())? + var receiveResponseHeadCallback: ((_ head: HTTPResponseHead) -> ())? + var receiveResponseBodyPartsCallback: ((_ buffer: CircularBuffer) -> ())? + var succeedRequestCallback: ((_ buffer: CircularBuffer?) -> ())? + var failCallback: ((_ error: Error) -> ())? + + + /// captures all ``HTTPExecutableRequest`` method calls in the order of occurrence, including arguments. + /// If you are not interested in the arguments you can use `events.map(\.kind)` to get all events without arguments. + private(set) var events: [Event] = [] + + init( + head: NIOHTTP1.HTTPRequestHead = .init(version: .http1_1, method: .GET, uri: "http://localhost/"), + framingMetadata: RequestFramingMetadata = .init(connectionClose: false, body: .fixedSize(0)), + file: StaticString = #file, + line: UInt = #line + ) { + self.requestHead = head + self.requestFramingMetadata = framingMetadata + self.file = file + self.line = line + } + + private func calledUnimplementedMethod(_ name: String) { + guard raiseErrorIfUnimplementedMethodIsCalled else { return } + XCTFail("\(name) invoked but it is not implemented", file: file, line: line) + } + + func willExecuteRequest(_ executor: HTTPRequestExecutor) { + self.events.append(.willExecuteRequest(executor)) + guard let willExecuteRequestCallback = willExecuteRequestCallback else { + return self.calledUnimplementedMethod(#function) + } + willExecuteRequestCallback(executor) + } + func requestHeadSent() { + self.events.append(.requestHeadSent) + guard let requestHeadSentCallback = requestHeadSentCallback else { + return self.calledUnimplementedMethod(#function) + } + requestHeadSentCallback() + } + func resumeRequestBodyStream() { + self.events.append(.resumeRequestBodyStream) + guard let resumeRequestBodyStreamCallback = resumeRequestBodyStreamCallback else { + return self.calledUnimplementedMethod(#function) + } + resumeRequestBodyStreamCallback() + } + func pauseRequestBodyStream() { + self.events.append(.pauseRequestBodyStream) + guard let pauseRequestBodyStreamCallback = pauseRequestBodyStreamCallback else { + return self.calledUnimplementedMethod(#function) + } + pauseRequestBodyStreamCallback() + } + func receiveResponseHead(_ head: HTTPResponseHead) { + self.events.append(.receiveResponseHead(head)) + guard let receiveResponseHeadCallback = receiveResponseHeadCallback else { + return self.calledUnimplementedMethod(#function) + } + receiveResponseHeadCallback(head) + } + func receiveResponseBodyParts(_ buffer: CircularBuffer) { + self.events.append(.receiveResponseBodyParts(buffer)) + guard let receiveResponseBodyPartsCallback = receiveResponseBodyPartsCallback else { + return self.calledUnimplementedMethod(#function) + } + receiveResponseBodyPartsCallback(buffer) + } + func succeedRequest(_ buffer: CircularBuffer?) { + self.events.append(.succeedRequest(buffer)) + guard let succeedRequestCallback = succeedRequestCallback else { + return self.calledUnimplementedMethod(#function) + } + succeedRequestCallback(buffer) + } + func fail(_ error: Error) { + self.events.append(.fail(error)) + guard let failCallback = failCallback else { + return self.calledUnimplementedMethod(#function) + } + failCallback(error) } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index de1b0481d..ce533dd3e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3364,43 +3364,17 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertNoThrow(try httpClient.shutdown().wait()) } - func testMassiveHeader() throws { - //let httpBin = HTTPBin(.http2(compress: false)) - let httpBin = HTTPBin(.http1_1()) - defer { - print("bin shutdown started") - XCTAssertNoThrow(try httpBin.shutdown()) - print("bin shutdown complete") - } - let factory = { (label: String) -> LogHandler in StreamLogHandler.standardOutput(label: label) } - var bgLogger = Logger(label: "BG", factory: factory) - bgLogger.logLevel = .trace - let localClient = HTTPClient( - eventLoopGroupProvider: .shared(self.clientGroup), - configuration: .init( - certificateVerification: .none - ), - backgroundActivityLogger: bgLogger - ) - defer { - print("client shutdown started") - XCTAssertNoThrow(try localClient.syncShutdown()) - print("client shutdown complete") + func testMassiveHeaderHTTP1() throws { + var request = try HTTPClient.Request(url: defaultHTTPBin.baseURL, method: .POST) + // add ~64 KB header + let headerValue = String(repeating: "0", count: 1024) + for headerID in 0..<(64) { + request.headers.replaceOrAdd(name: "larg-header-\(headerID)", value: headerValue) } - var rqLogger = Logger(label: "RQ", factory: factory) - rqLogger.logLevel = .trace - var request = try HTTPClient.Request(url: httpBin.baseURL, method: .POST) - // add 4 Megabyte header - let headerValue = String(repeating: "0", count: 1024 * 4) // non empty body is important to trigger this bug as we otherwise finish the request in a single flush request.body = .byteBuffer(ByteBuffer(bytes: [0])) - for headerID in 0..<(1024) { - request.headers.replaceOrAdd(name: "larg-header-\(headerID)", value: headerValue) - } - let requests = (0..<1).map { _ in - localClient.execute(request: request, deadline: .now() + .seconds(10), logger: rqLogger) - } - XCTAssertNoThrow(try EventLoopFuture.whenAllSucceed(requests, on: clientGroup.any()).wait()) + + XCTAssertNoThrow(try defaultClient.execute(request: request).wait()) } } From fb644162246b76e43f390e30e94f1accd75a9789 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 12:26:49 +0100 Subject: [PATCH 3/8] Refactor tests --- ...TTP1ClientChannelHandlerTests+XCTest.swift | 1 + .../HTTP1ClientChannelHandlerTests.swift | 142 +--------------- .../HTTPClientTests+XCTest.swift | 2 +- .../HTTPClientTests.swift | 1 + .../HTTPConnectionPool+HTTP1StateTests.swift | 38 ++--- ...onnectionPool+HTTP2StateMachineTests.swift | 56 +++---- .../Mocks/MockConnectionPool.swift | 6 +- .../Mocks/MockHTTPExecutableRequest.swift | 156 ++++++++++++++++++ 8 files changed, 212 insertions(+), 190 deletions(-) create mode 100644 Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests+XCTest.swift index 66c1a48d1..2502e6fb7 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests+XCTest.swift @@ -33,6 +33,7 @@ extension HTTP1ClientChannelHandlerTests { ("testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand", testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand), ("testWriteHTTPHeadFails", testWriteHTTPHeadFails), ("testHandlerClosesChannelIfLastActionIsSendEndAndItFails", testHandlerClosesChannelIfLastActionIsSendEndAndItFails), + ("testChannelBecomesNonWritableDuringHeaderWrite", testChannelBecomesNonWritableDuringHeaderWrite), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index 975e998b7..c88b9357c 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -527,7 +527,8 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { } } - func test() throws { + func testChannelBecomesNonWritableDuringHeaderWrite() throws { + try XCTSkipIf(true, "this currently fails and will be fixed in follow up PR") final class ChangeWritabilityOnFlush: ChannelOutboundHandler { typealias OutboundIn = Any func flush(context: ChannelHandlerContext) { @@ -553,7 +554,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { try channel.connect(to: .init(ipAddress: "127.0.0.1", port: 80)).wait() - let request = HTTPTestRequest() + let request = MockHTTPExecutableRequest() // non empty body is important to trigger this bug as we otherwise finish the request in a single flush request.requestFramingMetadata.body = .fixedSize(1) request.raiseErrorIfUnimplementedMethodIsCalled = false @@ -562,143 +563,6 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { } } -final class HTTPTestRequest: HTTPExecutableRequest { - enum Event { - /// ``Event`` without associated values - enum Kind: Hashable { - case willExecuteRequest - case requestHeadSent - case resumeRequestBodyStream - case pauseRequestBodyStream - case receiveResponseHead - case receiveResponseBodyParts - case succeedRequest - case fail - } - case willExecuteRequest(HTTPRequestExecutor) - case requestHeadSent - case resumeRequestBodyStream - case pauseRequestBodyStream - case receiveResponseHead(HTTPResponseHead) - case receiveResponseBodyParts(CircularBuffer) - case succeedRequest(CircularBuffer?) - case fail(Error) - - var kind: Kind { - switch self { - case .willExecuteRequest: return .willExecuteRequest - case .requestHeadSent: return .requestHeadSent - case .resumeRequestBodyStream: return .resumeRequestBodyStream - case .pauseRequestBodyStream: return .pauseRequestBodyStream - case .receiveResponseHead: return .receiveResponseHead - case .receiveResponseBodyParts: return .receiveResponseBodyParts - case .succeedRequest: return .succeedRequest - case .fail: return .fail - } - } - } - - var logger: Logging.Logger = Logger(label: "request") - var requestHead: NIOHTTP1.HTTPRequestHead - var requestFramingMetadata: RequestFramingMetadata - var requestOptions: RequestOptions = .forTests() - - /// if true and ``HTTPExecutableRequest`` method is called without setting a corisbonding callback on `self` e.g. - /// If ``HTTPExecutableRequest\.willExecuteRequest(_:)`` is called but ``willExecuteRequestCallback`` is not set, - /// ``XCTestFail(_:)`` will be called to fail the current test. - var raiseErrorIfUnimplementedMethodIsCalled: Bool = true - private var file: StaticString - private var line: UInt - - var willExecuteRequestCallback: ((_: HTTPRequestExecutor) -> ())? - var requestHeadSentCallback: (() -> ())? - var resumeRequestBodyStreamCallback: (() -> ())? - var pauseRequestBodyStreamCallback: (() -> ())? - var receiveResponseHeadCallback: ((_ head: HTTPResponseHead) -> ())? - var receiveResponseBodyPartsCallback: ((_ buffer: CircularBuffer) -> ())? - var succeedRequestCallback: ((_ buffer: CircularBuffer?) -> ())? - var failCallback: ((_ error: Error) -> ())? - - - /// captures all ``HTTPExecutableRequest`` method calls in the order of occurrence, including arguments. - /// If you are not interested in the arguments you can use `events.map(\.kind)` to get all events without arguments. - private(set) var events: [Event] = [] - - init( - head: NIOHTTP1.HTTPRequestHead = .init(version: .http1_1, method: .GET, uri: "http://localhost/"), - framingMetadata: RequestFramingMetadata = .init(connectionClose: false, body: .fixedSize(0)), - file: StaticString = #file, - line: UInt = #line - ) { - self.requestHead = head - self.requestFramingMetadata = framingMetadata - self.file = file - self.line = line - } - - private func calledUnimplementedMethod(_ name: String) { - guard raiseErrorIfUnimplementedMethodIsCalled else { return } - XCTFail("\(name) invoked but it is not implemented", file: file, line: line) - } - - func willExecuteRequest(_ executor: HTTPRequestExecutor) { - self.events.append(.willExecuteRequest(executor)) - guard let willExecuteRequestCallback = willExecuteRequestCallback else { - return self.calledUnimplementedMethod(#function) - } - willExecuteRequestCallback(executor) - } - func requestHeadSent() { - self.events.append(.requestHeadSent) - guard let requestHeadSentCallback = requestHeadSentCallback else { - return self.calledUnimplementedMethod(#function) - } - requestHeadSentCallback() - } - func resumeRequestBodyStream() { - self.events.append(.resumeRequestBodyStream) - guard let resumeRequestBodyStreamCallback = resumeRequestBodyStreamCallback else { - return self.calledUnimplementedMethod(#function) - } - resumeRequestBodyStreamCallback() - } - func pauseRequestBodyStream() { - self.events.append(.pauseRequestBodyStream) - guard let pauseRequestBodyStreamCallback = pauseRequestBodyStreamCallback else { - return self.calledUnimplementedMethod(#function) - } - pauseRequestBodyStreamCallback() - } - func receiveResponseHead(_ head: HTTPResponseHead) { - self.events.append(.receiveResponseHead(head)) - guard let receiveResponseHeadCallback = receiveResponseHeadCallback else { - return self.calledUnimplementedMethod(#function) - } - receiveResponseHeadCallback(head) - } - func receiveResponseBodyParts(_ buffer: CircularBuffer) { - self.events.append(.receiveResponseBodyParts(buffer)) - guard let receiveResponseBodyPartsCallback = receiveResponseBodyPartsCallback else { - return self.calledUnimplementedMethod(#function) - } - receiveResponseBodyPartsCallback(buffer) - } - func succeedRequest(_ buffer: CircularBuffer?) { - self.events.append(.succeedRequest(buffer)) - guard let succeedRequestCallback = succeedRequestCallback else { - return self.calledUnimplementedMethod(#function) - } - succeedRequestCallback(buffer) - } - func fail(_ error: Error) { - self.events.append(.fail(error)) - guard let failCallback = failCallback else { - return self.calledUnimplementedMethod(#function) - } - failCallback(error) - } -} - class TestBackpressureWriter { let eventLoop: EventLoop diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 337e235a6..f9ddb1c8b 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -143,7 +143,7 @@ extension HTTPClientTests { ("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail), ("testMassiveDownload", testMassiveDownload), ("testShutdownWithFutures", testShutdownWithFutures), - ("testMassiveHeader", testMassiveHeader), + ("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index ce533dd3e..58e2ebc0a 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3365,6 +3365,7 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { } func testMassiveHeaderHTTP1() throws { + try XCTSkipIf(true, "this currently crashes and will be fixed in follow up PR") var request = try HTTPClient.Request(url: defaultHTTPBin.baseURL, method: .POST) // add ~64 KB header let headerValue = String(repeating: "0", count: 1024) diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift index 125ba1a74..6cb097b04 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift @@ -37,7 +37,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // for the first eight requests, the pool should try to create new connections. for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .createConnection(let connectionID, let connectionEL) = action.connection else { @@ -53,7 +53,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // the next eight requests should only be queued. for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .none = action.connection else { @@ -120,7 +120,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // for the first eight requests, the pool should try to create new connections. for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .createConnection(let connectionID, let connectionEL) = action.connection else { @@ -136,7 +136,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // the next eight requests should only be queued. for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .none = action.connection else { @@ -181,7 +181,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { retryConnectionEstablishment: true ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -239,7 +239,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { retryConnectionEstablishment: true ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -276,7 +276,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { retryConnectionEstablishment: true ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -310,7 +310,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { XCTAssertEqual(cleanupContext.connectBackoff, []) // 4. execute another request - let finalMockRequest = MockHTTPRequest(eventLoop: elg.next()) + let finalMockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let finalRequest = HTTPConnectionPool.Request(finalMockRequest) let failAction = state.executeRequest(finalRequest) XCTAssertEqual(failAction.connection, .none) @@ -339,7 +339,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { return XCTFail("Expected to still have connections available") } - let mockRequest = MockHTTPRequest(eventLoop: eventLoop) + let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -359,7 +359,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var queuer = MockRequestQueuer() for _ in 0..<100 { let eventLoop = elg.next() - let mockRequest = MockHTTPRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false) + let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -418,7 +418,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // 10% of the cases enforce the eventLoop let elRequired = (0..<10).randomElement().flatMap { $0 == 0 ? true : false }! - let mockRequest = MockHTTPRequest(eventLoop: reqEventLoop, requiresEventLoopForChannel: elRequired) + let mockRequest = MockHTTPScheduableRequest(eventLoop: reqEventLoop, requiresEventLoopForChannel: elRequired) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -482,7 +482,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { XCTAssertEqual(connections.parked, 8) // close a leased connection == abort - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) guard let connectionToAbort = connections.newestParkedConnection else { return XCTFail("Expected to have a parked connection") @@ -536,7 +536,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { return XCTFail("Expected to still have connections available") } - let mockRequest = MockHTTPRequest(eventLoop: eventLoop) + let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -553,7 +553,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { for _ in 0..<100 { let eventLoop = elg.next() - let mockRequest = MockHTTPRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false) + let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -667,7 +667,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { retryConnectionEstablishment: true ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -706,7 +706,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { retryConnectionEstablishment: true ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -738,7 +738,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { retryConnectionEstablishment: true ) - let mockRequest = MockHTTPRequest(eventLoop: eventLoop.next(), requiresEventLoopForChannel: false) + let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop.next(), requiresEventLoopForChannel: false) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -762,7 +762,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { retryConnectionEstablishment: true ) - let mockRequest1 = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) + let mockRequest1 = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) let request1 = HTTPConnectionPool.Request(mockRequest1) let executeAction1 = state.executeRequest(request1) @@ -773,7 +773,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { XCTAssertEqual(executeAction1.request, .scheduleRequestTimeout(for: request1, on: mockRequest1.eventLoop)) - let mockRequest2 = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) + let mockRequest2 = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) let request2 = HTTPConnectionPool.Request(mockRequest2) let executeAction2 = state.executeRequest(request2) diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift index 10fad7bd6..2fefa697b 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift @@ -36,7 +36,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { ) /// first request should create a new connection - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -52,7 +52,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { /// subsequent requests should not create a connection for _ in 0..<9 { - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -103,7 +103,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { /// 4 streams are available and therefore request should be executed immediately for _ in 0..<4 { - let mockRequest = MockHTTPRequest(eventLoop: el1, requiresEventLoopForChannel: true) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1, requiresEventLoopForChannel: true) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -146,7 +146,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { lifecycleState: .running ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -205,7 +205,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { lifecycleState: .running ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -243,7 +243,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { lifecycleState: .running ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) @@ -274,7 +274,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { lifecycleState: .running ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -313,7 +313,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { lifecycleState: .running ) - let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) @@ -347,7 +347,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { XCTAssertEqual(cleanupContext.connectBackoff, []) // 4. execute another request - let finalMockRequest = MockHTTPRequest(eventLoop: elg.next()) + let finalMockRequest = MockHTTPScheduableRequest(eventLoop: elg.next()) let finalRequest = HTTPConnectionPool.Request(finalMockRequest) let failAction = state.executeRequest(finalRequest) XCTAssertEqual(failAction.connection, .none) @@ -371,9 +371,9 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { lifecycleState: .running ) - let mockRequest1 = MockHTTPRequest(eventLoop: el1) + let mockRequest1 = MockHTTPScheduableRequest(eventLoop: el1) let request1 = HTTPConnectionPool.Request(mockRequest1) - let mockRequest2 = MockHTTPRequest(eventLoop: el1) + let mockRequest2 = MockHTTPScheduableRequest(eventLoop: el1) let request2 = HTTPConnectionPool.Request(mockRequest2) let executeAction1 = http1State.executeRequest(request1) @@ -456,7 +456,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { )) // execute request on idle connection - let mockRequest1 = MockHTTPRequest(eventLoop: el1) + let mockRequest1 = MockHTTPScheduableRequest(eventLoop: el1) let request1 = HTTPConnectionPool.Request(mockRequest1) let request1Action = state.executeRequest(request1) XCTAssertEqual(request1Action.request, .executeRequest(request1, conn1, cancelTimeout: false)) @@ -468,7 +468,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { XCTAssertEqual(closeStream1Action.connection, .scheduleTimeoutTimer(conn1ID, on: el1)) // execute request on idle connection with required event loop - let mockRequest2 = MockHTTPRequest(eventLoop: el1, requiresEventLoopForChannel: true) + let mockRequest2 = MockHTTPScheduableRequest(eventLoop: el1, requiresEventLoopForChannel: true) let request2 = HTTPConnectionPool.Request(mockRequest2) let request2Action = state.executeRequest(request2) XCTAssertEqual(request2Action.request, .executeRequest(request2, conn1, cancelTimeout: false)) @@ -535,7 +535,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { )) // create new http2 connection - let mockRequest1 = MockHTTPRequest(eventLoop: el2, requiresEventLoopForChannel: true) + let mockRequest1 = MockHTTPScheduableRequest(eventLoop: el2, requiresEventLoopForChannel: true) let request1 = HTTPConnectionPool.Request(mockRequest1) let executeAction = state.executeRequest(request1) XCTAssertEqual(executeAction.request, .scheduleRequestTimeout(for: request1, on: el2)) @@ -614,7 +614,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { )) // execute request on idle connection - let mockRequest1 = MockHTTPRequest(eventLoop: el1) + let mockRequest1 = MockHTTPScheduableRequest(eventLoop: el1) let request1 = HTTPConnectionPool.Request(mockRequest1) let request1Action = state.executeRequest(request1) XCTAssertEqual(request1Action.request, .executeRequest(request1, conn1, cancelTimeout: false)) @@ -659,14 +659,14 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { )) // execute request - let mockRequest1 = MockHTTPRequest(eventLoop: el1) + let mockRequest1 = MockHTTPScheduableRequest(eventLoop: el1) let request1 = HTTPConnectionPool.Request(mockRequest1) let request1Action = state.executeRequest(request1) XCTAssertEqual(request1Action.request, .executeRequest(request1, conn1, cancelTimeout: false)) XCTAssertEqual(request1Action.connection, .cancelTimeoutTimer(conn1ID)) // queue request - let mockRequest2 = MockHTTPRequest(eventLoop: el1) + let mockRequest2 = MockHTTPScheduableRequest(eventLoop: el1) let request2 = HTTPConnectionPool.Request(mockRequest2) let request2Action = state.executeRequest(request2) XCTAssertEqual(request2Action.request, .scheduleRequestTimeout(for: request2, on: el1)) @@ -711,7 +711,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { /// first 8 request should create a new connection var connectionIDs: [HTTPConnectionPool.Connection.ID] = [] for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .createConnection(let connID, let eventLoop) = action.connection else { @@ -730,7 +730,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { /// after we reached the `maximumConcurrentHTTP1Connections`, we will not create new connections for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) XCTAssertEqual(action.connection, .none) @@ -799,7 +799,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { ) /// create a new connection - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .createConnection(let conn1ID, let eventLoop) = action.connection else { @@ -847,7 +847,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { /// first 8 request should create a new connection var connectionIDs: [HTTPConnectionPool.Connection.ID] = [] for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .createConnection(let connID, let eventLoop) = action.connection else { @@ -862,7 +862,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { /// after we reached the `maximumConcurrentHTTP1Connections`, we will not create new connections for _ in 0..<8 { - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) XCTAssertEqual(action.connection, .none) @@ -984,7 +984,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { ) // create http2 connection - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request1 = HTTPConnectionPool.Request(mockRequest) let action1 = state.executeRequest(request1) guard case .createConnection(let http2ConnID, let http2EventLoop) = action1.connection else { @@ -1008,7 +1008,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { } // a request with new required event loop should create a new connection - let mockRequestWithRequiredEventLoop = MockHTTPRequest(eventLoop: el2, requiresEventLoopForChannel: true) + let mockRequestWithRequiredEventLoop = MockHTTPScheduableRequest(eventLoop: el2, requiresEventLoopForChannel: true) let requestWithRequiredEventLoop = HTTPConnectionPool.Request(mockRequestWithRequiredEventLoop) let action2 = state.executeRequest(requestWithRequiredEventLoop) guard case .createConnection(let http1ConnId, let http1EventLoop) = action2.connection else { @@ -1054,7 +1054,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { ) // create http2 connection - let mockRequest = MockHTTPRequest(eventLoop: el1) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el1) let request1 = HTTPConnectionPool.Request(mockRequest) let action1 = state.executeRequest(request1) guard case .createConnection(let http2ConnID, let http2EventLoop) = action1.connection else { @@ -1078,7 +1078,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { } // a request with new required event loop should create a new connection - let mockRequestWithRequiredEventLoop = MockHTTPRequest(eventLoop: el2, requiresEventLoopForChannel: true) + let mockRequestWithRequiredEventLoop = MockHTTPScheduableRequest(eventLoop: el2, requiresEventLoopForChannel: true) let requestWithRequiredEventLoop = HTTPConnectionPool.Request(mockRequestWithRequiredEventLoop) let action2 = state.executeRequest(requestWithRequiredEventLoop) guard case .createConnection(let http1ConnId, let http1EventLoop) = action2.connection else { @@ -1131,7 +1131,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { var connectionIDs: [HTTPConnectionPool.Connection.ID] = [] for el in [el1, el2, el2] { - let mockRequest = MockHTTPRequest(eventLoop: el, requiresEventLoopForChannel: true) + let mockRequest = MockHTTPScheduableRequest(eventLoop: el, requiresEventLoopForChannel: true) let request = HTTPConnectionPool.Request(mockRequest) let action = state.executeRequest(request) guard case .createConnection(let connID, let eventLoop) = action.connection else { @@ -1210,7 +1210,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { // shall be queued. for i in 0..<1000 { let requestEL = elg.next() - let mockRequest = MockHTTPRequest(eventLoop: requestEL) + let mockRequest = MockHTTPScheduableRequest(eventLoop: requestEL) let request = HTTPConnectionPool.Request(mockRequest) let executeAction = state.executeRequest(request) diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift index 1b2c27b68..4374c713d 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift @@ -548,7 +548,7 @@ extension MockConnectionPool { var queuer = MockRequestQueuer() for _ in 0..) + case succeedRequest(CircularBuffer?) + case fail(Error) + + var kind: Kind { + switch self { + case .willExecuteRequest: return .willExecuteRequest + case .requestHeadSent: return .requestHeadSent + case .resumeRequestBodyStream: return .resumeRequestBodyStream + case .pauseRequestBodyStream: return .pauseRequestBodyStream + case .receiveResponseHead: return .receiveResponseHead + case .receiveResponseBodyParts: return .receiveResponseBodyParts + case .succeedRequest: return .succeedRequest + case .fail: return .fail + } + } + } + + var logger: Logging.Logger = Logger(label: "request") + var requestHead: NIOHTTP1.HTTPRequestHead + var requestFramingMetadata: RequestFramingMetadata + var requestOptions: RequestOptions = .forTests() + + /// if true and ``HTTPExecutableRequest`` method is called without setting a corisbonding callback on `self` e.g. + /// If ``HTTPExecutableRequest\.willExecuteRequest(_:)`` is called but ``willExecuteRequestCallback`` is not set, + /// ``XCTestFail(_:)`` will be called to fail the current test. + var raiseErrorIfUnimplementedMethodIsCalled: Bool = true + private var file: StaticString + private var line: UInt + + var willExecuteRequestCallback: ((_: HTTPRequestExecutor) -> ())? + var requestHeadSentCallback: (() -> ())? + var resumeRequestBodyStreamCallback: (() -> ())? + var pauseRequestBodyStreamCallback: (() -> ())? + var receiveResponseHeadCallback: ((_ head: HTTPResponseHead) -> ())? + var receiveResponseBodyPartsCallback: ((_ buffer: CircularBuffer) -> ())? + var succeedRequestCallback: ((_ buffer: CircularBuffer?) -> ())? + var failCallback: ((_ error: Error) -> ())? + + + /// captures all ``HTTPExecutableRequest`` method calls in the order of occurrence, including arguments. + /// If you are not interested in the arguments you can use `events.map(\.kind)` to get all events without arguments. + private(set) var events: [Event] = [] + + init( + head: NIOHTTP1.HTTPRequestHead = .init(version: .http1_1, method: .GET, uri: "http://localhost/"), + framingMetadata: RequestFramingMetadata = .init(connectionClose: false, body: .fixedSize(0)), + file: StaticString = #file, + line: UInt = #line + ) { + self.requestHead = head + self.requestFramingMetadata = framingMetadata + self.file = file + self.line = line + } + + private func calledUnimplementedMethod(_ name: String) { + guard raiseErrorIfUnimplementedMethodIsCalled else { return } + XCTFail("\(name) invoked but it is not implemented", file: file, line: line) + } + + func willExecuteRequest(_ executor: HTTPRequestExecutor) { + self.events.append(.willExecuteRequest(executor)) + guard let willExecuteRequestCallback = willExecuteRequestCallback else { + return self.calledUnimplementedMethod(#function) + } + willExecuteRequestCallback(executor) + } + func requestHeadSent() { + self.events.append(.requestHeadSent) + guard let requestHeadSentCallback = requestHeadSentCallback else { + return self.calledUnimplementedMethod(#function) + } + requestHeadSentCallback() + } + func resumeRequestBodyStream() { + self.events.append(.resumeRequestBodyStream) + guard let resumeRequestBodyStreamCallback = resumeRequestBodyStreamCallback else { + return self.calledUnimplementedMethod(#function) + } + resumeRequestBodyStreamCallback() + } + func pauseRequestBodyStream() { + self.events.append(.pauseRequestBodyStream) + guard let pauseRequestBodyStreamCallback = pauseRequestBodyStreamCallback else { + return self.calledUnimplementedMethod(#function) + } + pauseRequestBodyStreamCallback() + } + func receiveResponseHead(_ head: HTTPResponseHead) { + self.events.append(.receiveResponseHead(head)) + guard let receiveResponseHeadCallback = receiveResponseHeadCallback else { + return self.calledUnimplementedMethod(#function) + } + receiveResponseHeadCallback(head) + } + func receiveResponseBodyParts(_ buffer: CircularBuffer) { + self.events.append(.receiveResponseBodyParts(buffer)) + guard let receiveResponseBodyPartsCallback = receiveResponseBodyPartsCallback else { + return self.calledUnimplementedMethod(#function) + } + receiveResponseBodyPartsCallback(buffer) + } + func succeedRequest(_ buffer: CircularBuffer?) { + self.events.append(.succeedRequest(buffer)) + guard let succeedRequestCallback = succeedRequestCallback else { + return self.calledUnimplementedMethod(#function) + } + succeedRequestCallback(buffer) + } + func fail(_ error: Error) { + self.events.append(.fail(error)) + guard let failCallback = failCallback else { + return self.calledUnimplementedMethod(#function) + } + failCallback(error) + } +} From cb026ca2a1b3806e5135de31ddec8ed12400a685 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 13:23:22 +0100 Subject: [PATCH 4/8] Remove debugging artefacts --- .../AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift | 3 --- .../ConnectionPool/HTTP2/HTTP2IdleHandler.swift | 3 +-- .../AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift | 3 --- Tests/AsyncHTTPClientTests/HTTPClientBase.swift | 2 +- Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift | 3 +-- 5 files changed, 3 insertions(+), 11 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift index c81047bf6..5859e619a 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift @@ -244,9 +244,6 @@ final class HTTP2Connection { self.channel.closeFuture.whenComplete { _ in self.openStreams.remove(box) } - channel.closeFuture.whenComplete { result in - print("H2 closed", result) - } channel.write(request, promise: nil) return channel.eventLoop.makeSucceededVoidFuture() diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift index f32fe734a..c522b2425 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift @@ -62,8 +62,7 @@ final class HTTP2IdleHandler: ChannelDuplexH let frame = self.unwrapInboundIn(data) switch frame.payload { - case .goAway(_, let errorCode, _): - print(errorCode) + case .goAway: let action = self.state.goAwayReceived() self.run(action, context: context) diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index c88b9357c..b0477aaaf 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -544,9 +544,6 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { backgroundLogger: Logger(label: "no-op", factory: SwiftLogNoOpLogHandler.init), connectionIdLoggerMetadata: "test connection" ) - handler.onRequestCompleted = { - print("onRequestCompleted") - } let channel = EmbeddedChannel(handlers: [ ChangeWritabilityOnFlush(), handler, diff --git a/Tests/AsyncHTTPClientTests/HTTPClientBase.swift b/Tests/AsyncHTTPClientTests/HTTPClientBase.swift index af310953a..188a6959f 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientBase.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientBase.swift @@ -39,7 +39,7 @@ class XCTestCaseHTTPClientTestsBaseClass: XCTestCase { var backgroundLogStore: CollectEverythingLogHandler.LogStore! var defaultHTTPBinURLPrefix: String { - return "http://localhost:\(self.defaultHTTPBin.port)/" + self.defaultHTTPBin.baseURL } override func setUp() { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 7b6554cc3..9da37ac08 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -568,8 +568,7 @@ internal final class HTTPBin where initialSettings: [ // TODO: make max concurrent streams configurable HTTP2Setting(parameter: .maxConcurrentStreams, value: 10), - HTTP2Setting(parameter: .maxHeaderListSize, value: 1024 * 1024 * 16), - HTTP2Setting(parameter: .maxFrameSize, value: 1024 * 1024 * 8), + HTTP2Setting(parameter: .maxHeaderListSize, value: HPACKDecoder.defaultMaxHeaderListSize), ] ) let multiplexer = HTTP2StreamMultiplexer( From a7f64d46238146e8cb958002197dfd0e60095664 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 13:27:52 +0100 Subject: [PATCH 5/8] Fix typo --- .../AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift b/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift index 42db320e6..a64b22fc3 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift @@ -59,7 +59,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { var requestFramingMetadata: RequestFramingMetadata var requestOptions: RequestOptions = .forTests() - /// if true and ``HTTPExecutableRequest`` method is called without setting a corisbonding callback on `self` e.g. + /// if true and ``HTTPExecutableRequest`` method is called without setting a corresponding callback on `self` e.g. /// If ``HTTPExecutableRequest\.willExecuteRequest(_:)`` is called but ``willExecuteRequestCallback`` is not set, /// ``XCTestFail(_:)`` will be called to fail the current test. var raiseErrorIfUnimplementedMethodIsCalled: Bool = true From 3f8f55ff5741175e7114bd08a7f95e01d6d22fe6 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 13:45:52 +0100 Subject: [PATCH 6/8] Fix formatting --- .../HTTP1/HTTP1ClientChannelHandler.swift | 8 ++-- .../HTTP1ClientChannelHandlerTests.swift | 5 +-- .../HTTPClientTestUtils.swift | 2 +- .../HTTPClientTests.swift | 8 ++-- .../Mocks/MockHTTPExecutableRequest.swift | 45 +++++++++++-------- 5 files changed, 37 insertions(+), 31 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index dc5003b90..76335bc9b 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -35,7 +35,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { didSet { if let newRequest = self.request { var requestLogger = newRequest.logger - requestLogger[metadataKey: "ahc-connection-id"] = connectionIdLoggerMetadata + requestLogger[metadataKey: "ahc-connection-id"] = self.connectionIdLoggerMetadata requestLogger[metadataKey: "ahc-el"] = "\(self.eventLoop)" self.logger = requestLogger @@ -61,8 +61,8 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { private var logger: Logger private let eventLoop: EventLoop private let connectionIdLoggerMetadata: Logger.MetadataValue - - var onRequestCompleted: () -> () = {} + + var onRequestCompleted: () -> Void = {} init(eventLoop: EventLoop, backgroundLogger: Logger, connectionIdLoggerMetadata: Logger.MetadataValue) { self.eventLoop = eventLoop self.backgroundLogger = backgroundLogger @@ -330,7 +330,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { // we must check if the request is still present here. guard let request = self.request else { return } request.requestHeadSent() - + request.resumeRequestBodyStream() } else { context.write(self.wrapOutboundOut(.head(head)), promise: nil) diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index b0477aaaf..29d373cd7 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -526,7 +526,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { XCTAssertTrue(error is FailEndHandler.Error) } } - + func testChannelBecomesNonWritableDuringHeaderWrite() throws { try XCTSkipIf(true, "this currently fails and will be fixed in follow up PR") final class ChangeWritabilityOnFlush: ChannelOutboundHandler { @@ -549,8 +549,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { handler, ], loop: eventLoop) try channel.connect(to: .init(ipAddress: "127.0.0.1", port: 80)).wait() - - + let request = MockHTTPExecutableRequest() // non empty body is important to trigger this bug as we otherwise finish the request in a single flush request.requestFramingMetadata.body = .fixedSize(1) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 9da37ac08..e50dab3b6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -385,7 +385,7 @@ internal final class HTTPBin where return self.socketAddress.pathname! } }() - + return "\(scheme)://\(host):\(self.port)/" } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 58e2ebc0a..8e5d5fbfa 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3363,19 +3363,19 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup)) XCTAssertNoThrow(try httpClient.shutdown().wait()) } - + func testMassiveHeaderHTTP1() throws { try XCTSkipIf(true, "this currently crashes and will be fixed in follow up PR") var request = try HTTPClient.Request(url: defaultHTTPBin.baseURL, method: .POST) // add ~64 KB header let headerValue = String(repeating: "0", count: 1024) - for headerID in 0..<(64) { + for headerID in 0..<64 { request.headers.replaceOrAdd(name: "larg-header-\(headerID)", value: headerValue) } - + // non empty body is important to trigger this bug as we otherwise finish the request in a single flush request.body = .byteBuffer(ByteBuffer(bytes: [0])) - + XCTAssertNoThrow(try defaultClient.execute(request: request).wait()) } } diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift b/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift index a64b22fc3..aa0dc45eb 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift @@ -31,6 +31,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { case succeedRequest case fail } + case willExecuteRequest(HTTPRequestExecutor) case requestHeadSent case resumeRequestBodyStream @@ -39,7 +40,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { case receiveResponseBodyParts(CircularBuffer) case succeedRequest(CircularBuffer?) case fail(Error) - + var kind: Kind { switch self { case .willExecuteRequest: return .willExecuteRequest @@ -53,33 +54,32 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } } } - + var logger: Logging.Logger = Logger(label: "request") var requestHead: NIOHTTP1.HTTPRequestHead var requestFramingMetadata: RequestFramingMetadata var requestOptions: RequestOptions = .forTests() - + /// if true and ``HTTPExecutableRequest`` method is called without setting a corresponding callback on `self` e.g. /// If ``HTTPExecutableRequest\.willExecuteRequest(_:)`` is called but ``willExecuteRequestCallback`` is not set, /// ``XCTestFail(_:)`` will be called to fail the current test. var raiseErrorIfUnimplementedMethodIsCalled: Bool = true private var file: StaticString private var line: UInt - - var willExecuteRequestCallback: ((_: HTTPRequestExecutor) -> ())? - var requestHeadSentCallback: (() -> ())? - var resumeRequestBodyStreamCallback: (() -> ())? - var pauseRequestBodyStreamCallback: (() -> ())? - var receiveResponseHeadCallback: ((_ head: HTTPResponseHead) -> ())? - var receiveResponseBodyPartsCallback: ((_ buffer: CircularBuffer) -> ())? - var succeedRequestCallback: ((_ buffer: CircularBuffer?) -> ())? - var failCallback: ((_ error: Error) -> ())? - - + + var willExecuteRequestCallback: ((HTTPRequestExecutor) -> Void)? + var requestHeadSentCallback: (() -> Void)? + var resumeRequestBodyStreamCallback: (() -> Void)? + var pauseRequestBodyStreamCallback: (() -> Void)? + var receiveResponseHeadCallback: ((HTTPResponseHead) -> Void)? + var receiveResponseBodyPartsCallback: ((CircularBuffer) -> Void)? + var succeedRequestCallback: ((CircularBuffer?) -> Void)? + var failCallback: ((Error) -> Void)? + /// captures all ``HTTPExecutableRequest`` method calls in the order of occurrence, including arguments. /// If you are not interested in the arguments you can use `events.map(\.kind)` to get all events without arguments. private(set) var events: [Event] = [] - + init( head: NIOHTTP1.HTTPRequestHead = .init(version: .http1_1, method: .GET, uri: "http://localhost/"), framingMetadata: RequestFramingMetadata = .init(connectionClose: false, body: .fixedSize(0)), @@ -91,12 +91,12 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { self.file = file self.line = line } - + private func calledUnimplementedMethod(_ name: String) { - guard raiseErrorIfUnimplementedMethodIsCalled else { return } - XCTFail("\(name) invoked but it is not implemented", file: file, line: line) + guard self.raiseErrorIfUnimplementedMethodIsCalled else { return } + XCTFail("\(name) invoked but it is not implemented", file: self.file, line: self.line) } - + func willExecuteRequest(_ executor: HTTPRequestExecutor) { self.events.append(.willExecuteRequest(executor)) guard let willExecuteRequestCallback = willExecuteRequestCallback else { @@ -104,6 +104,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } willExecuteRequestCallback(executor) } + func requestHeadSent() { self.events.append(.requestHeadSent) guard let requestHeadSentCallback = requestHeadSentCallback else { @@ -111,6 +112,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } requestHeadSentCallback() } + func resumeRequestBodyStream() { self.events.append(.resumeRequestBodyStream) guard let resumeRequestBodyStreamCallback = resumeRequestBodyStreamCallback else { @@ -118,6 +120,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } resumeRequestBodyStreamCallback() } + func pauseRequestBodyStream() { self.events.append(.pauseRequestBodyStream) guard let pauseRequestBodyStreamCallback = pauseRequestBodyStreamCallback else { @@ -125,6 +128,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } pauseRequestBodyStreamCallback() } + func receiveResponseHead(_ head: HTTPResponseHead) { self.events.append(.receiveResponseHead(head)) guard let receiveResponseHeadCallback = receiveResponseHeadCallback else { @@ -132,6 +136,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } receiveResponseHeadCallback(head) } + func receiveResponseBodyParts(_ buffer: CircularBuffer) { self.events.append(.receiveResponseBodyParts(buffer)) guard let receiveResponseBodyPartsCallback = receiveResponseBodyPartsCallback else { @@ -139,6 +144,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } receiveResponseBodyPartsCallback(buffer) } + func succeedRequest(_ buffer: CircularBuffer?) { self.events.append(.succeedRequest(buffer)) guard let succeedRequestCallback = succeedRequestCallback else { @@ -146,6 +152,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { } succeedRequestCallback(buffer) } + func fail(_ error: Error) { self.events.append(.fail(error)) guard let failCallback = failCallback else { From efa86b5a6712bae7218743bf95caf89b447a803b Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 15:22:59 +0100 Subject: [PATCH 7/8] Remove `promise?.succeed(())` --- .../ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift | 1 - Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index 76335bc9b..fc2715237 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -157,7 +157,6 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { metadata: req.requestFramingMetadata ) self.run(action, context: context) - promise?.succeed(()) } func read(context: ChannelHandlerContext) { diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index 29d373cd7..820e6cf10 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -554,7 +554,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { // non empty body is important to trigger this bug as we otherwise finish the request in a single flush request.requestFramingMetadata.body = .fixedSize(1) request.raiseErrorIfUnimplementedMethodIsCalled = false - try channel.writeOutbound(request) + channel.writeAndFlush(request, promise: nil) XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent]) } } From 5f5dc1e3e5c2d0610bf0a8a18b83d04034cdd8b3 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 26 Jan 2023 00:11:09 +0100 Subject: [PATCH 8/8] Rename `onRequestCompleted` to `onConnectionIdle` --- .../ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift | 8 ++++---- .../ConnectionPool/HTTP1/HTTP1Connection.swift | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index fc2715237..626a6fc23 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -62,7 +62,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { private let eventLoop: EventLoop private let connectionIdLoggerMetadata: Logger.MetadataValue - var onRequestCompleted: () -> Void = {} + var onConnectionIdle: () -> Void = {} init(eventLoop: EventLoop, backgroundLogger: Logger, connectionIdLoggerMetadata: Logger.MetadataValue) { self.eventLoop = eventLoop self.backgroundLogger = backgroundLogger @@ -275,7 +275,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { if shouldClose { context.close(promise: nil) } else { - self.onRequestCompleted() + self.onConnectionIdle() } oldRequest.succeedRequest(buffer) @@ -287,7 +287,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: writePromise) case .informConnectionIsIdle: - self.onRequestCompleted() + self.onConnectionIdle() oldRequest.succeedRequest(buffer) } @@ -304,7 +304,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { oldRequest.fail(error) case .informConnectionIsIdle: - self.onRequestCompleted() + self.onConnectionIdle() oldRequest.fail(error) case .failWritePromise(let writePromise): diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift index 7962e4df7..ee0a78498 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift @@ -137,7 +137,7 @@ final class HTTP1Connection { backgroundLogger: logger, connectionIdLoggerMetadata: "\(self.id)" ) - channelHandler.onRequestCompleted = { + channelHandler.onConnectionIdle = { self.taskCompleted() }