apple / swift-nio-ssl

TLS Support for SwiftNIO, based on BoringSSL.
https://swiftpackageindex.com/apple/swift-nio-ssl/main/documentation/niossl
Apache License 2.0
388 stars 139 forks source link

Broken test: NIOSSLIntegrationTest.testGiantWrite #388

Closed finagolfin closed 2 years ago

finagolfin commented 2 years ago

Ever since that test was added in #384/#385, I'm seeing crashes or hangs (on my Android CI) when running these tests, which looks like this on a small linux x86_64 VPS with 1 GB RAM and 2.4 GB swap:

> SWIFTCI_USE_LOCAL_DEPS=1 ../swift-5.6.2-RELEASE-ubuntu20.04/usr/bin/swift test --filter testGiantWrite
Building for debugging...
Build complete! (0.23s)
Test Suite 'Selected tests' started at 2022-07-31 03:34:52.701
Test Suite 'NIOSSLIntegrationTest' started at 2022-07-31 03:34:52.713
Test Case 'NIOSSLIntegrationTest.testGiantWrite' started at 2022-07-31 03:34:53.255
error: Exited with signal code 9

Even on this repo's CI with a presumably beefy server, I see it takes 13.65 seconds just to run this single test. Disabling this test gets the tests running and passing again. On Android 12 in the Termux app, running this test crashes the app.

I think something needs to be done.

Lukasa commented 2 years ago

This test deliberately tests an extremely large write to hit a specific edge case that is only triggered when processing writes that are larger than 2 GB in size. The effect of the test is to allocate and dirty 4GB of memory. On machines with limited RAM this is necessarily going to cause issues.

The unfortunate reality is that there's no feasible way to make this test take less time or to test the functionality without exercising the code path. There is no hook at which we can intercept the code, and adding one for explicitly this purpose doesn't achieve a useful testing function. All we can do is skip the test on machines where the test is likely to be excessively costly. The 10s runtime of the test in CI is something I'm willing to tolerate, but for your small VPS we could plausibly attempt to skip the test instead.

finagolfin commented 2 years ago

Confirmed that the problem is gone on linux x86_64 and Android AArch64, thanks.