diff --git a/Sources/NIOHPACK/HPACKHeader.swift b/Sources/NIOHPACK/HPACKHeader.swift index febb5299..eb599d67 100644 --- a/Sources/NIOHPACK/HPACKHeader.swift +++ b/Sources/NIOHPACK/HPACKHeader.swift @@ -18,6 +18,22 @@ import NIOHTTP1 /// Very similar to `NIOHTTP1.HTTPHeaders`, but with extra data for storing indexing /// information. public struct HPACKHeaders: ExpressibleByDictionaryLiteral, NIOSendable { + /// The maximum size of the canonical connection header value array to use when removing + /// connection headers during `HTTPHeaders` normalisation. When using an array the removal + /// is O(H·C) where H is the length of headers to noramlize and C is the length of the + /// connection header value array. + /// + /// Beyond this limit we construct a set of the connection header values to reduce the + /// complexity to O(H). + /// + /// We use an array for small connection header lists as it is cheaper (values don't need to be + /// hashed and constructing a set incurs an additional allocation). The value of 32 was picked + /// as the crossover point where using an array became more expensive than using a set when + /// running the `hpackheaders_normalize_httpheaders_removing_10k_conn_headers` performance test + /// with varying input sizes. + @usableFromInline + static let connectionHeaderValueArraySizeLimit = 32 + @usableFromInline internal var headers: [HPACKHeader] @@ -33,10 +49,21 @@ public struct HPACKHeaders: ExpressibleByDictionaryLiteral, NIOSendable { self.headers = httpHeaders.map { HPACKHeader(name: $0.name.lowercased(), value: $0.value) } let connectionHeaderValue = httpHeaders[canonicalForm: "connection"] - - self.headers.removeAll { header in - connectionHeaderValue.contains(header.name[...]) || - HPACKHeaders.illegalHeaders.contains(header.name) + + // Above a limit we use a set rather than scanning the connection header value array. + // See `Self.connectionHeaderValueArraySizeLimit`. + if connectionHeaderValue.count > Self.connectionHeaderValueArraySizeLimit { + var headersToRemove = Set(connectionHeaderValue) + // Since we have a set we can just merge in the illegal headers. + headersToRemove.formUnion(Self.illegalHeaders.lazy.map { $0[...] }) + self.headers.removeAll { header in + headersToRemove.contains(header.name[...]) + } + } else { + self.headers.removeAll { header in + connectionHeaderValue.contains(header.name[...]) || + HPACKHeaders.illegalHeaders.contains(header.name) + } } } else { self.headers = httpHeaders.map { HPACKHeader(name: $0.name, value: $0.value) } diff --git a/Tests/NIOHPACKTests/HPACKHeadersTests+XCTest.swift b/Tests/NIOHPACKTests/HPACKHeadersTests+XCTest.swift index 0f45f4ad..7b7d35b3 100644 --- a/Tests/NIOHPACKTests/HPACKHeadersTests+XCTest.swift +++ b/Tests/NIOHPACKTests/HPACKHeadersTests+XCTest.swift @@ -28,6 +28,8 @@ extension HPACKHeadersTests { static var allTests : [(String, (HPACKHeadersTests) -> () throws -> Void)] { return [ ("testHPACKHeadersAreHashable", testHPACKHeadersAreHashable), + ("testNormalizationOfHTTPHeaders", testNormalizationOfHTTPHeaders), + ("testNormalizationOfHTTPHeadersWithManyConnectionHeaderValues", testNormalizationOfHTTPHeadersWithManyConnectionHeaderValues), ] } } diff --git a/Tests/NIOHPACKTests/HPACKHeadersTests.swift b/Tests/NIOHPACKTests/HPACKHeadersTests.swift index bee09893..bbe323c7 100644 --- a/Tests/NIOHPACKTests/HPACKHeadersTests.swift +++ b/Tests/NIOHPACKTests/HPACKHeadersTests.swift @@ -15,6 +15,7 @@ import XCTest import NIOCore import NIOHPACK +import NIOHTTP1 final class HPACKHeadersTests: XCTestCase { func testHPACKHeadersAreHashable() throws { @@ -71,4 +72,51 @@ final class HPACKHeadersTests: XCTestCase { Set([firstHeaders, thirdHeaders, fourthHeaders]) ) } + + func testNormalizationOfHTTPHeaders() { + let httpHeaders: HTTPHeaders = [ + "connection": "keepalive", + "connection": "remove-me, and-me", + "connection": "also-me-please", + "remove-me": "", + "and-me": "", + "also-me-please": "", + "but-not-me": "", + "keep-alive": "remove-me", + "proxy-connection": "me too", + "transfer-encoding": "me three" + ] + + let normalized = HPACKHeaders(httpHeaders: httpHeaders, normalizeHTTPHeaders: true) + let expected: HPACKHeaders = [ + "but-not-me": "" + ] + + XCTAssertEqual(normalized, expected) + } + + func testNormalizationOfHTTPHeadersWithManyConnectionHeaderValues() { + var httpHeaders: HTTPHeaders = [ + "keep-alive": "remove-me", + "proxy-connection": "me too", + "transfer-encoding": "me three", + "but-not-me": "", + ] + + // Add a bunch of connection headers to remove. We add a large number because the + // implementation of the normalizing init branches on the number of connection header + // values. + for i in 0 ..< 512 { + let toRemove = "value-\(i)" + httpHeaders.add(name: "connection", value: toRemove) + httpHeaders.add(name: toRemove, value: "") + } + + let normalized = HPACKHeaders(httpHeaders: httpHeaders, normalizeHTTPHeaders: true) + let expected: HPACKHeaders = [ + "but-not-me": "" + ] + + XCTAssertEqual(normalized, expected) + } }