From 2588689ba27d880ddddb6ff2dabfe63f4ca0c5c2 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Thu, 16 Jun 2022 14:22:39 +0100 Subject: [PATCH 1/2] Correctly reset our state after .sendEnd Motivation In some cases, the last thing that happens in a request-response pair is that we send our HTTP/1.1 .end. This can happen when the peer has sent an early response, before we have finished uploading our body. When it does, we need to be diligent about cleaning up our connection state. Unfortunately, there was an edge in the HTTP1ConnectionStateMachine that processed .succeedRequest but that did not transition the state into either .idle or .closing. That was an error, and needed to be fixed. Modifications Transition to .idle when we're returning .succeedRequest(.sendRequestEnd). Result Fewer crashes --- .../HTTP1/HTTP1ConnectionStateMachine.swift | 1 + .../HTTPClientTestUtils.swift | 6 ++- .../HTTPClientTests.swift | 54 +++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift index ecff7afc7..f0aff762c 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift @@ -412,6 +412,7 @@ extension HTTP1ConnectionStateMachine.State { self = .closing newFinalAction = .close case .sendRequestEnd(let writePromise): + self = .idle newFinalAction = .sendRequestEnd(writePromise) case .none: self = .idle diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 63a1cf540..f2cc7b1d8 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -1267,7 +1267,11 @@ final class HTTP200DelayedHandler: ChannelInboundHandler { let request = self.unwrapInboundIn(data) switch request { case .head: - break + // Once we have received one response, all further requests are responded to immediately. + if self.pendingBodyParts == nil { + context.writeAndFlush(self.wrapOutboundOut(.head(.init(version: .http1_1, status: .ok))), promise: nil) + context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil) + } case .body: if let pendingBodyParts = self.pendingBodyParts { if pendingBodyParts > 0 { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 726809415..e5d935fb9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3021,6 +3021,60 @@ class HTTPClientTests: XCTestCase { XCTAssertNil(try delegate.next().wait()) } + // This test is identical to the one above, except that we send another request immediately after. This is a regression + // test for https://github.com/swift-server/async-http-client/issues/595. + func testBiDirectionalStreamingEarly200DoesntPreventUsFromSendingMoreRequests() { + let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTP200DelayedHandler(bodyPartsBeforeResponse: 1) } + defer { XCTAssertNoThrow(try httpBin.shutdown()) } + + let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 2) + defer { XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully()) } + let writeEL = eventLoopGroup.next() + + let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoopGroup)) + defer { XCTAssertNoThrow(try httpClient.syncShutdown()) } + + let body: HTTPClient.Body = .stream { writer in + let finalPromise = writeEL.makePromise(of: Void.self) + + func writeLoop(_ writer: HTTPClient.Body.StreamWriter, index: Int) { + // always invoke from the wrong el to test thread safety + writeEL.preconditionInEventLoop() + + if index >= 30 { + return finalPromise.succeed(()) + } + + let sent = ByteBuffer(integer: index) + writer.write(.byteBuffer(sent)).whenComplete { result in + switch result { + case .success: + writeEL.execute { + writeLoop(writer, index: index + 1) + } + + case .failure(let error): + finalPromise.fail(error) + } + } + } + + writeEL.execute { + writeLoop(writer, index: 0) + } + + return finalPromise.futureResult + } + + let request = try! HTTPClient.Request(url: "http://localhost:\(httpBin.port)", body: body) + let future = httpClient.execute(request: request) + XCTAssertNoThrow(try future.wait()) + + // Try another request + let future2 = httpClient.execute(request: request) + XCTAssertNoThrow(try future2.wait()) + } + func testSynchronousHandshakeErrorReporting() throws { // This only affects cases where we use NIOSSL. guard !isTestingNIOTS() else { return } From 8801683222bbdf3ffa07c4668d285039b1c4adef Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 17 Jun 2022 09:18:43 +0100 Subject: [PATCH 2/2] Fix up soundness --- 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 d6fe77e47..a709cf2d6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -130,6 +130,7 @@ extension HTTPClientTests { ("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer), ("testBiDirectionalStreaming", testBiDirectionalStreaming), ("testBiDirectionalStreamingEarly200", testBiDirectionalStreamingEarly200), + ("testBiDirectionalStreamingEarly200DoesntPreventUsFromSendingMoreRequests", testBiDirectionalStreamingEarly200DoesntPreventUsFromSendingMoreRequests), ("testSynchronousHandshakeErrorReporting", testSynchronousHandshakeErrorReporting), ("testFileDownloadChunked", testFileDownloadChunked), ("testCloseWhileBackpressureIsExertedIsFine", testCloseWhileBackpressureIsExertedIsFine),