From 0aecc2be52b36387a109543bbbb12361c65ed979 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 24 Jan 2023 20:27:15 +0100 Subject: [PATCH 01/16] 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 02/16] 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 03/16] 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 04/16] 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 05/16] 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 06/16] 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 07/16] 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 38b3bf0ccbd40474c2d9bd67de4e00f0c0864fe0 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 19:36:37 +0100 Subject: [PATCH 08/16] Add test for HTTP2 request with large header Motivation We currently don't handle large headers well which trigger a channel writability change event. Modification Add failing (but currently skipped) tests which reproduces the issue Result We can reliably reproduce the large request header issue in an integration and unit test. Note that the actual fix is not included to make reviewing easier and will come in a follow up PR. --- .../HTTP2ClientRequestHandlerTests.swift | 29 +++++++++++++++ .../HTTPClientTestUtils.swift | 25 +++++++++---- .../HTTPClientTests.swift | 36 +++++++++++++++++++ 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift index e67529ad8..58c787396 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift @@ -345,4 +345,33 @@ class HTTP2ClientRequestHandlerTests: XCTestCase { XCTAssertEqual(embedded.isActive, false) } } + + 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) { + context.flush() + (context.channel as! EmbeddedChannel).isWritable = false + context.fireChannelWritabilityChanged() + } + } + let eventLoopGroup = EmbeddedEventLoopGroup(loops: 1) + let eventLoop = eventLoopGroup.next() as! EmbeddedEventLoop + let handler = HTTP2ClientRequestHandler( + eventLoop: eventLoop + ) + let channel = EmbeddedChannel(handlers: [ + ChangeWritabilityOnFlush(), + 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) + request.raiseErrorIfUnimplementedMethodIsCalled = false + channel.writeAndFlush(request, promise: nil) + XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent]) + } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index e50dab3b6..ed78ee003 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -329,17 +329,32 @@ internal final class HTTPBin where // supports http1.1 connections only, which can be either plain text or encrypted case http1_1(ssl: Bool = false, compress: Bool = false) // supports http1.1 and http2 connections which must be always encrypted - case http2(compress: Bool) + case http2( + compress: Bool = false, + settings: HTTP2Settings? = nil + ) // supports request decompression and http response compression var compress: Bool { switch self { case .refuse: return false - case .http1_1(ssl: _, compress: let compress), .http2(compress: let compress): + case .http1_1(ssl: _, compress: let compress), .http2(compress: let compress, _): return compress } } + + var httpSettings: HTTP2Settings { + switch self { + case .http1_1, .http2(_, nil), .refuse: + return [ + HTTP2Setting(parameter: .maxConcurrentStreams, value: 100), + HTTP2Setting(parameter: .maxHeaderListSize, value: HPACKDecoder.defaultMaxHeaderListSize), + ] + case .http2(_, .some(let customSettings)): + return customSettings + } + } } enum Proxy { @@ -565,11 +580,7 @@ internal final class HTTPBin where // Successful upgrade to HTTP/2. Let the user configure the pipeline. let http2Handler = NIOHTTP2Handler( mode: .server, - initialSettings: [ - // TODO: make max concurrent streams configurable - HTTP2Setting(parameter: .maxConcurrentStreams, value: 10), - HTTP2Setting(parameter: .maxHeaderListSize, value: HPACKDecoder.defaultMaxHeaderListSize), - ] + initialSettings: self.mode.httpSettings ) let multiplexer = HTTP2StreamMultiplexer( mode: .server, diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 8e5d5fbfa..5a398147d 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3378,4 +3378,40 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertNoThrow(try defaultClient.execute(request: request).wait()) } + + func testMassiveHeaderHTTP2() throws { + try XCTSkipIf(true, "this currently crashes and will be fixed in follow up PR") + let bin = HTTPBin(.http2(settings: [ + .init(parameter: .maxConcurrentStreams, value: 100), + .init(parameter: .maxHeaderListSize, value: 1024 * 256), + .init(parameter: .maxFrameSize, value: 1024 * 256), + ])) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + let loggerFactor = StreamLogHandler.standardOutput(label:) + var bgLogger = Logger(label: "BG", factory: loggerFactor) + bgLogger.logLevel = .trace + + let client = HTTPClient( + eventLoopGroupProvider: .shared(clientGroup), + configuration: .init(certificateVerification: .none), + backgroundActivityLogger: bgLogger + ) + + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + var request = try HTTPClient.Request(url: bin.baseURL, method: .POST) + // add ~200 KB header + let headerValue = String(repeating: "0", count: 1024) + for headerID in 0..<200 { + 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])) + + var rqLogger = Logger(label: "RQ", factory: loggerFactor) + rqLogger.logLevel = .trace + XCTAssertNoThrow(try client.execute(request: request, logger: rqLogger).wait()) + } } From 8ad3d8a52bf535faa41f26e91fc7c08a991e2090 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 20:23:55 +0100 Subject: [PATCH 09/16] Remove logging --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 5a398147d..d836fc9e4 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3388,14 +3388,9 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { ])) defer { XCTAssertNoThrow(try bin.shutdown()) } - let loggerFactor = StreamLogHandler.standardOutput(label:) - var bgLogger = Logger(label: "BG", factory: loggerFactor) - bgLogger.logLevel = .trace - let client = HTTPClient( eventLoopGroupProvider: .shared(clientGroup), - configuration: .init(certificateVerification: .none), - backgroundActivityLogger: bgLogger + configuration: .init(certificateVerification: .none) ) defer { XCTAssertNoThrow(try client.syncShutdown()) } @@ -3410,8 +3405,6 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { // 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])) - var rqLogger = Logger(label: "RQ", factory: loggerFactor) - rqLogger.logLevel = .trace - XCTAssertNoThrow(try client.execute(request: request, logger: rqLogger).wait()) + XCTAssertNoThrow(try client.execute(request: request).wait()) } } From c9a28146cd77aeb3e720f6992316523ebf22ffdd Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 23:29:14 +0100 Subject: [PATCH 10/16] Fix crash for large HTTP request headers Fix crash for when sending HTTP request headers result in a channel writability change event --- .../HTTP1/HTTP1ClientChannelHandler.swift | 43 ++++------ .../HTTP1/HTTP1ConnectionStateMachine.swift | 24 +++++- .../HTTP2/HTTP2ClientRequestHandler.swift | 41 ++++------ .../HTTPRequestStateMachine.swift | 82 ++++++++++++++----- .../HTTP1ClientChannelHandlerTests.swift | 1 - .../HTTP1ConnectionStateMachineTests.swift | 40 ++++++--- .../HTTP2ClientRequestHandlerTests.swift | 1 - .../HTTPClientTests.swift | 2 - .../HTTPRequestStateMachineTests.swift | 77 +++++++++-------- 9 files changed, 190 insertions(+), 121 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index fc2715237..473dded46 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -183,9 +183,19 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { private func run(_ action: HTTP1ConnectionStateMachine.Action, context: ChannelHandlerContext) { switch action { - case .sendRequestHead(let head, startBody: let startBody): - self.sendRequestHead(head, startBody: startBody, context: context) - + case .sendRequestHead(let head, let sendEnd): + self.sendRequestHead(head, sendEnd: sendEnd, context: context) + case .notifyRequestHeadSendSuccessfully(let resumeRequestBodyStream, let startIdleTimer): + + request!.requestHeadSent() + if resumeRequestBodyStream { + request!.resumeRequestBodyStream() + } + if startIdleTimer { + if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { + self.runTimeoutAction(timeoutAction, context: context) + } + } case .sendBodyPart(let part, let writePromise): context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: writePromise) @@ -320,32 +330,15 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { } } - private func sendRequestHead(_ head: HTTPRequestHead, startBody: Bool, context: ChannelHandlerContext) { - if startBody { - context.writeAndFlush(self.wrapOutboundOut(.head(head)), promise: nil) - - // The above write might trigger an error, which may lead to a call to `errorCaught`, - // which in turn, may fail the request and pop it from the handler. For this reason - // we must check if the request is still present here. - guard let request = self.request else { return } - request.requestHeadSent() - - request.resumeRequestBodyStream() - } else { + private func sendRequestHead(_ head: HTTPRequestHead, sendEnd: Bool, context: ChannelHandlerContext) { + if sendEnd { context.write(self.wrapOutboundOut(.head(head)), promise: nil) context.write(self.wrapOutboundOut(.end(nil)), promise: nil) context.flush() - - // The above write might trigger an error, which may lead to a call to `errorCaught`, - // which in turn, may fail the request and pop it from the handler. For this reason - // we must check if the request is still present here. - guard let request = self.request else { return } - request.requestHeadSent() - - if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { - self.runTimeoutAction(timeoutAction, context: context) - } + } else { + context.writeAndFlush(self.wrapOutboundOut(.head(head)), promise: nil) } + self.run(self.state.headSent(), context: context) } private func runTimeoutAction(_ action: IdleReadStateMachine.Action, context: ChannelHandlerContext) { diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift index e7258611c..6e79383ec 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift @@ -57,7 +57,11 @@ struct HTTP1ConnectionStateMachine { case none } - case sendRequestHead(HTTPRequestHead, startBody: Bool) + case sendRequestHead(HTTPRequestHead, sendEnd: Bool) + case notifyRequestHeadSendSuccessfully( + resumeRequestBodyStream: Bool, + startIdleTimer: Bool + ) case sendBodyPart(IOData, EventLoopPromise?) case sendRequestEnd(EventLoopPromise?) case failSendBodyPart(Error, EventLoopPromise?) @@ -350,6 +354,17 @@ struct HTTP1ConnectionStateMachine { return state.modify(with: action) } } + + mutating func headSent() -> Action { + guard case .inRequest(var requestStateMachine, let close) = state else { + return .wait + } + return self.avoidingStateMachineCoW { state in + let action = requestStateMachine.headSent() + state = .inRequest(requestStateMachine, close: close) + return state.modify(with: action) + } + } } extension HTTP1ConnectionStateMachine { @@ -390,8 +405,10 @@ extension HTTP1ConnectionStateMachine { extension HTTP1ConnectionStateMachine.State { fileprivate mutating func modify(with action: HTTPRequestStateMachine.Action) -> HTTP1ConnectionStateMachine.Action { switch action { - case .sendRequestHead(let head, let startBody): - return .sendRequestHead(head, startBody: startBody) + case .sendRequestHead(let head, let sendEnd): + return .sendRequestHead(head, sendEnd: sendEnd) + case .notifyRequestHeadSendSuccessfully(let resumeRequestBodyStream, let startIdleTimer): + return .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: resumeRequestBodyStream, startIdleTimer: startIdleTimer) case .pauseRequestBodyStream: return .pauseRequestBodyStream case .resumeRequestBodyStream: @@ -462,6 +479,7 @@ extension HTTP1ConnectionStateMachine.State { case .failSendStreamFinished(let error, let writePromise): return .failSendStreamFinished(error, writePromise) + } } } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift index 578b83029..96ee23bd6 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift @@ -140,9 +140,18 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { private func run(_ action: HTTPRequestStateMachine.Action, context: ChannelHandlerContext) { switch action { - case .sendRequestHead(let head, let startBody): - self.sendRequestHead(head, startBody: startBody, context: context) - + case .sendRequestHead(let head, let sendEnd): + self.sendRequestHead(head, sendEnd: sendEnd, context: context) + case .notifyRequestHeadSendSuccessfully(let resumeRequestBodyStream, let startIdleTimer): + request!.requestHeadSent() + if resumeRequestBodyStream { + request!.resumeRequestBodyStream() + } + if startIdleTimer { + if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { + self.runTimeoutAction(timeoutAction, context: context) + } + } case .pauseRequestBodyStream: // We can force unwrap the request here, as we have just validated in the state machine, // that the request is neither failed nor finished yet @@ -210,31 +219,15 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { } } - private func sendRequestHead(_ head: HTTPRequestHead, startBody: Bool, context: ChannelHandlerContext) { - if startBody { - context.writeAndFlush(self.wrapOutboundOut(.head(head)), promise: nil) - - // The above write might trigger an error, which may lead to a call to `errorCaught`, - // which in turn, may fail the request and pop it from the handler. For this reason - // we must check if the request is still present here. - guard let request = self.request else { return } - request.requestHeadSent() - request.resumeRequestBodyStream() - } else { + private func sendRequestHead(_ head: HTTPRequestHead, sendEnd: Bool, context: ChannelHandlerContext) { + if sendEnd { context.write(self.wrapOutboundOut(.head(head)), promise: nil) context.write(self.wrapOutboundOut(.end(nil)), promise: nil) context.flush() - - // The above write might trigger an error, which may lead to a call to `errorCaught`, - // which in turn, may fail the request and pop it from the handler. For this reason - // we must check if the request is still present here. - guard let request = self.request else { return } - request.requestHeadSent() - - if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { - self.runTimeoutAction(timeoutAction, context: context) - } + } else { + context.writeAndFlush(self.wrapOutboundOut(.head(head)), promise: nil) } + self.run(self.state.headSent(), context: context) } private func runSuccessfulFinalAction(_ action: HTTPRequestStateMachine.Action.FinalSuccessfulRequestAction, context: ChannelHandlerContext) { diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift index aafa3d28b..009fd964f 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift @@ -20,21 +20,24 @@ struct HTTPRequestStateMachine { fileprivate enum State { /// The initial state machine state. The only valid mutation is `start()`. The state will /// transitions to: - /// - `.waitForChannelToBecomeWritable` - /// - `.running(.streaming, .initialized)` (if the Channel is writable and if a request body is expected) - /// - `.running(.endSent, .initialized)` (if the Channel is writable and no request body is expected) + /// - `.waitForChannelToBecomeWritable` (if the channel becomes non writable while sending the header) + /// - `.sendingHead` if the channel is writable case initialized + /// Waiting for the channel to be writable. Valid transitions are: - /// - `.running(.streaming, .initialized)` (once the Channel is writable again and if a request body is expected) - /// - `.running(.endSent, .initialized)` (once the Channel is writable again and no request body is expected) + /// - `.running(.streaming, .waitingForHead)` (once the Channel is writable again and if a request body is expected) + /// - `.running(.endSent, .waitingForHead)` (once the Channel is writable again and no request body is expected) /// - `.failed` (if a connection error occurred) case waitForChannelToBecomeWritable(HTTPRequestHead, RequestFramingMetadata) + /// A request is on the wire. Valid transitions are: /// - `.finished` /// - `.failed` case running(RequestState, ResponseState) + /// The request has completed successfully case finished + /// The request has failed case failed(Error) @@ -93,7 +96,11 @@ struct HTTPRequestStateMachine { case none } - case sendRequestHead(HTTPRequestHead, startBody: Bool) + case sendRequestHead(HTTPRequestHead, sendEnd: Bool) + case notifyRequestHeadSendSuccessfully( + resumeRequestBodyStream: Bool, + startIdleTimer: Bool + ) case sendBodyPart(IOData, EventLoopPromise?) case sendRequestEnd(EventLoopPromise?) case failSendBodyPart(Error, EventLoopPromise?) @@ -223,6 +230,7 @@ struct HTTPRequestStateMachine { // the request failed, before it was sent onto the wire. self.state = .failed(error) return .failRequest(error, .none) + case .running: self.state = .failed(error) return .failRequest(error, .close(nil)) @@ -520,7 +528,7 @@ struct HTTPRequestStateMachine { switch self.state { case .initialized, .waitForChannelToBecomeWritable: - preconditionFailure("How can we receive a response head before sending a request head ourselves") + preconditionFailure("How can we receive a response head before sending a request head ourselves \(self.state)") case .running(.streaming(let expectedBodyLength, let sentBodyBytes, producer: .paused), .waitingForHead): self.state = .running( @@ -561,7 +569,7 @@ struct HTTPRequestStateMachine { mutating func receivedHTTPResponseBodyPart(_ body: ByteBuffer) -> Action { switch self.state { case .initialized, .waitForChannelToBecomeWritable: - preconditionFailure("How can we receive a response head before sending a request head ourselves. Invalid state: \(self.state)") + preconditionFailure("How can we receive a response head before completely sending a request head ourselves. Invalid state: \(self.state)") case .running(_, .waitingForHead): preconditionFailure("How can we receive a response body, if we haven't received a head. Invalid state: \(self.state)") @@ -587,7 +595,7 @@ struct HTTPRequestStateMachine { private mutating func receivedHTTPResponseEnd() -> Action { switch self.state { case .initialized, .waitForChannelToBecomeWritable: - preconditionFailure("How can we receive a response head before sending a request head ourselves. Invalid state: \(self.state)") + preconditionFailure("How can we receive a response end before completely sending a request head ourselves. Invalid state: \(self.state)") case .running(_, .waitingForHead): preconditionFailure("How can we receive a response end, if we haven't a received a head. Invalid state: \(self.state)") @@ -654,7 +662,7 @@ struct HTTPRequestStateMachine { case .initialized, .running(_, .waitingForHead), .waitForChannelToBecomeWritable: - preconditionFailure("The response is expected to only ask for more data after the response head was forwarded") + preconditionFailure("The response is expected to only ask for more data after the response head was forwarded \(self.state)") case .running(let requestState, .receivingBody(let head, var responseStreamState)): return self.avoidingStateMachineCoW { state -> Action in @@ -697,18 +705,52 @@ struct HTTPRequestStateMachine { } private mutating func startSendingRequest(head: HTTPRequestHead, metadata: RequestFramingMetadata) -> Action { - switch metadata.body { - case .stream: - self.state = .running(.streaming(expectedBodyLength: nil, sentBodyBytes: 0, producer: .producing), .waitingForHead) - return .sendRequestHead(head, startBody: true) - case .fixedSize(0): + let length = metadata.body.expectedLength + if length == 0 { // no body self.state = .running(.endSent, .waitingForHead) - return .sendRequestHead(head, startBody: false) - case .fixedSize(let length): - // length is greater than zero and we therefore have a body to send - self.state = .running(.streaming(expectedBodyLength: length, sentBodyBytes: 0, producer: .producing), .waitingForHead) - return .sendRequestHead(head, startBody: true) + return .sendRequestHead(head, sendEnd: true) + } else { + self.state = .running(.streaming(expectedBodyLength: length, sentBodyBytes: 0, producer: .paused), .waitingForHead) + return .sendRequestHead(head, sendEnd: false) + } + } + + mutating func headSent() -> Action { + switch self.state { + case .initialized, .waitForChannelToBecomeWritable, .finished: + preconditionFailure("Not a valid transition after `.sendingHeader`: \(self.state)") + + case .running(.streaming(let expectedBodyLength, let sentBodyBytes, producer: .paused), let responseState): + let startProducing = self.isChannelWritable && expectedBodyLength != sentBodyBytes + self.state = .running(.streaming( + expectedBodyLength: expectedBodyLength, + sentBodyBytes: sentBodyBytes, + producer: startProducing ? .producing : .paused + ), responseState) + return .notifyRequestHeadSendSuccessfully( + resumeRequestBodyStream: startProducing, + startIdleTimer: false + ) + case .running(.endSent, _): + return .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true) + case .running(.streaming(_, _, producer: .producing), _): + preconditionFailure("request body producing can not start before we have successfully send the header \(self.state)") + case .failed: + return .wait + + case .modifying: + preconditionFailure("Invalid state: \(self.state)") + + } + } +} + +extension RequestFramingMetadata.Body { + var expectedLength: Int? { + switch self { + case .fixedSize(let length): return length + case .stream: return nil } } } diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index 820e6cf10..bdf897b3d 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -528,7 +528,6 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { } 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) { diff --git a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift index fd771aca0..8032917fb 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift @@ -26,7 +26,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: ["content-length": "4"]) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4)) XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata), .wait) - XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, sendEnd: false)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: true, startIdleTimer: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0])) let part1 = IOData.byteBuffer(ByteBuffer(bytes: [1])) @@ -64,7 +65,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["content-length": "12"]) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -92,7 +94,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["connection": "close"]) let metadata = RequestFramingMetadata(connectionClose: true, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -108,7 +111,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_0, status: .ok, headers: ["content-length": "4"]) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -124,7 +128,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_0, status: .ok, headers: ["content-length": "4", "connection": "keep-alive"]) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -141,7 +146,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["connection": "close"]) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -170,9 +176,11 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none)) + + XCTAssertEqual(state.headSent(), .wait) } func testRequestWasCancelledWhileUploadingData() { @@ -182,7 +190,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: ["content-length": "4"]) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4)) XCTAssertEqual(state.runNewRequest(head: requestHead, metadata: metadata), .wait) - XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, sendEnd: false)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: true, startIdleTimer: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0])) let part1 = IOData.byteBuffer(ByteBuffer(bytes: [1])) @@ -235,7 +244,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) XCTAssertEqual(state.channelRead(.body(ByteBuffer(string: "Hello world!\n"))), .wait) @@ -250,7 +260,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .switchingProtocols) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) XCTAssertEqual(state.channelRead(.end(nil)), .succeedRequest(.close, [])) @@ -262,7 +273,8 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) - XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .init(statusCode: 103, reasonPhrase: "Early Hints")) XCTAssertEqual(state.channelRead(.head(responseHead)), .wait) XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none)) @@ -294,6 +306,12 @@ extension HTTP1ConnectionStateMachine.Action: Equatable { case (.sendRequestHead(let lhsHead, let lhsStartBody), .sendRequestHead(let rhsHead, let rhsStartBody)): return lhsHead == rhsHead && lhsStartBody == rhsStartBody + + case ( + .notifyRequestHeadSendSuccessfully(let lhsResumeRequestBodyStream, let lhsStartIdleTimer), + .notifyRequestHeadSendSuccessfully(let rhsResumeRequestBodyStream, let rhsStartIdleTimer) + ): + return lhsResumeRequestBodyStream == rhsResumeRequestBodyStream && lhsStartIdleTimer == rhsStartIdleTimer case (.sendBodyPart(let lhsData, let lhsPromise), .sendBodyPart(let rhsData, let rhsPromise)): return lhsData == rhsData && lhsPromise?.futureResult == rhsPromise?.futureResult diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift index 58c787396..195c948ac 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift @@ -347,7 +347,6 @@ class HTTP2ClientRequestHandlerTests: XCTestCase { } 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) { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index d836fc9e4..414f25be5 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3365,7 +3365,6 @@ 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) @@ -3380,7 +3379,6 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { } func testMassiveHeaderHTTP2() throws { - try XCTSkipIf(true, "this currently crashes and will be fixed in follow up PR") let bin = HTTPBin(.http2(settings: [ .init(parameter: .maxConcurrentStreams, value: 100), .init(parameter: .maxHeaderListSize, value: 1024 * 256), diff --git a/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift index 61ea4702b..6609451c2 100644 --- a/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift @@ -24,7 +24,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -38,7 +38,8 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "4")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: true, startIdleTimer: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0])) let part1 = IOData.byteBuffer(ByteBuffer(bytes: [1])) let part2 = IOData.byteBuffer(ByteBuffer(bytes: [2])) @@ -72,7 +73,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "4")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0, 1, 2, 3])) let part1 = IOData.byteBuffer(ByteBuffer(bytes: [0, 1, 2, 3])) XCTAssertEqual(state.requestStreamPartReceived(part0, promise: nil), .sendBodyPart(part0, nil)) @@ -87,7 +88,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "8")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(8)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: [0, 1, 2, 3])) XCTAssertEqual(state.requestStreamPartReceived(part0, promise: nil), .sendBodyPart(part0, nil)) @@ -98,7 +99,8 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "12")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(12)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: true, startIdleTimer: false)) let part = IOData.byteBuffer(ByteBuffer(bytes: [0, 1, 2, 3])) XCTAssertEqual(state.requestStreamPartReceived(part, promise: nil), .sendBodyPart(part, nil)) @@ -132,7 +134,8 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "12")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(12)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) + XCTAssertEqual(state.headSent(), .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: true, startIdleTimer: false)) let part = IOData.byteBuffer(ByteBuffer(bytes: [0, 1, 2, 3])) XCTAssertEqual(state.requestStreamPartReceived(part, promise: nil), .sendBodyPart(part, nil)) XCTAssertEqual(state.writabilityChanged(writable: false), .pauseRequestBodyStream) @@ -157,7 +160,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "12")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(12)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: 0...3)) XCTAssertEqual(state.requestStreamPartReceived(part0, promise: nil), .sendBodyPart(part0, nil)) @@ -179,7 +182,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "12")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(12)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: 0...3)) XCTAssertEqual(state.requestStreamPartReceived(part0, promise: nil), .sendBodyPart(part0, nil)) @@ -200,7 +203,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "12")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(12)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: 0...3)) XCTAssertEqual(state.requestStreamPartReceived(part0, promise: nil), .sendBodyPart(part0, nil)) @@ -219,7 +222,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: HTTPHeaders([("content-length", "12")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(12)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) let part0 = IOData.byteBuffer(ByteBuffer(bytes: 0...3)) XCTAssertEqual(state.requestStreamPartReceived(part0, promise: nil), .sendBodyPart(part0, nil)) @@ -239,7 +242,7 @@ class HTTPRequestStateMachineTests: XCTestCase { let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .wait) XCTAssertEqual(state.read(), .read) - XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.writabilityChanged(writable: true), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -261,7 +264,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: HTTPHeaders([("content-length", "12")])) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -288,7 +291,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: HTTPHeaders([("content-length", "12")])) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -315,7 +318,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: HTTPHeaders([("content-length", "12")])) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -362,7 +365,7 @@ class HTTPRequestStateMachineTests: XCTestCase { // --- sending request let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) // --- receiving response let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["content-length": "4"]) @@ -377,7 +380,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) state.requestCancelled().assertFailRequest(HTTPClientError.cancelled, .close(nil)) } @@ -385,7 +388,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: .init([("content-length", "4")])) let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) state.requestCancelled().assertFailRequest(HTTPClientError.cancelled, .close(nil)) XCTAssertEqual(state.requestStreamPartReceived(.byteBuffer(.init(bytes: 1...3)), promise: nil), .failSendBodyPart(HTTPClientError.cancelled, nil)) } @@ -394,7 +397,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: HTTPHeaders([("content-length", "12")])) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -411,7 +414,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let continueHead = HTTPResponseHead(version: .http1_1, status: .continue) XCTAssertEqual(state.channelRead(.head(continueHead)), .wait) @@ -427,7 +430,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -439,7 +442,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -451,7 +454,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) state.errorHappened(HTTPParserError.invalidChunkSize).assertFailRequest(HTTPParserError.invalidChunkSize, .close(nil)) XCTAssertEqual(state.requestCancelled(), .wait, "A cancellation that happens to late is ignored") @@ -461,7 +464,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_0, status: .internalServerError) XCTAssertEqual(state.channelRead(.head(responseHead)), .forwardResponseHead(responseHead, pauseRequestBodyStream: false)) @@ -477,7 +480,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_0, status: .internalServerError) let body = ByteBuffer(string: "foo bar") @@ -495,7 +498,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .stream) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: true)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: false)) let part1: ByteBuffer = .init(string: "foo") XCTAssertEqual(state.requestStreamPartReceived(.byteBuffer(part1), promise: nil), .sendBodyPart(.byteBuffer(part1), nil)) @@ -515,7 +518,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok) let body = ByteBuffer(string: "foo bar") @@ -531,7 +534,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) XCTAssertEqual(state.errorHappened(NIOSSLError.uncleanShutdown), .wait) state.channelInactive().assertFailRequest(HTTPClientError.remoteConnectionClosed, .none) @@ -542,7 +545,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) state.errorHappened(ArbitraryError()).assertFailRequest(ArbitraryError(), .close(nil)) XCTAssertEqual(state.channelInactive(), .wait) @@ -552,7 +555,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["content-length": "30"]) let body = ByteBuffer(string: "foo bar") @@ -570,7 +573,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["Content-Length": "50"]) let body = ByteBuffer(string: "foo bar") @@ -591,7 +594,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["Content-Length": "50"]) let body = ByteBuffer(string: "foo bar") @@ -612,7 +615,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["Content-Length": "50"]) let body = ByteBuffer(string: "foo bar") @@ -632,7 +635,7 @@ class HTTPRequestStateMachineTests: XCTestCase { var state = HTTPRequestStateMachine(isChannelWritable: true) let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) - XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, startBody: false)) + XCTAssertEqual(state.startRequest(head: requestHead, metadata: metadata), .sendRequestHead(requestHead, sendEnd: true)) let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["Content-Length": "50"]) let body = ByteBuffer(string: "foo bar") @@ -667,7 +670,13 @@ extension HTTPRequestStateMachine.Action: Equatable { switch (lhs, rhs) { case (.sendRequestHead(let lhsHead, let lhsStartBody), .sendRequestHead(let rhsHead, let rhsStartBody)): return lhsHead == rhsHead && lhsStartBody == rhsStartBody - + + case ( + .notifyRequestHeadSendSuccessfully(let lhsResumeRequestBodyStream, let lhsStartIdleTimer), + .notifyRequestHeadSendSuccessfully(let rhsResumeRequestBodyStream, let rhsStartIdleTimer) + ): + return lhsResumeRequestBodyStream == rhsResumeRequestBodyStream && lhsStartIdleTimer == rhsStartIdleTimer + case (.sendBodyPart(let lhsData, let lhsPromise), .sendBodyPart(let rhsData, let rhsPromise)): return lhsData == rhsData && lhsPromise?.futureResult == rhsPromise?.futureResult From 5fb70dc77d3e6b4dd6eb79535b7b611a59f6ecd4 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 23:36:15 +0100 Subject: [PATCH 11/16] Formatting and linux tests --- .../HTTP2ClientRequestHandlerTests+XCTest.swift | 1 + .../HTTP2ClientRequestHandlerTests.swift | 2 +- Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift | 2 +- Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 1 + Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 8 ++++---- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift index 8fa219838..221a63211 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift @@ -30,6 +30,7 @@ extension HTTP2ClientRequestHandlerTests { ("testIdleReadTimeout", testIdleReadTimeout), ("testIdleReadTimeoutIsCanceledIfRequestIsCanceled", testIdleReadTimeoutIsCanceledIfRequestIsCanceled), ("testWriteHTTPHeadFails", testWriteHTTPHeadFails), + ("testChannelBecomesNonWritableDuringHeaderWrite", testChannelBecomesNonWritableDuringHeaderWrite), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift index 58c787396..5dfce3f9d 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift @@ -345,7 +345,7 @@ class HTTP2ClientRequestHandlerTests: XCTestCase { XCTAssertEqual(embedded.isActive, false) } } - + func testChannelBecomesNonWritableDuringHeaderWrite() throws { try XCTSkipIf(true, "this currently fails and will be fixed in follow up PR") final class ChangeWritabilityOnFlush: ChannelOutboundHandler { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index ed78ee003..07753cf89 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -343,7 +343,7 @@ internal final class HTTPBin where return compress } } - + var httpSettings: HTTP2Settings { switch self { case .http1_1, .http2(_, nil), .refuse: diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index f9ddb1c8b..6e84f9d29 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -144,6 +144,7 @@ extension HTTPClientTests { ("testMassiveDownload", testMassiveDownload), ("testShutdownWithFutures", testShutdownWithFutures), ("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1), + ("testMassiveHeaderHTTP2", testMassiveHeaderHTTP2), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index d836fc9e4..54d854bf0 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3378,7 +3378,7 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertNoThrow(try defaultClient.execute(request: request).wait()) } - + func testMassiveHeaderHTTP2() throws { try XCTSkipIf(true, "this currently crashes and will be fixed in follow up PR") let bin = HTTPBin(.http2(settings: [ @@ -3387,14 +3387,14 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { .init(parameter: .maxFrameSize, value: 1024 * 256), ])) defer { XCTAssertNoThrow(try bin.shutdown()) } - + let client = HTTPClient( eventLoopGroupProvider: .shared(clientGroup), configuration: .init(certificateVerification: .none) ) - + defer { XCTAssertNoThrow(try client.syncShutdown()) } - + var request = try HTTPClient.Request(url: bin.baseURL, method: .POST) // add ~200 KB header let headerValue = String(repeating: "0", count: 1024) From 183f75e039ea71bcc79d49e2cfc2d6d278da6d65 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 25 Jan 2023 23:36:55 +0100 Subject: [PATCH 12/16] Formatting and linux tests --- .../HTTP1/HTTP1ClientChannelHandler.swift | 6 +++--- .../HTTP1/HTTP1ConnectionStateMachine.swift | 5 ++--- .../HTTP2/HTTP2ClientRequestHandler.swift | 4 ++-- .../ConnectionPool/HTTPRequestStateMachine.swift | 15 +++++++-------- .../HTTP1ConnectionStateMachineTests.swift | 4 ++-- .../HTTP2ClientRequestHandlerTests+XCTest.swift | 1 + .../HTTP2ClientRequestHandlerTests.swift | 2 +- .../HTTPClientTestUtils.swift | 2 +- .../HTTPClientTests+XCTest.swift | 1 + Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 8 ++++---- .../HTTPRequestStateMachineTests.swift | 4 ++-- 11 files changed, 26 insertions(+), 26 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index 473dded46..9aeb4c6e1 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -186,10 +186,10 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { case .sendRequestHead(let head, let sendEnd): self.sendRequestHead(head, sendEnd: sendEnd, context: context) case .notifyRequestHeadSendSuccessfully(let resumeRequestBodyStream, let startIdleTimer): - - request!.requestHeadSent() + + self.request!.requestHeadSent() if resumeRequestBodyStream { - request!.resumeRequestBodyStream() + self.request!.resumeRequestBodyStream() } if startIdleTimer { if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift index 6e79383ec..a908ded9a 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift @@ -354,9 +354,9 @@ struct HTTP1ConnectionStateMachine { return state.modify(with: action) } } - + mutating func headSent() -> Action { - guard case .inRequest(var requestStateMachine, let close) = state else { + guard case .inRequest(var requestStateMachine, let close) = self.state else { return .wait } return self.avoidingStateMachineCoW { state in @@ -479,7 +479,6 @@ extension HTTP1ConnectionStateMachine.State { case .failSendStreamFinished(let error, let writePromise): return .failSendStreamFinished(error, writePromise) - } } } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift index 96ee23bd6..ad169596d 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift @@ -143,9 +143,9 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { case .sendRequestHead(let head, let sendEnd): self.sendRequestHead(head, sendEnd: sendEnd, context: context) case .notifyRequestHeadSendSuccessfully(let resumeRequestBodyStream, let startIdleTimer): - request!.requestHeadSent() + self.request!.requestHeadSent() if resumeRequestBodyStream { - request!.resumeRequestBodyStream() + self.request!.resumeRequestBodyStream() } if startIdleTimer { if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift index 009fd964f..4835feac3 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift @@ -23,21 +23,21 @@ struct HTTPRequestStateMachine { /// - `.waitForChannelToBecomeWritable` (if the channel becomes non writable while sending the header) /// - `.sendingHead` if the channel is writable case initialized - + /// Waiting for the channel to be writable. Valid transitions are: /// - `.running(.streaming, .waitingForHead)` (once the Channel is writable again and if a request body is expected) /// - `.running(.endSent, .waitingForHead)` (once the Channel is writable again and no request body is expected) /// - `.failed` (if a connection error occurred) case waitForChannelToBecomeWritable(HTTPRequestHead, RequestFramingMetadata) - + /// A request is on the wire. Valid transitions are: /// - `.finished` /// - `.failed` case running(RequestState, ResponseState) - + /// The request has completed successfully case finished - + /// The request has failed case failed(Error) @@ -715,12 +715,12 @@ struct HTTPRequestStateMachine { return .sendRequestHead(head, sendEnd: false) } } - + mutating func headSent() -> Action { switch self.state { case .initialized, .waitForChannelToBecomeWritable, .finished: preconditionFailure("Not a valid transition after `.sendingHeader`: \(self.state)") - + case .running(.streaming(let expectedBodyLength, let sentBodyBytes, producer: .paused), let responseState): let startProducing = self.isChannelWritable && expectedBodyLength != sentBodyBytes self.state = .running(.streaming( @@ -738,10 +738,9 @@ struct HTTPRequestStateMachine { preconditionFailure("request body producing can not start before we have successfully send the header \(self.state)") case .failed: return .wait - + case .modifying: preconditionFailure("Invalid state: \(self.state)") - } } } diff --git a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift index 8032917fb..ce8e6ed17 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift @@ -179,7 +179,7 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true)) XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none)) - + XCTAssertEqual(state.headSent(), .wait) } @@ -306,7 +306,7 @@ extension HTTP1ConnectionStateMachine.Action: Equatable { case (.sendRequestHead(let lhsHead, let lhsStartBody), .sendRequestHead(let rhsHead, let rhsStartBody)): return lhsHead == rhsHead && lhsStartBody == rhsStartBody - + case ( .notifyRequestHeadSendSuccessfully(let lhsResumeRequestBodyStream, let lhsStartIdleTimer), .notifyRequestHeadSendSuccessfully(let rhsResumeRequestBodyStream, let rhsStartIdleTimer) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift index 8fa219838..221a63211 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift @@ -30,6 +30,7 @@ extension HTTP2ClientRequestHandlerTests { ("testIdleReadTimeout", testIdleReadTimeout), ("testIdleReadTimeoutIsCanceledIfRequestIsCanceled", testIdleReadTimeoutIsCanceledIfRequestIsCanceled), ("testWriteHTTPHeadFails", testWriteHTTPHeadFails), + ("testChannelBecomesNonWritableDuringHeaderWrite", testChannelBecomesNonWritableDuringHeaderWrite), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift index 195c948ac..4873bc169 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift @@ -345,7 +345,7 @@ class HTTP2ClientRequestHandlerTests: XCTestCase { XCTAssertEqual(embedded.isActive, false) } } - + func testChannelBecomesNonWritableDuringHeaderWrite() throws { final class ChangeWritabilityOnFlush: ChannelOutboundHandler { typealias OutboundIn = Any diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index ed78ee003..07753cf89 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -343,7 +343,7 @@ internal final class HTTPBin where return compress } } - + var httpSettings: HTTP2Settings { switch self { case .http1_1, .http2(_, nil), .refuse: diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index f9ddb1c8b..6e84f9d29 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -144,6 +144,7 @@ extension HTTPClientTests { ("testMassiveDownload", testMassiveDownload), ("testShutdownWithFutures", testShutdownWithFutures), ("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1), + ("testMassiveHeaderHTTP2", testMassiveHeaderHTTP2), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 414f25be5..10f150b6b 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3377,7 +3377,7 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertNoThrow(try defaultClient.execute(request: request).wait()) } - + func testMassiveHeaderHTTP2() throws { let bin = HTTPBin(.http2(settings: [ .init(parameter: .maxConcurrentStreams, value: 100), @@ -3385,14 +3385,14 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { .init(parameter: .maxFrameSize, value: 1024 * 256), ])) defer { XCTAssertNoThrow(try bin.shutdown()) } - + let client = HTTPClient( eventLoopGroupProvider: .shared(clientGroup), configuration: .init(certificateVerification: .none) ) - + defer { XCTAssertNoThrow(try client.syncShutdown()) } - + var request = try HTTPClient.Request(url: bin.baseURL, method: .POST) // add ~200 KB header let headerValue = String(repeating: "0", count: 1024) diff --git a/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift index 6609451c2..92bf42b1d 100644 --- a/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift @@ -670,13 +670,13 @@ extension HTTPRequestStateMachine.Action: Equatable { switch (lhs, rhs) { case (.sendRequestHead(let lhsHead, let lhsStartBody), .sendRequestHead(let rhsHead, let rhsStartBody)): return lhsHead == rhsHead && lhsStartBody == rhsStartBody - + case ( .notifyRequestHeadSendSuccessfully(let lhsResumeRequestBodyStream, let lhsStartIdleTimer), .notifyRequestHeadSendSuccessfully(let rhsResumeRequestBodyStream, let rhsStartIdleTimer) ): return lhsResumeRequestBodyStream == rhsResumeRequestBodyStream && lhsStartIdleTimer == rhsStartIdleTimer - + case (.sendBodyPart(let lhsData, let lhsPromise), .sendBodyPart(let rhsData, let rhsPromise)): return lhsData == rhsData && lhsPromise?.futureResult == rhsPromise?.futureResult From 7d96a89a047258c1e933dd79693fda4bbc801d1d Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 26 Jan 2023 13:50:09 +0100 Subject: [PATCH 13/16] Generate linux tests --- Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index f9ddb1c8b..6e84f9d29 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -144,6 +144,7 @@ extension HTTPClientTests { ("testMassiveDownload", testMassiveDownload), ("testShutdownWithFutures", testShutdownWithFutures), ("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1), + ("testMassiveHeaderHTTP2", testMassiveHeaderHTTP2), ] } } From 1ddafda133a3157f8789b528a7077e66648e4235 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 26 Jan 2023 13:52:17 +0100 Subject: [PATCH 14/16] Use previous default max concurrent streams value of 10 --- Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 07753cf89..ca24cba1c 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -348,7 +348,7 @@ internal final class HTTPBin where switch self { case .http1_1, .http2(_, nil), .refuse: return [ - HTTP2Setting(parameter: .maxConcurrentStreams, value: 100), + HTTP2Setting(parameter: .maxConcurrentStreams, value: 10), HTTP2Setting(parameter: .maxHeaderListSize, value: HPACKDecoder.defaultMaxHeaderListSize), ] case .http2(_, .some(let customSettings)): From 18f6505fd957e3ac2e8aeca148faa95f05987b38 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 9 Feb 2023 12:01:56 +0100 Subject: [PATCH 15/16] Fix crash if request is canceled after request header is send --- .../HTTP1/HTTP1ClientChannelHandler.swift | 10 ++++-- .../HTTP2/HTTP2ClientRequestHandler.swift | 9 ++++-- .../HTTPClientTests.swift | 32 +++++++++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index 27933fc98..8af70ac23 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -186,10 +186,14 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { case .sendRequestHead(let head, let sendEnd): self.sendRequestHead(head, sendEnd: sendEnd, context: context) case .notifyRequestHeadSendSuccessfully(let resumeRequestBodyStream, let startIdleTimer): - + // We can force unwrap the request here, as we have just validated in the state machine, + // that the request is neither failed nor finished yet self.request!.requestHeadSent() - if resumeRequestBodyStream { - self.request!.resumeRequestBodyStream() + if resumeRequestBodyStream, let request = self.request { + // The above request head send notification might lead the request to mark itself as + // cancelled, which in turn might pop the request of the handler. For this reason we + // must check if the request is still present here. + request.resumeRequestBodyStream() } if startIdleTimer { if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift index ad169596d..e7412f5c2 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift @@ -143,9 +143,14 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { case .sendRequestHead(let head, let sendEnd): self.sendRequestHead(head, sendEnd: sendEnd, context: context) case .notifyRequestHeadSendSuccessfully(let resumeRequestBodyStream, let startIdleTimer): + // We can force unwrap the request here, as we have just validated in the state machine, + // that the request is neither failed nor finished yet self.request!.requestHeadSent() - if resumeRequestBodyStream { - self.request!.resumeRequestBodyStream() + if resumeRequestBodyStream, let request = self.request { + // The above request head send notification might lead the request to mark itself as + // cancelled, which in turn might pop the request of the handler. For this reason we + // must check if the request is still present here. + request.resumeRequestBodyStream() } if startIdleTimer { if let timeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 10f150b6b..68beeb9a1 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3405,4 +3405,36 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertNoThrow(try client.execute(request: request).wait()) } + + func testCancelingHTTP1RequestAfterHeaderSend() throws { + var request = try HTTPClient.Request(url: self.defaultHTTPBin.baseURL + "/wait", method: .POST) + // non-empty body is important + request.body = .byteBuffer(ByteBuffer([1])) + + class CancelAfterHeadSend: HTTPClientResponseDelegate { + init() {} + func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws -> Void { } + func didSendRequestHead(task: HTTPClient.Task, _ head: HTTPRequestHead) { + task.cancel() + } + } + XCTAssertThrowsError(try defaultClient.execute(request: request, delegate: CancelAfterHeadSend()).wait()) + } + + func testCancelingHTTP2RequestAfterHeaderSend() throws { + let bin = HTTPBin(.http2()) + defer { XCTAssertNoThrow(try bin.shutdown()) } + var request = try HTTPClient.Request(url: bin.baseURL + "/wait", method: .POST) + // non-empty body is important + request.body = .byteBuffer(ByteBuffer([1])) + + class CancelAfterHeadSend: HTTPClientResponseDelegate { + init() {} + func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws -> Void { } + func didSendRequestHead(task: HTTPClient.Task, _ head: HTTPRequestHead) { + task.cancel() + } + } + XCTAssertThrowsError(try defaultClient.execute(request: request, delegate: CancelAfterHeadSend()).wait()) + } } From 7fcecdfd8b39ae32b09429e8f2aac200e5638724 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 9 Feb 2023 15:04:57 +0100 Subject: [PATCH 16/16] generate linux tests and run swift format --- .../HTTPClientTests+XCTest.swift | 2 ++ Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 12 ++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 6e84f9d29..d5a8160b6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -145,6 +145,8 @@ extension HTTPClientTests { ("testShutdownWithFutures", testShutdownWithFutures), ("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1), ("testMassiveHeaderHTTP2", testMassiveHeaderHTTP2), + ("testCancelingHTTP1RequestAfterHeaderSend", testCancelingHTTP1RequestAfterHeaderSend), + ("testCancelingHTTP2RequestAfterHeaderSend", testCancelingHTTP2RequestAfterHeaderSend), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 68beeb9a1..49f94a7d4 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3405,32 +3405,32 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertNoThrow(try client.execute(request: request).wait()) } - + func testCancelingHTTP1RequestAfterHeaderSend() throws { var request = try HTTPClient.Request(url: self.defaultHTTPBin.baseURL + "/wait", method: .POST) // non-empty body is important request.body = .byteBuffer(ByteBuffer([1])) - + class CancelAfterHeadSend: HTTPClientResponseDelegate { init() {} - func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws -> Void { } + func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws {} func didSendRequestHead(task: HTTPClient.Task, _ head: HTTPRequestHead) { task.cancel() } } XCTAssertThrowsError(try defaultClient.execute(request: request, delegate: CancelAfterHeadSend()).wait()) } - + func testCancelingHTTP2RequestAfterHeaderSend() throws { let bin = HTTPBin(.http2()) defer { XCTAssertNoThrow(try bin.shutdown()) } var request = try HTTPClient.Request(url: bin.baseURL + "/wait", method: .POST) // non-empty body is important request.body = .byteBuffer(ByteBuffer([1])) - + class CancelAfterHeadSend: HTTPClientResponseDelegate { init() {} - func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws -> Void { } + func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws {} func didSendRequestHead(task: HTTPClient.Task, _ head: HTTPRequestHead) { task.cancel() }