Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace covert pipe with self-pipe SIGCHLD handler #2550

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+119 −7
Open

Replace covert pipe with self-pipe SIGCHLD handler #2550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 118 additions & 7 deletions src/subprocess-posix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ extern char** environ;

using namespace std;

static void CloseFileWhenPidExits(pid_t pid, int fd);

Subprocess::Subprocess(bool use_console) : fd_(-1), pid_(-1),
use_console_(use_console) {
}
Expand Down Expand Up @@ -103,12 +105,13 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
err = posix_spawn_file_actions_adddup2(&action, output_pipe[1], 2);
if (err != 0)
Fatal("posix_spawn_file_actions_adddup2: %s", strerror(err));
err = posix_spawn_file_actions_addclose(&action, output_pipe[1]);
if (err != 0)
Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
// In the console case, output_pipe is still inherited by the child and
// closed when the subprocess finishes, which then notifies ninja.
}
err = posix_spawn_file_actions_addclose(&action, output_pipe[1]);
if (err != 0)
Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));

#ifdef POSIX_SPAWN_USEVFORK
flags |= POSIX_SPAWN_USEVFORK;
#endif
Expand All @@ -123,14 +126,22 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
if (err != 0)
Fatal("posix_spawn: %s", strerror(err));

if (use_console_) {
// Shared console case: We keep the write-side of the pipe just so that we
// can close it on SIGCHLD reception.
// Calling this is safe as SIGCHLD is blocked except during ppoll/pselect().
CloseFileWhenPidExits(pid_, output_pipe[1]);
} else {
// Not shared console: Only the subprocess needs the write-end of the pipe.
close(output_pipe[1]);
}

err = posix_spawnattr_destroy(&attr);
if (err != 0)
Fatal("posix_spawnattr_destroy: %s", strerror(err));
err = posix_spawn_file_actions_destroy(&action);
if (err != 0)
Fatal("posix_spawn_file_actions_destroy: %s", strerror(err));

close(output_pipe[1]);
return true;
}

Expand All @@ -150,8 +161,10 @@ void Subprocess::OnPipeReady() {
ExitStatus Subprocess::Finish() {
assert(pid_ != -1);
int status;
if (waitpid(pid_, &status, 0) < 0)
Fatal("waitpid(%d): %s", pid_, strerror(errno));
while (waitpid(pid_, &status, 0) < 0) {
if (errno != EINTR)
Fatal("waitpid(%d): %s", pid_, strerror(errno));
}
pid_ = -1;

#ifdef _AIX
Expand Down Expand Up @@ -205,12 +218,102 @@ void SubprocessSet::HandlePendingInterruption() {
interrupted_ = SIGHUP;
}

// SIGCHLD handling:

struct PidFdEntry;
typedef unsigned int IxEntry;
// PidFdList is used to store the PID and file descriptor pairs of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

@ntrrgc ntrrgc Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// subprocesses being run by ninja that share a terminal.
// On SIGCHLD the PID of the signal is matched with one of the entries and the
// associated file descriptor closed. Then the entry is freed for reuse.
// Normally we would use something like std::unordered_map<>, but that involves
// memory allocations which are not reentrant.
struct PidFdList {
PidFdEntry* entries; // Entry with index 0 is reserved as NULL and not used.
size_t capacity; // Number of PidFdEntry allocated for pid_fds.
IxEntry ix_head_used;
IxEntry ix_head_free;
};

// A PidFdEntry may be used or free.
struct PidFdEntry {
pid_t pid; // -1 if this is a free entry
int fd;
// ix_next of a used entry points to another used entry, ix_next of free entry
// points to another free entry.
IxEntry ix_next;
};

static PidFdList PID_FD_LIST = { nullptr, 0, 0, 1 };

static void initializePidFdEntries(PidFdEntry* entries, IxEntry ix_start, size_t capacity) {
for (unsigned int i = ix_start; i < capacity; i++) {
entries[i].pid = -1;
entries[i].fd = -1;
entries[i].ix_next = i + 1;
}
entries[capacity - 1].ix_next = 0; // end of the free list.
}

// Must only be called with SIGCHLD blocked.
static void CloseFileWhenPidExits(pid_t pid, int fd) {
PidFdList* list = &PID_FD_LIST;
if (!list->entries) {
// First use, allocate the list.
list->capacity = 16;
list->entries = new PidFdEntry[list->capacity];
initializePidFdEntries(list->entries, 1, list->capacity);
} else if (!list->ix_head_free) {
// Expand the list to get more free entries.
size_t old_capacity = list->capacity;
list->capacity *= 2;
list->entries = static_cast<PidFdEntry*>(realloc(list->entries,
sizeof(PidFdEntry) * list->capacity));
initializePidFdEntries(list->entries, old_capacity, list->capacity);
list->ix_head_free = old_capacity;
}
// Replace the head free entry and make it the new used head.
IxEntry ix_entry = list->ix_head_free;
PidFdEntry* entry = &list->entries[ix_entry];
IxEntry ix_next_free = entry->ix_next;
entry->pid = pid;
entry->fd = fd;
entry->ix_next = list->ix_head_used;
list->ix_head_used = ix_entry;
list->ix_head_free = ix_next_free;
}

static void SigChldHandler(int signo, siginfo_t* info, void* context) {
int pid = info->si_pid;

PidFdList* list = &PID_FD_LIST;
if (!list->entries)
return;
for (IxEntry* ix = &list->ix_head_used; *ix; ix = &list->entries[*ix].ix_next) {
PidFdEntry* entry = &list->entries[*ix];
if (entry->pid == pid) {
// Found a match. Close the pipe and remove the entry.
close(entry->fd);
IxEntry ix_next_used = entry->ix_next;
entry->fd = -1;
entry->pid = -1;
entry->ix_next = list->ix_head_free;
list->ix_head_free = *ix;
*ix = ix_next_used;
break;
}
}
}

SubprocessSet::SubprocessSet() {
// Block all these signals.
// Their handlers will only be enabled during ppoll/pselect().
sigset_t set;
sigemptyset(&set);
sigaddset(&set, SIGINT);
sigaddset(&set, SIGTERM);
sigaddset(&set, SIGHUP);
sigaddset(&set, SIGCHLD);
if (sigprocmask(SIG_BLOCK, &set, &old_mask_) < 0)
Fatal("sigprocmask: %s", strerror(errno));

Expand All @@ -223,6 +326,12 @@ SubprocessSet::SubprocessSet() {
Fatal("sigaction: %s", strerror(errno));
if (sigaction(SIGHUP, &act, &old_hup_act_) < 0)
Fatal("sigaction: %s", strerror(errno));

memset(&act, 0, sizeof(act));
act.sa_flags = SA_SIGINFO | SA_NOCLDSTOP;
act.sa_sigaction = SigChldHandler;
if (sigaction(SIGCHLD, &act, &old_chld_act_) < 0)
Fatal("sigaction: %s", strerror(errno));
ntrrgc marked this conversation as resolved.
Show resolved Hide resolved
}

SubprocessSet::~SubprocessSet() {
Expand All @@ -234,6 +343,8 @@ SubprocessSet::~SubprocessSet() {
Fatal("sigaction: %s", strerror(errno));
if (sigaction(SIGHUP, &old_hup_act_, 0) < 0)
Fatal("sigaction: %s", strerror(errno));
if (sigaction(SIGCHLD, &old_chld_act_, 0) < 0)
Fatal("sigaction: %s", strerror(errno));
if (sigprocmask(SIG_SETMASK, &old_mask_, 0) < 0)
Fatal("sigprocmask: %s", strerror(errno));
}
Expand Down
1 change: 1 addition & 0 deletions src/subprocess.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ struct SubprocessSet {
struct sigaction old_int_act_;
struct sigaction old_term_act_;
struct sigaction old_hup_act_;
struct sigaction old_chld_act_;
sigset_t old_mask_;
#endif
};
Expand Down
Loading