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