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.

Author: dnadoba

Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift

@@ -30,6 +30,7 @@ extension HTTP2ClientRequestHandlerTests {
             ("testIdleReadTimeout", testIdleReadTimeout),
             ("testIdleReadTimeoutIsCanceledIfRequestIsCanceled", testIdleReadTimeoutIsCanceledIfRequestIsCanceled),
             ("testWriteHTTPHeadFails", testWriteHTTPHeadFails),
+            ("testChannelBecomesNonWritableDuringHeaderWrite", testChannelBecomesNonWritableDuringHeaderWrite),
         ]
     }
 }

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])
+    }
 }

Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift

@@ -329,17 +329,32 @@ internal final class HTTPBin<RequestHandler: ChannelInboundHandler> 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<RequestHandler: ChannelInboundHandler> 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,

Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift

@@ -144,6 +144,7 @@ extension HTTPClientTests {
             ("testMassiveDownload", testMassiveDownload),
             ("testShutdownWithFutures", testShutdownWithFutures),
             ("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1),
+            ("testMassiveHeaderHTTP2", testMassiveHeaderHTTP2),
         ]
     }
 }

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())
+    }
 }