apple / swift-nio-http2

HTTP/2 support for SwiftNIO
https://swiftpackageindex.com/apple/swift-nio-http2/main/documentation/niohttp2
Apache License 2.0
462 stars 82 forks source link

The new HPACK header test for RemoveAll() fails on Android #394

Closed finagolfin closed 1 year ago

finagolfin commented 1 year ago

393 broke my Android CI, which runs that test in an Android x86_64 emulator, with these 4 test failures:

/home/runner/work/swift-android-sdk/swift-android-sdk/snh/Tests/NIOHPACKTests/HPACKHeadersTests.swift:194: error: HPACKHeadersTests.testRemoveAll : XCTAssertEqual failed: ("7") is not equal to ("6") - 
/home/runner/work/swift-android-sdk/swift-android-sdk/snh/Tests/NIOHPACKTests/HPACKHeadersTests.swift:197: error: HPACKHeadersTests.testRemoveAll : XCTAssertEqual failed: ("7") is not equal to ("6") - 
/home/runner/work/swift-android-sdk/swift-android-sdk/snh/Tests/NIOHPACKTests/HPACKHeadersTests.swift:200: error: HPACKHeadersTests.testRemoveAll : XCTAssertEqual failed: ("7") is not equal to ("6") - 
/home/runner/work/swift-android-sdk/swift-android-sdk/snh/Tests/NIOHPACKTests/HPACKHeadersTests.swift:204: error: HPACKHeadersTests.testRemoveAll : XCTAssertEqual failed: ("7") is not equal to ("6") - 

All four are related to checking the capacity of the header array and not getting the expected size.

I can reproduce on my Android AArch64 phone, so I stuck these print()s in the initializer and found the offending line when the test is run:

diff --git a/Sources/NIOHPACK/HPACKHeader.swift b/Sources/NIOHPACK/HPACKHeader.swift
index 866025c..bd26df9 100644
--- a/Sources/NIOHPACK/HPACKHeader.swift
+++ b/Sources/NIOHPACK/HPACKHeader.swift
@@ -85,7 +85,11 @@ public struct HPACKHeaders: ExpressibleByDictionaryLiteral, Sendable {
     ///     - headers: An initial set of headers to use to populate the header block.
     @inlinable
     public init(_ headers: [(String, String)] = []) {
+        print(headers.count)
+        print(headers.capacity)
         self.headers = headers.map { HPACKHeader(name: $0.0, value: $0.1) }
+        print(self.headers.count)
+        print(self.headers.capacity)
     }

     /// Construct a `HPACKHeaders` structure.

That prints the following for this test:

Test Case 'HPACKHeadersTests.testRemoveAll' started at 2023-05-18 21:31:43.616
6
6
6
7

I have no idea if this map should be allocating a larger capacity or if this even matters: shouldn't the array capacity just be an implementation detail?

Opening this to see what you all think of this strange failure, let me know.

Lukasa commented 1 year ago

Yeah I think depending on the specific numbers of the capacity is probably wrong. I'd happily take a patch that measures the capacity before removeAll and then confirms it's unchanged, rather than relying on hardcoded numbers. Are you open to making that patch?

finagolfin commented 1 year ago

Submitted your suggestion as #395.