From 12ced80212e92a7c82165b84f4f75e66d7f5299f Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 17 Aug 2022 11:53:26 +0200 Subject: [PATCH 1/7] Tollerate more data after request body is cancelled --- .../AsyncAwait/Transaction+StateMachine.swift | 22 ++++---- .../AsyncAwaitEndToEndTests.swift | 53 +++++++++++++++++++ 2 files changed, 66 insertions(+), 9 deletions(-) diff --git a/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift b/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift index 53bc8c8a6..8bbdcd5b9 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift @@ -429,19 +429,22 @@ extension Transaction { case .executing(_, _, .waitingForResponseHead): preconditionFailure("If we receive a response body, we must have received a head before") - case .executing(let context, let requestState, .buffering(let streamID, var currentBuffer, next: let next)): - guard case .askExecutorForMore = next else { - preconditionFailure("If we have received an error or eof before, why did we get another body part? Next: \(next)") - } - + case .executing(_, _, .buffering(_, _, next: .endOfFile)): + preconditionFailure("If we have received an eof before, why did we get another body part?") + + case .executing(_, _, .buffering(_, _, next: .error)): + // we might still get pending buffers if the user has canceled the request + return .none + + case .executing(let context, let requestState, .buffering(let streamID, var currentBuffer, next: .askExecutorForMore)): if currentBuffer.isEmpty { currentBuffer = buffer } else { currentBuffer.append(contentsOf: buffer) } - self.state = .executing(context, requestState, .buffering(streamID, currentBuffer, next: next)) + self.state = .executing(context, requestState, .buffering(streamID, currentBuffer, next: .askExecutorForMore)) return .none - + case .executing(let executor, let requestState, .waitingForResponseIterator(var currentBuffer, next: let next)): guard case .askExecutorForMore = next else { preconditionFailure("If we have received an error or eof before, why did we get another body part? Next: \(next)") @@ -690,10 +693,11 @@ extension Transaction { case .finished: // the request failed or was cancelled before, we can ignore all events return .none - + case .executing(_, _, .buffering(_, _, next: .error)): + // we might still get pending buffers if the user has canceled the request + return .none case .executing(_, _, .waitingForResponseIterator(_, next: .error)), .executing(_, _, .waitingForResponseIterator(_, next: .endOfFile)), - .executing(_, _, .buffering(_, _, next: .error)), .executing(_, _, .buffering(_, _, next: .endOfFile)), .executing(_, _, .finished(_, _)): preconditionFailure("Already received an eof or error before. Must not receive further events. Invalid state: \(self.state)") diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 68457d4ff..ce7ce6d52 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -578,6 +578,53 @@ final class AsyncAwaitEndToEndTests: XCTestCase { } #endif } + + func test() { + #if compiler(>=5.5.2) && canImport(_Concurrency) + guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return } + XCTAsyncTest { + let client = HTTPClient(eventLoopGroupProvider: .createNew) + + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + var request = HTTPClientRequest(url: "http://api.weather.gov/zones/forecast/AKZ026/forecast") + request.headers.add(name: "User-Agent", value: "Swift HTTPClient") + let response = try await client.execute(request, deadline: .now() + .seconds(10)) + + let maxBodySize = 1024 * 1024 + + await XCTAssertThrowsError(try await response.body.collect(upTo: maxBodySize)) { error in + XCTAssert(error is NIOTooManyBytesError) + } + + // we need to wait a bit to receive more packets before we shutdown the HTTPClient + try await Task.sleep(nanoseconds: UInt64(TimeAmount.milliseconds(100).nanoseconds)) + } + #endif + } + + /// Regression test for https://github.com/swift-server/async-http-client/issues/612 + func testCancelingBodyDoesNotCrash() { + #if compiler(>=5.5.2) && canImport(_Concurrency) + guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return } + XCTAsyncTest { + let client = makeDefaultHTTPClient() + defer { XCTAssertNoThrow(try client.syncShutdown()) } + let bin = HTTPBin(.http2(compress: true)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + let request = HTTPClientRequest(url: "https://127.0.0.1:\(bin.port)/mega-chunked") + let response = try await client.execute(request, deadline: .now() + .seconds(1)) + + await XCTAssertThrowsError(try await response.body.collect(upTo: 100)) { error in + XCTAssert(error is NIOTooManyBytesError) + } + + // we need to wait a bit to receive more packets before we shutdown the HTTPClient + try await Task.sleep(nanoseconds: UInt64(TimeAmount.milliseconds(100).nanoseconds)) + } + #endif + } } #if compiler(>=5.5.2) && canImport(_Concurrency) @@ -590,3 +637,9 @@ extension AsyncSequence where Element == ByteBuffer { } } #endif + +extension NIOTooManyBytesError: Equatable { + public static func == (lhs: NIOTooManyBytesError, rhs: NIOTooManyBytesError) -> Bool { + true + } +} From 00aea735930e59ac5981aa719199c831e9063cb3 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 17 Aug 2022 12:21:57 +0200 Subject: [PATCH 2/7] wait is not needed if we shutdown the server first --- Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift | 3 --- 1 file changed, 3 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index ce7ce6d52..859195269 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -619,9 +619,6 @@ final class AsyncAwaitEndToEndTests: XCTestCase { await XCTAssertThrowsError(try await response.body.collect(upTo: 100)) { error in XCTAssert(error is NIOTooManyBytesError) } - - // we need to wait a bit to receive more packets before we shutdown the HTTPClient - try await Task.sleep(nanoseconds: UInt64(TimeAmount.milliseconds(100).nanoseconds)) } #endif } From 3891cd310f5236f0de5736d204d5e266c326064f Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 17 Aug 2022 12:30:02 +0200 Subject: [PATCH 3/7] Remove test that depends on external resources --- .../AsyncAwaitEndToEndTests.swift | 24 ------------------- 1 file changed, 24 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 859195269..026f53b7f 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -579,30 +579,6 @@ final class AsyncAwaitEndToEndTests: XCTestCase { #endif } - func test() { - #if compiler(>=5.5.2) && canImport(_Concurrency) - guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return } - XCTAsyncTest { - let client = HTTPClient(eventLoopGroupProvider: .createNew) - - defer { XCTAssertNoThrow(try client.syncShutdown()) } - - var request = HTTPClientRequest(url: "http://api.weather.gov/zones/forecast/AKZ026/forecast") - request.headers.add(name: "User-Agent", value: "Swift HTTPClient") - let response = try await client.execute(request, deadline: .now() + .seconds(10)) - - let maxBodySize = 1024 * 1024 - - await XCTAssertThrowsError(try await response.body.collect(upTo: maxBodySize)) { error in - XCTAssert(error is NIOTooManyBytesError) - } - - // we need to wait a bit to receive more packets before we shutdown the HTTPClient - try await Task.sleep(nanoseconds: UInt64(TimeAmount.milliseconds(100).nanoseconds)) - } - #endif - } - /// Regression test for https://github.com/swift-server/async-http-client/issues/612 func testCancelingBodyDoesNotCrash() { #if compiler(>=5.5.2) && canImport(_Concurrency) From 4be7d923eae5309bf9b90004a52ceb1baf0ba3ce Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 17 Aug 2022 13:48:12 +0200 Subject: [PATCH 4/7] Remove unused conformance to Equatable --- Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 026f53b7f..c40f63e56 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -610,9 +610,3 @@ extension AsyncSequence where Element == ByteBuffer { } } #endif - -extension NIOTooManyBytesError: Equatable { - public static func == (lhs: NIOTooManyBytesError, rhs: NIOTooManyBytesError) -> Bool { - true - } -} From 7719076b327ad692cc38c3fb69315daf4f6b7b20 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 17 Aug 2022 13:53:43 +0200 Subject: [PATCH 5/7] SwiftFormat --- .../AsyncAwait/Transaction+StateMachine.swift | 6 +++--- Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift b/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift index 8bbdcd5b9..38709c9a7 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift @@ -431,11 +431,11 @@ extension Transaction { case .executing(_, _, .buffering(_, _, next: .endOfFile)): preconditionFailure("If we have received an eof before, why did we get another body part?") - + case .executing(_, _, .buffering(_, _, next: .error)): // we might still get pending buffers if the user has canceled the request return .none - + case .executing(let context, let requestState, .buffering(let streamID, var currentBuffer, next: .askExecutorForMore)): if currentBuffer.isEmpty { currentBuffer = buffer @@ -444,7 +444,7 @@ extension Transaction { } self.state = .executing(context, requestState, .buffering(streamID, currentBuffer, next: .askExecutorForMore)) return .none - + case .executing(let executor, let requestState, .waitingForResponseIterator(var currentBuffer, next: let next)): guard case .askExecutorForMore = next else { preconditionFailure("If we have received an error or eof before, why did we get another body part? Next: \(next)") diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index c40f63e56..7fe607aba 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -578,7 +578,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase { } #endif } - + /// Regression test for https://github.com/swift-server/async-http-client/issues/612 func testCancelingBodyDoesNotCrash() { #if compiler(>=5.5.2) && canImport(_Concurrency) @@ -588,10 +588,10 @@ final class AsyncAwaitEndToEndTests: XCTestCase { defer { XCTAssertNoThrow(try client.syncShutdown()) } let bin = HTTPBin(.http2(compress: true)) defer { XCTAssertNoThrow(try bin.shutdown()) } - + let request = HTTPClientRequest(url: "https://127.0.0.1:\(bin.port)/mega-chunked") let response = try await client.execute(request, deadline: .now() + .seconds(1)) - + await XCTAssertThrowsError(try await response.body.collect(upTo: 100)) { error in XCTAssert(error is NIOTooManyBytesError) } From dfcba559f9f20b05e6c25ff64bda3c0ab5187626 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 17 Aug 2022 13:54:40 +0200 Subject: [PATCH 6/7] run generate_linux_tests.rb --- Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift index 397071e08..3fb55779f 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift @@ -43,6 +43,7 @@ extension AsyncAwaitEndToEndTests { ("testInvalidURL", testInvalidURL), ("testRedirectChangesHostHeader", testRedirectChangesHostHeader), ("testShutdown", testShutdown), + ("testCancelingBodyDoesNotCrash", testCancelingBodyDoesNotCrash), ] } } From c8450aea16e54160ab387d83a2f9144661644954 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 17 Aug 2022 14:16:19 +0200 Subject: [PATCH 7/7] Increase timeout for CI --- Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 7fe607aba..91482d08c 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -590,7 +590,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase { defer { XCTAssertNoThrow(try bin.shutdown()) } let request = HTTPClientRequest(url: "https://127.0.0.1:\(bin.port)/mega-chunked") - let response = try await client.execute(request, deadline: .now() + .seconds(1)) + let response = try await client.execute(request, deadline: .now() + .seconds(10)) await XCTAssertThrowsError(try await response.body.collect(upTo: 100)) { error in XCTAssert(error is NIOTooManyBytesError)