Skip to content

Add test for HTTP2 request with large header #659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jan 26, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ extension HTTP2ClientRequestHandlerTests {
("testIdleReadTimeout", testIdleReadTimeout),
("testIdleReadTimeoutIsCanceledIfRequestIsCanceled", testIdleReadTimeoutIsCanceledIfRequestIsCanceled),
("testWriteHTTPHeadFails", testWriteHTTPHeadFails),
("testChannelBecomesNonWritableDuringHeaderWrite", testChannelBecomesNonWritableDuringHeaderWrite),
]
}
}
29 changes: 29 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
}
25 changes: 18 additions & 7 deletions Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ extension HTTPClientTests {
("testMassiveDownload", testMassiveDownload),
("testShutdownWithFutures", testShutdownWithFutures),
("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1),
("testMassiveHeaderHTTP2", testMassiveHeaderHTTP2),
]
}
}
29 changes: 29 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}