From 42bb531e497f94517fcb9d94abdfcd993eb849ec Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Fri, 25 Sep 2026 15:16:22 +0200 Subject: [PATCH] Fix HTTP1Connection reuse crash after request outlives response ### Motivation If the response finishes before the request body stream, we use the write promise of the request's `.end` to tell the connection pool that the connection can be reused. This write promise can be completed synchronously inside `writeAndFlush`. In that case the pool may schedule the next request onto the connection before `writeAndFlush` returns. Afterwards we transitioned the idle read/write timeout state machines, which by then belonged to the *new* request. With an idle read timeout configured, this crashes with: HTTP1ClientChannelHandler.swift: Fatal error: Invalid state. Waiting for response data must start after request head was sent This is a regression introduced in 1.31.0 (#879). ### Changes - Update the idle read and write timeout state machines *before* writing and flushing the request's `.end`, so they are always applied to the request that is actually ending. - Add a Swift Testing reproducer using `EmbeddedChannel` that schedules a new request from `onConnectionIdle` while the `.end` write completes. - Allow `MockHTTPExecutableRequest` to be created with custom `RequestOptions`. ### Result A request whose response finishes before its body stream no longer crashes the connection when the pool immediately reuses that connection. --- .../HTTP1/HTTP1ClientChannelHandler.swift | 9 +- .../HTTP1ClientChannelHandlerTesting.swift | 105 ++++++++++++++++++ .../Mocks/MockHTTPExecutableRequest.swift | 4 +- 3 files changed, 115 insertions(+), 3 deletions(-) create mode 100644 Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTesting.swift diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift index e59f0ab7e..c5345d875 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift @@ -289,8 +289,11 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { } } - context.writeAndFlush(self.wrapOutboundOut(.end(trailers)), promise: writePromise) - + // We must update the idle timeout state machines before writing the request end. The + // write promise may be completed synchronously within `writeAndFlush`. If the final action + // is `.informConnectionIsIdle`, the pool might then synchronously schedule a new request + // onto this connection. The idle timeout state machines would then belong to the new + // request and must not be transitioned on behalf of the old one. if let readTimeoutAction = self.idleReadTimeoutStateMachine?.requestEndSent() { self.runTimeoutAction(readTimeoutAction, context: context) } @@ -299,6 +302,8 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { self.runTimeoutAction(writeTimeoutAction, context: context) } + context.writeAndFlush(self.wrapOutboundOut(.end(trailers)), promise: writePromise) + 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 diff --git a/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTesting.swift b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTesting.swift new file mode 100644 index 000000000..d7ee16180 --- /dev/null +++ b/Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTesting.swift @@ -0,0 +1,105 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2026 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +import Logging +import NIOCore +import NIOEmbedded +import NIOHTTP1 +import Testing + +@testable import AsyncHTTPClient + +struct HTTP1ClientChannelHandlerTesting { + /// If the response end is received before the request body stream is finished, the connection is + /// marked idle from within the write promise of the request `.end`. On a real channel that write + /// promise may run synchronously inside `writeAndFlush`. We use that promise to signal to + /// the pool that the connection has become idle. Therefore the connection pool may schedule the + /// next request onto the connection synchronously from `onConnectionIdle`. The handler must not + /// apply the old request's idle timeout transitions to the new request. + @Test func newRequestScheduledFromOnConnectionIdleDuringRequestEndWrite() throws { + let eventLoop = EmbeddedEventLoop() + let handler = HTTP1ClientChannelHandler( + eventLoop: eventLoop, + backgroundLogger: Logger(label: "no-op", factory: SwiftLogNoOpLogHandler.init), + connectionIdLoggerMetadata: "test connection" + ) + let channel = EmbeddedChannel(handlers: [handler], loop: eventLoop) + try channel.connect(to: .init(ipAddress: "127.0.0.1", port: 80)).wait() + + let firstRequest = MockHTTPExecutableRequest( + head: .init(version: .http1_1, method: .POST, uri: "http://localhost/"), + framingMetadata: RequestFramingMetadata(connectionClose: false, body: .stream), + requestOptions: .forTests(idleReadTimeout: .milliseconds(200)), + raiseErrorIfUnimplementedMethodIsCalled: false + ) + let secondRequest = MockHTTPExecutableRequest( + head: .init(version: .http1_1, method: .GET, uri: "http://localhost/"), + framingMetadata: RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)), + requestOptions: .forTests(idleReadTimeout: .milliseconds(200)), + raiseErrorIfUnimplementedMethodIsCalled: false + ) + + // Mimic the connection pool, which synchronously executes the next queued request on the + // connection if it is informed about the idle connection on the connection's event loop. + var connectionIdleCount = 0 + handler.onConnectionIdle = { + connectionIdleCount += 1 + if connectionIdleCount == 1 { + channel.write(secondRequest, promise: nil) + } + } + + let executor = handler.requestExecutor + firstRequest.resumeRequestBodyStreamCallback = { + executor.writeRequestBodyPart(.byteBuffer(.init(string: "Hello")), request: firstRequest, promise: nil) + } + + channel.write(firstRequest, promise: nil) + #expect(try channel.readOutbound(as: HTTPClientRequestPart.self) == .head(firstRequest.requestHead)) + #expect( + try channel.readOutbound(as: HTTPClientRequestPart.self) == .body(.byteBuffer(.init(string: "Hello"))) + ) + + // Receive the full response, while the request body stream is still open. + try channel.writeInbound(HTTPClientResponsePart.head(.init(version: .http1_1, status: .ok))) + channel.read() + try channel.writeInbound(HTTPClientResponsePart.end(nil)) + + // Finishing the body stream writes `.end`. The EmbeddedChannel succeeds the write promise + // synchronously in `flush`, which marks the connection idle, which in turn writes the second + // request into the handler before `writeAndFlush` has returned. + executor.finishRequestBodyStream(trailers: nil, request: firstRequest, promise: nil) + + #expect(try channel.readOutbound(as: HTTPClientRequestPart.self) == .end(nil)) + #expect(try channel.readOutbound(as: HTTPClientRequestPart.self) == .head(secondRequest.requestHead)) + #expect(try channel.readOutbound(as: HTTPClientRequestPart.self) == .end(nil)) + #expect(connectionIdleCount == 1) + + #expect( + firstRequest.events.map(\.kind) == [ + .willExecuteRequest, .requestHeadSent, .resumeRequestBodyStream, + .receiveResponseHead, .receiveResponseEnd, .requestBodySent, + ] + ) + #expect(secondRequest.events.map(\.kind) == [.willExecuteRequest, .requestHeadSent]) + + // The second request's idle read timeout must still work as expected. + eventLoop.advanceTime(by: .milliseconds(250)) + #expect(secondRequest.events.map(\.kind) == [.willExecuteRequest, .requestHeadSent, .fail]) + if case .fail(let error) = secondRequest.events.last { + #expect(error as? HTTPClientError == .readTimeout) + } + #expect(!channel.isActive) + } +} diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift b/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift index 227bbeff3..2d4d6471f 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockHTTPExecutableRequest.swift @@ -63,7 +63,7 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { let logger: Logging.Logger = Logger(label: "request") let requestHead: NIOHTTP1.HTTPRequestHead let requestFramingMetadata: RequestFramingMetadata - let requestOptions: RequestOptions = .forTests() + let requestOptions: RequestOptions /// 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, @@ -146,12 +146,14 @@ final class MockHTTPExecutableRequest: HTTPExecutableRequest { init( head: NIOHTTP1.HTTPRequestHead = .init(version: .http1_1, method: .GET, uri: "http://localhost/"), framingMetadata: RequestFramingMetadata = .init(connectionClose: false, body: .fixedSize(0)), + requestOptions: RequestOptions = .forTests(), raiseErrorIfUnimplementedMethodIsCalled: Bool = true, file: StaticString = #file, line: UInt = #line ) { self.requestHead = head self.requestFramingMetadata = framingMetadata + self.requestOptions = requestOptions self.raiseErrorIfUnimplementedMethodIsCalled = raiseErrorIfUnimplementedMethodIsCalled self.file = file self.line = line