From 59bfb96afb2a45feb79ed8a3f51d97ad4da879cb Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 26 Jan 2023 14:26:58 +0100 Subject: [PATCH] Add test for HTTP2 request with large header (#659) 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. --- ...TTP2ClientRequestHandlerTests+XCTest.swift | 1 + .../HTTP2ClientRequestHandlerTests.swift | 29 +++++++++++++++++++ .../HTTPClientTestUtils.swift | 25 +++++++++++----- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 29 +++++++++++++++++++ 5 files changed, 78 insertions(+), 7 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift index 8fa21983..221a6321 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 e67529ad..5dfce3f9 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 e50dab3b..ca24cba1 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: 10), + 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+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index f9ddb1c8..6e84f9d2 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 8e5d5fbf..54d854bf 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3378,4 +3378,33 @@ 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 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) + 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])) + + XCTAssertNoThrow(try client.execute(request: request).wait()) + } }