Process.WaitForExit() gets slower the larger your open file limit (ulimit -n) is on Linux

[akoeplinger]

Steps to Reproduce

1. Compile this: csc test.cs:

public static class Program {
    public static int Main () {
        for (int i = 0; i < 100; i++) {
            var p = System.Diagnostics.Process.Start ("echo", "hello");
            if (!p.WaitForExit (10000)) return 1;
        }
        return 0;
    }
}

2. $ (ulimit -n 1000; time mono test.exe)

...
real	0m0.402s
user	0m0.028s
sys	0m0.184s

3. $ (ulimit -n 100000; time mono test.exe)

...
real	0m1.277s
user	0m0.380s
sys	0m0.592s

4. $ (ulimit -n 1000000; time mono test.exe)

...
real	0m8.758s
user	0m3.424s
sys	0m4.424s

Current Behavior

It gets slower the higher your ulimit -n is.

Note: you may need to change your system settings (/etc/security/limits.conf) to allow higher limits.

Expected Behavior

Not getting slower.

On which platforms did you notice this

[ ] macOS
[ X ] Linux, Ubuntu 14.04/16.04
[ ] Windows

Version Used: master, 5.8.088 and 4.2.1 (so either a very old bug or something outside our control)

This was the root cause behind an issue (#6537) we had on Jenkins because the Azure Linux VM builders had ulimit -n set to 1048576.

[migueldeicaza]

This might be caused by the need of the child process to close the file descriptors on fork, to prevent the child from accessing the parent's descriptors.

What we could do is set close-on-exec flag on any file descriptor we open, to avoid paying this price at fork/exec time.

[lewurm]

relevant code that Miguel mentioned:

mono/mono/metadata/w32process-unix.c

Lines 1987 to 1991 in 5839d7b

	/* Close all file descriptors */
	for (i = eg_getdtablesize() - 1; i > 2; i--)
	close (i);

[lewurm]

I should point out that the strace log mentioned by @akoeplinger in #6537 (comment) explicitly filtered out syscalls to close, because it spammed the log a lot, so yeah, that might be it 😅

[luhenry]

I would definitely prefer the close-on-exec flag over what we have today. Let me work on a PR for that.

[luhenry]

The problem I am running into is open and other syscalls which open file descriptors are used all over the code base. So we would need to either sprinkle fcntl (fd, F_SETFD, O_CLOEXEC) all around, or come up with a centralized way of opening file descriptors.

[lewurm]

how about adding g_open to our eglib?

[luhenry]

@lewurm that would work. The slight issue is eglib uses the loop-before-exec approach (see https://github.com/mono/mono/blob/master/mono/eglib/gspawn.c#L294) so the models kind of clashes, but I don't think it has that much importance.

[jaykrell]

This area is messed up but better on Windows -- the flag/default for handle inheritance is reversed. So you tend not to inherit many builds. However if you do mark a handle for inheritance, it has two other problems, actually likely also on Posix. The problem is two part. First, imagine a multi threaded program, that runs child processes, an sometimes wants some inherited handles. A real example is Visual Studio running git and wanting to redirect stdout/stderr. Prior to Vista, any handle marked inheritable, would be inherited by all child processes recieving any inheritable handles. You couldn't direct specific handles to specific processes. Vista extended the CreateProcess API to address this, but inadequately. While you can direct specific handles to specific processes, they still have to be marked inheritable, and they will still be inherited through any CreateProcess using the old API. The fix is obvious in retrospect. The extended CreateProcess API should not require the handles to be marked inheritable. It is implied by passing them in the new parameters. This does cause real problems. An awkward workaround that worked in my code, but did not work with Visual Studio + git is that you can also specify the parent, from which to inherit. So you create a dummy parent, duplicate your handles into it, inheritable, and then use it as the parent. For Visual Studio + git they modified git to workaround, i.e. to take files on the command line to redirected into, instead of using stdout/stderr. The problem is the kernel APIs assume either usermode is single threaded, or that within a single process, there is some sort of oversight over all the CreateProcess calls, like they can all share a lock. But large programs don't work that way.

[migueldeicaza]

On Linux, we can use O_CLOEXEC as an option to open(2), without having to call fcntl afterwards:

https://www.freebsd.org/cgi/man.cgi?query=open&manpath=SuSE+Linux/i386+11.3

It was designed to avoid a race condition in multi-threaded programs that do open/fcntl, this email from 2015 on the Gnome lists does not sound good:

https://mail.gnome.org/archives/gtk-devel-list/2015-March/msg00038.html

It seems that it might not be worth fixing this issue. Almost every program on a system with those ulimits will run into this problem, closing all the descriptors is a common Unix idiom, and you can see these closes() on almost every application that does fork/exec. Mono is not alone on this.

[jaykrell]

I understand the open+fcntl problem, but what is wrong with open(cloexec?) And, how about using posix_spawn? It looks promising on Macosx at least, you can list specifically what fd to inherit -- like Windows tried but messed up.

[migueldeicaza]

open(cloexec) has the problem mentioned on my previous post - and additionally, this assumes we get every handle to work properly, any mistakes here would be a problem for our users.

As for posix_spawn, this is my take: on MacOS it is documented as a system call, which might be useful, except MacOS tends to not have a lot of open file descriptors, so introducing this change wont likely have any performance difference (OSX defaults to 256 descriptors).

As for Linux, it is implemented as a userlevel method, not a system call, so the user-level capability will just loop in the same way ours does.

Does not seem worth the confusion

[jaykrell]

I understand the need to get them all. I thought someone proposed we ban open and instead mono_open or such, that gets it right. Perhaps that is still fragile -- how to ban open? pragma poison in glib.h maybe?

If you do get them all, I still don't understand the problem, with open(cloexec).

As well though, maybe we should just use clone? And don't give it the flag to inherit any files? And then if we need to inherit some, dup them right after? Do we even need to inherit any?
And then I realize this is Linux-specific, but that helps?

OSX really defaults to 256 file limit, wow, wierd to have 8 bit limits on 64bit systems.
My system suggests a limit around 2.5k -- desktop, not iOS (I assume fork is rarer on mobile??)
Still only 11 bits.

$ cat 1.c && gcc 1.c && ./a.out
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main()
{
int count = 0;
while (open("/dev/null", O_RDONLY) >= 0)
count ++;
printf("%d %d %s\n", count, errno, strerror(errno));
return 0;
}2557 24 Too many open files

There might still be a problem of other code in the process calling open or fork, but mono-only scenarios would work? So I have to look at what is fork vs. clone, easy to replace or not.

[jaykrell]

Hm I don't see if there is a way to dup an fd into a process other than via fork. :( Win32 DuplicateHandle -- lots of cross process operations are in Win32 (VirtualAllocEx, CreateRemoteThread, VirtualQueryEx, ReadProcessMemory), but seemingly almost none in Posix. :(

[jaykrell]

Rust appears to have taken the cloexec in every open approach.
rust-lang/rust#24034

[migueldeicaza]

It is not just open, it is on every file descriptor opened, both via managed and unmanaged code.

The rust approach has both the problem mentioned above by the GNome guys, and one that is even worse and unaccounted for.

Even if we were to cleanup all of Mono, this would not cover any file descriptor opened by libraries (for example, Gtk+, Cocoa, and so on). So we should removed some scenarios, not all of them, so correct execution would still require us to close everything on fork.

[jaykrell]

Ah, everyone has to pass the flag for every open/dup/etc.
What a mess. Everyone constantly reimplementing similar stuff and seemingly none do what people think, and can't work without cooperation on locks.
joyent/libuv#1483
https://bugs.chromium.org/p/chromium/issues/detail?id=179923#c35
etc.

Is clone w/o any file inheritance too draconion, need stdin/out/err? I think so.
Besides being Linux specific.
The right way is to specify the inherited file handles explicitly even for in/out/error and for the default to be noinherit.

It looks like Apple almost got it right but not quite.
Windows (Vista) also came close but missed.
Alas.

[NattyNarwhal]

This is reproducible on AIX, and if you have your ulimits to unlimited, it starts at -2 (that's seemingly what the function in eglib gets out of rlimit - it says #define RLIM64_INFINITY (0x7fffffffffffffffLL)) and has to work its way down closing FDs until it hits stderr. In worst case scenarios, I find it takes ~30 minutes for it to reach exec. This is what was bunging up the CI server. (The workaround is dropping your fd limit to something relatively small.)

Stupid idea for a workaround when procfs is available and CLOEXEC variants are off the table: enumerate /proc/<pid>/fd and close anything that isn't stdio, instead of just going through everything?

[jaykrell]

Looks like CoreCLR addresses this mostly, fcntl(FD_SETFD, FD_CLOEXEC) after every open.

https://github.com/dotnet/coreclr/blame/32f0f9721afb584b4a14d69135bea7ddc129f755/src/pal/src/file/file.cpp#L707

[NattyNarwhal]

Fixed by 0d29dfb.