testSubprocessDoesNotInheritVeryHighFileDescriptors and testLineSequence are extremely slow

[jakepetroules]

Following #79, testSubprocessDoesNotInheritVeryHighFileDescriptors and testLineSequence appear to be extremely slow on amazonlinux2 and focal (which notably contain older glibc missing modern features related to process descriptors and close_range). These CI jobs are taking multiple hours to complete.

[jakepetroules]

Note: this might reproduce only in containers by default.

Amazon Linux 2 container:

$ ulimit -Sn
2147483584

Real Debian VM:

$ ulimit -Sn
1024

Bumping up the limit and disabling use of close_range (simulating older glibc and/or kernel) demonstrates the problem.

I wonder if we should change the technique of closing file descriptors. It's not feasible to issue potentially billions of syscalls to close() in absence of a dedicated close_range syscall. Instead of returning the highest open FD from _highest_possibly_open_fd_dir_linux, could we instead pass a closure to this function and have it issue a close only for the fds that actually exist?

Not sure about non-Linux platforms which also lack a close_range/closefrom. posix_getdents is not async-signal-safe...

[iCharlesHu]

Bumping up the limit and disabling use of close_range (simulating older glibc and/or kernel) demonstrates the problem.

I don't think _highest_possibly_open_fd should always return the limit, right? It first tries to read /dev/fd, which should give us a more realistic number of how many open fds there are. Do you know if that doesn't work on amazonlinux2 and focal?

[jakepetroules]

The testSubprocessDoesNotInheritVeryHighFileDescriptors test will create a file descriptor with a ridiculously high number of 1 billion or so, since ulimits are so high in containers by default. _highest_possibly_open_fd is going to return that 1 billion file descriptor as the highest open one from reading /dev/fd. Then it tries to close every fd from 3 to a billion, which takes hours, even though there's only a dozen-ish file descriptors open. Thus I think this has to be done in terms of callbacks.

[weissi]

Bug is here:

swift-subprocess/Sources/_SubprocessCShims/process_shims.c

Line 696 in 680621b

for (int fd = STDERR_FILENO + 1; fd <= _highest_possibly_open_fd(); fd++) {

Correct code: https://github.com/swiftlang/swift-sdk-generator/blob/e29733e9c23aaae1414b2836434def63c8cc7028/Sources/CProcessSpawnSync/spawner.c#L171-L172

As written, the highest_possibly_open_fd() function gets run on every loop iteration. But really, we need to calculate the highest possibly open fd just once and then loop from 3 to that number. That number won't change so we don't have to reevaluate it every time.

[iCharlesHu]

@weissi can you elaborate on why do we need to calculate _highest_possibly_open_fd_dir_linux? Can we simply iterate through /dev/fd like we already do in _highest_possibly_open_fd_dir_linux and simply close the ones listed?

[weissi]

@weissi can you elaborate on why do we need to calculate _highest_possibly_open_fd_dir_linux? Can we simply iterate through /dev/fd like we already do in _highest_possibly_open_fd_dir_linux and simply close the ones listed?

That is what _highest_possibly_open_fd_dir_linux is doing (it's iterating through /dev/fd or /proc/self/fd). The key here is to do this once, you're doing this once per file descriptor. So if you have a million fds, your current code iterates /dev or /proc 1M times instead of just once

[iCharlesHu]

Resolved with: #171