Replace covert pipe with self-pipe SIGCHLD handler

[ntrrgc]

For background, see #2444 (comment).

In short, when running subprocesses that share the terminal, ninja intentionally leaves a pipe open before exec() so that it can use EOF from that pipe to detect when the subprocess has exited.

That mechanism is problematic: If the subprocess ends up spawning background processes (e.g. sccache), those would also inherit the pipe by default. In that case, ninja may not detect process termination until all background processes have quitted.

This patch makes it so that, instead of propagating the pipe file descriptor to the subprocess without its knowledge, ninja keeps both ends of the pipe to itself, and uses a SIGCHLD handler to close the write end of the pipe when the subprocess has truly exited.

During testing I found Subprocess::Finish() lacked EINTR retrying, which made ninja crash prematurely. This patch also fixes that.

Fixes #2444

[digit-google]

Avoid complicated logic in the signal handler. Instead write the child pid to a self-pipe that can be waited on in DoWork() (ensure the pipe descriptors are not leaked to spawned commands too). This avoids leaking the file descriptor to a global table, which makes reasoning about lifecycle difficult (e.g. there are code paths where this descriptor will never be closed properly in your code).

Using a linked list or any kind of map to find the fd from the pid is probably not needed. Just scan the array of Subprocess instances linearly, since command termination is not in the critical performance path (even when 1000+ of commands are launched in parallel).

[ntrrgc]

Just scan the array of Subprocess instances linearly

I considered this originally, but I don't know how many processes could end up in that table if someone really runs a lot of jobs share terminal, or if that is ever an use case.

Instead write the child pid to a self-pipe that can be waited on in DoWork()

I was trying to avoid modifications on DoWork(). I considered writing to pipes, but that has the additional risk that a write can potentially deadlock if the pipe buffer is full.
However, if we can rely on ppoll/pselect() returning EINTR after the first SIGCHLD signal handler execution, we could instead use a simple int field to communicate between the two, similar to how it's done for SIGINT:

Before ppoll(), set the "terminated PID" field to -1. Call ppoll(), if you get EINTR, check whether that field got a value other than -1. If it did, that's a process that is done. At that point we wouldn't even need the pipes, although they may still be useful to keep things orthogonal between the console and non-console use cases.

[digit-google]

I considered this originally, but I don't know how many processes could end up in that table if someone really runs a lot of jobs share terminal, or if that is ever an use case.

Ah, it turns out that Ninja, in its current design, only allows to run a single "console" sub-process at a time (this is implemented elsewhere and is not visible in this file). I think this can be leveraged to avoid using a self-pipe entirely.

I was trying to avoid modifications on DoWork(). I considered writing to pipes, but that has the additional risk that a write can potentially deadlock if the pipe buffer is full.

Technically, this is extremely unlikely. In this code, the signal handler can only run during the pselect() / ppoll() call. It would require thousands of processes to all terminate during that exact syscall to block the pipe buffer (which are very large these days, see https://github.com/afborchert/pipebuf?tab=readme-ov-file#some-results for some not-so-recent practical results).

But we can avoid pipes nonetheless.

However, if we can rely on ppoll/pselect() returning EINTR after the first SIGCHLD signal handler execution, we could instead use a simple int field to communicate between the two, similar to how it's done for SIGINT:

There is no actual guarantee that the system call would return after only a single SIGCHLD signal was handled.

On the other hand, because there is only one console subprocess, it should be possible to write its pid value to a global variable that the signal handler compares to. In case of success, it would set an atomic flag to true that can be trivially checked in DoWork(). More specifically:

• Add SubprocessSet::console_subproc_ as a Subprocess* pointer to the current console process if any.
  Ensure that starting a new subprocess updates the pointer if needed (and assert that only one can exist).

• Add two global sig_atomic_t values. One s_console_pid, will contain the pid of the console subprocess after it is started, or a special value (e.g. -1) to indicate there is no console process currently (which would be written in Subprocess::Finish). The second s_console_exited will be used as a boolean flag.

• The SIGCHLD handler simply compares the signal's pid to the value of s_console_pid. If they match, it sets s_console_exited to 1.

• In DoWork(), set s_console_exited to 0 before calling pselect() or ppoll(), and look at its value after the call.

Wdyt?

[ntrrgc]

There is no actual guarantee that the system call would return after only a single SIGCHLD signal was handled.

Sad. Do you have a source on this by any chance?

[digit-google]

Ah, I am trying to wrap my head around signals again, this is all so subtle, some details coming back:

SIGCHLD is a standard signals, not a real one, which means that it is not queued. When several processes terminate outside of the pselect() call, they are collapsed into a single signal handler call during the syscall (probably passing the pid of the last process). See
https://stackoverflow.com/questions/48750382/can-not-receive-all-the-sigchld

In other words, you we can only treat SIGCHLD as a boolean flag that says "some child has stopped", then have to use waitpid(..., WNOHANG) to scan the state of all processes of interest. Luckily for Ninja, that would be looking at the state of the single console process.

[ntrrgc]

Wow, that makes si_pid in SIGCHLD near-useless. If that is true, sigaction(2) should be updated to point it out.

[ntrrgc]

Just realized I also forgot to remove this comment:

    // In the console case, output_pipe is still inherited by the child and
    // closed when the subprocess finishes, which then notifies ninja.

[ntrrgc]

What is the point of HandlePendingInterruption()?

interrupted_ = 0;
int ret = ppoll(&fds.front(), nfds, NULL, &old_mask_);
if (ret == -1) {
  if (errno != EINTR) {
    perror("ninja: ppoll");
    return false;
  }
  return IsInterrupted();
}

HandlePendingInterruption();
if (IsInterrupted())
  return true;

If there is a SIGINT pending by the time HandlePendingInterruption() is called, wouldn't ppoll() handle it and return EINTR on the next DoWork() cycle?