From 90e8b58b572105566c545d0cf3a022bed1ad5fea Mon Sep 17 00:00:00 2001 From: Rick Newton-Rogers Date: Tue, 28 Jan 2025 14:43:18 +0000 Subject: [PATCH 1/3] Avoid precondition failure in write timeout Motivation: In some cases we can crash because of a precondition failure when the write timeout fires and we aren't in the running state. This can happen for example if the connection is closed whilst the write timer is active. Modifications: Remove the precondition and instead take no action if the timeout fires outside of the running state. Instead we take a new `Action`, `.noAction` when the timer fires. Result: Fewer crashes. --- .../HTTP1/HTTP1ClientChannelHandler.swift | 2 +- .../HTTP1/HTTP1ConnectionStateMachine.swift | 4 +- .../HTTP1ClientChannelHandlerTests.swift | 75 +++++++++++++++++++ 3 files changed, 79 insertions(+), 2 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index 74a0c72d7..1d4c90c09 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -281,7 +281,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { case .close: context.close(promise: nil) - case .wait: + case .wait, .noAction: break case .forwardResponseHead(let head, let pauseRequestBodyStream): diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift index aee0736ff..17dfcd7e5 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift @@ -83,6 +83,8 @@ struct HTTP1ConnectionStateMachine { case fireChannelActive case fireChannelInactive case fireChannelError(Error, closeConnection: Bool) + + case noAction } private var state: State @@ -359,7 +361,7 @@ struct HTTP1ConnectionStateMachine { mutating func idleWriteTimeoutTriggered() -> Action { guard case .inRequest(var requestStateMachine, let close) = self.state else { - preconditionFailure("Invalid state: \(self.state)") + return .noAction } return self.avoidingStateMachineCoW { state -> Action in diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index 53af0823d..6a2770b8b 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -840,6 +840,81 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { channel.writeAndFlush(request, promise: nil) XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent]) } + + class SlowHandler: ChannelOutboundHandler { + typealias OutboundIn = HTTPClientRequestPart + typealias OutboundOut = HTTPClientRequestPart + + func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise?) { + context.eventLoop.scheduleTask(in: .milliseconds(300)) { + promise?.succeed() + } + } + } + + func testIdleWriteTimeoutOutsideOfRunningState() { + let embedded = EmbeddedChannel() + var maybeTestUtils: HTTP1TestTools? + XCTAssertNoThrow(maybeTestUtils = try embedded.setupHTTP1Connection()) + print("pipeline", embedded.pipeline) + guard let testUtils = maybeTestUtils else { return XCTFail("Expected connection setup works") } + + var maybeRequest: HTTPClient.Request? + XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "http://localhost/")) + guard var request = maybeRequest else { return XCTFail("Expected to be able to create a request") } + + // start a request stream we'll never write to + let streamPromise = embedded.eventLoop.makePromise(of: Void.self) + let streamCallback = { @Sendable (streamWriter: HTTPClient.Body.StreamWriter) -> EventLoopFuture in + streamPromise.futureResult + } + request.body = .init(contentLength: nil, stream: streamCallback) + + let delegate = NullResponseDelegate() + var maybeRequestBag: RequestBag? + XCTAssertNoThrow( + maybeRequestBag = try RequestBag( + request: request, + eventLoopPreference: .delegate(on: embedded.eventLoop), + task: .init(eventLoop: embedded.eventLoop, logger: testUtils.logger), + redirectHandler: nil, + connectionDeadline: .now() + .seconds(30), + requestOptions: .forTests( + idleReadTimeout: .milliseconds(10), + idleWriteTimeout: .milliseconds(2) + ), + delegate: delegate + ) + ) + guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") } + + testUtils.connection.executeRequest(requestBag) + + XCTAssertNoThrow( + try embedded.receiveHeadAndVerify { + XCTAssertEqual($0.method, .GET) + XCTAssertEqual($0.uri, "/") + XCTAssertEqual($0.headers.first(name: "host"), "localhost") + } + ) + + // close the pipeline to simulate a server-side close + // note this happens before we write so the idle write timeout is still running + try! embedded.pipeline.close().wait() + + // advance time to trigger the idle write timeout + // and ensure that the state machine can tolerate this + embedded.embeddedEventLoop.advanceTime(by: .milliseconds(250)) + } +} + +class NullResponseDelegate: HTTPClientResponseDelegate { + typealias Response = Void + + func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws { + () + } + } class TestBackpressureWriter { From 96bfda21996a2d070f959f3f39c574ca5690c00b Mon Sep 17 00:00:00 2001 From: Rick Newton-Rogers Date: Tue, 28 Jan 2025 15:30:50 +0000 Subject: [PATCH 2/3] Clear write timeouts upon request completion. When a request completes we have no use for the idle write timer, we clear the read timer and we should clear the write one too. --- .../ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift | 2 ++ .../ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift | 2 ++ 2 files changed, 4 insertions(+) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index 1d4c90c09..2a9accfe5 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -314,6 +314,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { let oldRequest = self.request! self.request = nil self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context) + self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context) switch finalAction { case .close: @@ -353,6 +354,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { let oldRequest = self.request! self.request = nil self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context) + self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context) switch finalAction { case .close(let writePromise): diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift index 5e105c0d8..61350dfd7 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift @@ -240,6 +240,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { self.request!.fail(error) self.request = nil self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context) + self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context) // No matter the error reason, we must always make sure the h2 stream is closed. Only // once the h2 stream is closed, it is released from the h2 multiplexer. The // HTTPRequestStateMachine may signal finalAction: .none in the error case (as this is @@ -252,6 +253,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler { self.request!.succeedRequest(finalParts) self.request = nil self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context) + self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context) self.runSuccessfulFinalAction(finalAction, context: context) case .failSendBodyPart(let error, let writePromise), .failSendStreamFinished(let error, let writePromise): From dd703139ec82600803079515188bf41c4becd0da Mon Sep 17 00:00:00 2001 From: Rick Newton-Rogers Date: Tue, 28 Jan 2025 16:44:53 +0000 Subject: [PATCH 3/3] review comments --- .../HTTP1/HTTP1ClientChannelHandler.swift | 2 +- .../HTTP1/HTTP1ConnectionStateMachine.swift | 4 +-- .../HTTP1ClientChannelHandlerTests.swift | 26 +++---------------- .../HTTP1ConnectionStateMachineTests.swift | 20 ++++++++++++++ 4 files changed, 25 insertions(+), 27 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index 2a9accfe5..8203f07af 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -281,7 +281,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { case .close: context.close(promise: nil) - case .wait, .noAction: + case .wait: break case .forwardResponseHead(let head, let pauseRequestBodyStream): diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift index 17dfcd7e5..2cde1df3f 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift @@ -83,8 +83,6 @@ struct HTTP1ConnectionStateMachine { case fireChannelActive case fireChannelInactive case fireChannelError(Error, closeConnection: Bool) - - case noAction } private var state: State @@ -361,7 +359,7 @@ struct HTTP1ConnectionStateMachine { mutating func idleWriteTimeoutTriggered() -> Action { guard case .inRequest(var requestStateMachine, let close) = self.state else { - return .noAction + return .wait } return self.avoidingStateMachineCoW { state -> Action in diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift index 6a2770b8b..df1a2926a 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift @@ -841,17 +841,6 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent]) } - class SlowHandler: ChannelOutboundHandler { - typealias OutboundIn = HTTPClientRequestPart - typealias OutboundOut = HTTPClientRequestPart - - func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise?) { - context.eventLoop.scheduleTask(in: .milliseconds(300)) { - promise?.succeed() - } - } - } - func testIdleWriteTimeoutOutsideOfRunningState() { let embedded = EmbeddedChannel() var maybeTestUtils: HTTP1TestTools? @@ -870,8 +859,8 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { } request.body = .init(contentLength: nil, stream: streamCallback) - let delegate = NullResponseDelegate() - var maybeRequestBag: RequestBag? + let accumulator = ResponseAccumulator(request: request) + var maybeRequestBag: RequestBag? XCTAssertNoThrow( maybeRequestBag = try RequestBag( request: request, @@ -883,7 +872,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { idleReadTimeout: .milliseconds(10), idleWriteTimeout: .milliseconds(2) ), - delegate: delegate + delegate: accumulator ) ) guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") } @@ -908,15 +897,6 @@ class HTTP1ClientChannelHandlerTests: XCTestCase { } } -class NullResponseDelegate: HTTPClientResponseDelegate { - typealias Response = Void - - func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws { - () - } - -} - class TestBackpressureWriter { let eventLoop: EventLoop diff --git a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift index 18831d32f..1c6e9659f 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift @@ -101,6 +101,26 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { XCTAssertEqual(state.read(), .read) } + func testWriteTimeoutAfterErrorDoesntCrash() { + var state = HTTP1ConnectionStateMachine() + XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive) + + 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, sendEnd: true)) + XCTAssertEqual( + state.headSent(), + .notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true) + ) + + struct MyError: Error, Equatable {} + XCTAssertEqual(state.errorHappened(MyError()), .failRequest(MyError(), .close(nil))) + + // Primarily we care that we don't crash here + XCTAssertEqual(state.idleWriteTimeoutTriggered(), .wait) + } + func testAConnectionCloseHeaderInTheRequestLeadsToConnectionCloseAfterRequest() { var state = HTTP1ConnectionStateMachine() XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)