Skip to content

Commit 29db9a7

Browse files
authored
Merge pull request #2587 from ntrrgc/2025-04-14-iterator
Replace covert pipe with SIGCHLD handler + iterator bugfix
2 parents f2aa42a + 8393f25 commit 29db9a7

File tree

2 files changed

+148
-29
lines changed

2 files changed

+148
-29
lines changed

src/subprocess-posix.cc

Lines changed: 118 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ extern char** environ;
3737

3838
using namespace std;
3939

40+
namespace {
41+
ExitStatus ParseExitStatus(int status);
42+
}
43+
4044
Subprocess::Subprocess(bool use_console) : fd_(-1), pid_(-1),
4145
use_console_(use_console) {
4246
}
@@ -50,26 +54,34 @@ Subprocess::~Subprocess() {
5054
}
5155

5256
bool Subprocess::Start(SubprocessSet* set, const string& command) {
53-
int output_pipe[2];
54-
if (pipe(output_pipe) < 0)
55-
Fatal("pipe: %s", strerror(errno));
56-
fd_ = output_pipe[0];
57+
int subproc_stdout_fd = -1;
58+
if (use_console_) {
59+
fd_ = -1;
60+
} else {
61+
int output_pipe[2];
62+
if (pipe(output_pipe) < 0)
63+
Fatal("pipe: %s", strerror(errno));
64+
fd_ = output_pipe[0];
65+
subproc_stdout_fd = output_pipe[1];
5766
#if !defined(USE_PPOLL)
58-
// If available, we use ppoll in DoWork(); otherwise we use pselect
59-
// and so must avoid overly-large FDs.
60-
if (fd_ >= static_cast<int>(FD_SETSIZE))
61-
Fatal("pipe: %s", strerror(EMFILE));
67+
// If available, we use ppoll in DoWork(); otherwise we use pselect
68+
// and so must avoid overly-large FDs.
69+
if (fd_ >= static_cast<int>(FD_SETSIZE))
70+
Fatal("pipe: %s", strerror(EMFILE));
6271
#endif // !USE_PPOLL
63-
SetCloseOnExec(fd_);
72+
SetCloseOnExec(fd_);
73+
}
6474

6575
posix_spawn_file_actions_t action;
6676
int err = posix_spawn_file_actions_init(&action);
6777
if (err != 0)
6878
Fatal("posix_spawn_file_actions_init: %s", strerror(err));
6979

70-
err = posix_spawn_file_actions_addclose(&action, output_pipe[0]);
71-
if (err != 0)
72-
Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
80+
if (!use_console_) {
81+
err = posix_spawn_file_actions_addclose(&action, fd_);
82+
if (err != 0)
83+
Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
84+
}
7385

7486
posix_spawnattr_t attr;
7587
err = posix_spawnattr_init(&attr);
@@ -98,18 +110,17 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
98110
Fatal("posix_spawn_file_actions_addopen: %s", strerror(err));
99111
}
100112

101-
err = posix_spawn_file_actions_adddup2(&action, output_pipe[1], 1);
113+
err = posix_spawn_file_actions_adddup2(&action, subproc_stdout_fd, 1);
102114
if (err != 0)
103115
Fatal("posix_spawn_file_actions_adddup2: %s", strerror(err));
104-
err = posix_spawn_file_actions_adddup2(&action, output_pipe[1], 2);
116+
err = posix_spawn_file_actions_adddup2(&action, subproc_stdout_fd, 2);
105117
if (err != 0)
106118
Fatal("posix_spawn_file_actions_adddup2: %s", strerror(err));
107-
err = posix_spawn_file_actions_addclose(&action, output_pipe[1]);
119+
err = posix_spawn_file_actions_addclose(&action, subproc_stdout_fd);
108120
if (err != 0)
109121
Fatal("posix_spawn_file_actions_addclose: %s", strerror(err));
110-
// In the console case, output_pipe is still inherited by the child and
111-
// closed when the subprocess finishes, which then notifies ninja.
112122
}
123+
113124
#ifdef POSIX_SPAWN_USEVFORK
114125
flags |= POSIX_SPAWN_USEVFORK;
115126
#endif
@@ -131,7 +142,8 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
131142
if (err != 0)
132143
Fatal("posix_spawn_file_actions_destroy: %s", strerror(err));
133144

134-
close(output_pipe[1]);
145+
if (!use_console_)
146+
close(subproc_stdout_fd);
135147
return true;
136148
}
137149

@@ -148,13 +160,32 @@ void Subprocess::OnPipeReady() {
148160
}
149161
}
150162

151-
ExitStatus Subprocess::Finish() {
163+
164+
bool Subprocess::TryFinish(int waitpid_options) {
152165
assert(pid_ != -1);
153-
int status;
154-
if (waitpid(pid_, &status, 0) < 0)
155-
Fatal("waitpid(%d): %s", pid_, strerror(errno));
166+
int status, ret;
167+
while ((ret = waitpid(pid_, &status, waitpid_options)) < 0) {
168+
if (errno != EINTR)
169+
Fatal("waitpid(%d): %s", pid_, strerror(errno));
170+
}
171+
if (ret == 0)
172+
return false; // Subprocess is alive (WNOHANG-only).
156173
pid_ = -1;
174+
exit_status_ = ParseExitStatus(status);
175+
return true; // Subprocess has terminated.
176+
}
177+
178+
ExitStatus Subprocess::Finish() {
179+
if (pid_ != -1) {
180+
TryFinish(0);
181+
assert(pid_ == -1);
182+
}
183+
return exit_status_;
184+
}
157185

186+
namespace {
187+
188+
ExitStatus ParseExitStatus(int status) {
158189
#ifdef _AIX
159190
if (WIFEXITED(status) && WEXITSTATUS(status) & 0x80) {
160191
// Map the shell's exit code used for signal failure (128 + signal) to the
@@ -178,20 +209,31 @@ ExitStatus Subprocess::Finish() {
178209
return static_cast<ExitStatus>(status + 128);
179210
}
180211

212+
} // anonymous namespace
213+
181214
bool Subprocess::Done() const {
182-
return fd_ == -1;
215+
// Console subprocesses share console with ninja, and we consider them done
216+
// when they exit.
217+
// For other processes, we consider them done when we have consumed all their
218+
// output and closed their associated pipe.
219+
return (use_console_ && pid_ == -1) || (!use_console_ && fd_ == -1);
183220
}
184221

185222
const string& Subprocess::GetOutput() const {
186223
return buf_;
187224
}
188225

189-
int SubprocessSet::interrupted_;
226+
volatile sig_atomic_t SubprocessSet::interrupted_;
227+
volatile sig_atomic_t SubprocessSet::s_sigchld_received;
190228

191229
void SubprocessSet::SetInterruptedFlag(int signum) {
192230
interrupted_ = signum;
193231
}
194232

233+
void SubprocessSet::SigChldHandler(int signo, siginfo_t* info, void* context) {
234+
s_sigchld_received = 1;
235+
}
236+
195237
void SubprocessSet::HandlePendingInterruption() {
196238
sigset_t pending;
197239
sigemptyset(&pending);
@@ -208,11 +250,14 @@ void SubprocessSet::HandlePendingInterruption() {
208250
}
209251

210252
SubprocessSet::SubprocessSet() {
253+
// Block all these signals.
254+
// Their handlers will only be enabled during ppoll/pselect().
211255
sigset_t set;
212256
sigemptyset(&set);
213257
sigaddset(&set, SIGINT);
214258
sigaddset(&set, SIGTERM);
215259
sigaddset(&set, SIGHUP);
260+
sigaddset(&set, SIGCHLD);
216261
if (sigprocmask(SIG_BLOCK, &set, &old_mask_) < 0)
217262
Fatal("sigprocmask: %s", strerror(errno));
218263

@@ -225,6 +270,27 @@ SubprocessSet::SubprocessSet() {
225270
Fatal("sigaction: %s", strerror(errno));
226271
if (sigaction(SIGHUP, &act, &old_hup_act_) < 0)
227272
Fatal("sigaction: %s", strerror(errno));
273+
274+
memset(&act, 0, sizeof(act));
275+
act.sa_flags = SA_SIGINFO | SA_NOCLDSTOP;
276+
act.sa_sigaction = SigChldHandler;
277+
if (sigaction(SIGCHLD, &act, &old_chld_act_) < 0)
278+
Fatal("sigaction: %s", strerror(errno));
279+
}
280+
281+
// Reaps console processes that have exited and moves them from the running set
282+
// to the finished set.
283+
void SubprocessSet::CheckConsoleProcessTerminated() {
284+
if (!s_sigchld_received)
285+
return;
286+
for (auto i = running_.begin(); i != running_.end(); ) {
287+
if ((*i)->use_console_ && (*i)->TryFinish(WNOHANG)) {
288+
finished_.push(*i);
289+
i = running_.erase(i);
290+
} else {
291+
++i;
292+
}
293+
}
228294
}
229295

230296
SubprocessSet::~SubprocessSet() {
@@ -236,6 +302,8 @@ SubprocessSet::~SubprocessSet() {
236302
Fatal("sigaction: %s", strerror(errno));
237303
if (sigaction(SIGHUP, &old_hup_act_, 0) < 0)
238304
Fatal("sigaction: %s", strerror(errno));
305+
if (sigaction(SIGCHLD, &old_chld_act_, 0) < 0)
306+
Fatal("sigaction: %s", strerror(errno));
239307
if (sigprocmask(SIG_SETMASK, &old_mask_, 0) < 0)
240308
Fatal("sigprocmask: %s", strerror(errno));
241309
}
@@ -264,9 +332,21 @@ bool SubprocessSet::DoWork() {
264332
fds.push_back(pfd);
265333
++nfds;
266334
}
335+
if (nfds == 0) {
336+
// Add a dummy entry to prevent using an empty pollfd vector.
337+
// ppoll() allows to do this by setting fd < 0.
338+
pollfd pfd = { -1, 0, 0 };
339+
fds.push_back(pfd);
340+
++nfds;
341+
}
267342

268343
interrupted_ = 0;
344+
s_sigchld_received = 0;
269345
int ret = ppoll(&fds.front(), nfds, NULL, &old_mask_);
346+
// Note: This can remove console processes from the running set, but that is
347+
// not a problem for the pollfd set, as console processes are not part of the
348+
// pollfd set (they don't have a fd).
349+
CheckConsoleProcessTerminated();
270350
if (ret == -1) {
271351
if (errno != EINTR) {
272352
perror("ninja: ppoll");
@@ -275,16 +355,23 @@ bool SubprocessSet::DoWork() {
275355
return IsInterrupted();
276356
}
277357

358+
// ppoll/pselect prioritizes file descriptor events over a signal delivery.
359+
// However, if the user is trying to quit ninja, we should react as fast as
360+
// possible.
278361
HandlePendingInterruption();
279362
if (IsInterrupted())
280363
return true;
281364

365+
// Iterate through both the pollfd set and the running set.
366+
// All valid fds in the running set are in the pollfd, in the same order.
282367
nfds_t cur_nfd = 0;
283368
for (vector<Subprocess*>::iterator i = running_.begin();
284369
i != running_.end(); ) {
285370
int fd = (*i)->fd_;
286-
if (fd < 0)
371+
if (fd < 0) {
372+
++i;
287373
continue;
374+
}
288375
assert(fd == fds[cur_nfd].fd);
289376
if (fds[cur_nfd++].revents) {
290377
(*i)->OnPipeReady();
@@ -317,7 +404,9 @@ bool SubprocessSet::DoWork() {
317404
}
318405

319406
interrupted_ = 0;
320-
int ret = pselect(nfds, &set, 0, 0, 0, &old_mask_);
407+
s_sigchld_received = 0;
408+
int ret = pselect(nfds, (nfds > 0 ? &set : nullptr), 0, 0, 0, &old_mask_);
409+
CheckConsoleProcessTerminated();
321410
if (ret == -1) {
322411
if (errno != EINTR) {
323412
perror("ninja: pselect");
@@ -326,6 +415,9 @@ bool SubprocessSet::DoWork() {
326415
return IsInterrupted();
327416
}
328417

418+
// ppoll/pselect prioritizes file descriptor events over a signal delivery.
419+
// However, if the user is trying to quit ninja, we should react as fast as
420+
// possible.
329421
HandlePendingInterruption();
330422
if (IsInterrupted())
331423
return true;

src/subprocess.h

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,27 @@ struct Subprocess {
6868
char overlapped_buf_[4 << 10];
6969
bool is_reading_;
7070
#else
71+
/// The file descriptor that will be used in ppoll/pselect() for this process,
72+
/// if any. Otherwise -1.
73+
/// In non-console mode, this is the read-side of a pipe that was created
74+
/// specifically for this subprocess. The write-side of the pipe is given to
75+
/// the subprocess as combined stdout and stderr.
76+
/// In console mode no pipe is created: fd_ is -1, and process termination is
77+
/// detected using the SIGCHLD signal and waitpid(WNOHANG).
7178
int fd_;
79+
/// PID of the subprocess. Set to -1 when the subprocess is reaped.
7280
pid_t pid_;
81+
/// In POSIX platforms it is necessary to use waitpid(WNOHANG) to know whether
82+
/// a certain subprocess has finished. This is done for terminal subprocesses.
83+
/// However, this also causes the subprocess to be reaped before Finish() is
84+
/// called, so we need to store the ExitStatus so that a later Finish()
85+
/// invocation can return it.
86+
ExitStatus exit_status_;
87+
88+
/// Call waitpid() on the subprocess with the provided options and update the
89+
/// pid_ and exit_status_ fields.
90+
/// Return a boolean indicating whether the subprocess has indeed terminated.
91+
bool TryFinish(int waitpid_options);
7392
#endif
7493
bool use_console_;
7594

@@ -96,16 +115,24 @@ struct SubprocessSet {
96115
static HANDLE ioport_;
97116
#else
98117
static void SetInterruptedFlag(int signum);
99-
static void HandlePendingInterruption();
118+
static void SigChldHandler(int signo, siginfo_t* info, void* context);
119+
100120
/// Store the signal number that causes the interruption.
101121
/// 0 if not interruption.
102-
static int interrupted_;
103-
122+
static volatile sig_atomic_t interrupted_;
123+
/// Whether ninja should quit. Set on SIGINT, SIGTERM or SIGHUP reception.
104124
static bool IsInterrupted() { return interrupted_ != 0; }
125+
static void HandlePendingInterruption();
126+
127+
/// Initialized to 0 before ppoll/pselect().
128+
/// Filled to 1 by SIGCHLD handler when a child process terminates.
129+
static volatile sig_atomic_t s_sigchld_received;
130+
void CheckConsoleProcessTerminated();
105131

106132
struct sigaction old_int_act_;
107133
struct sigaction old_term_act_;
108134
struct sigaction old_hup_act_;
135+
struct sigaction old_chld_act_;
109136
sigset_t old_mask_;
110137
#endif
111138
};

0 commit comments

Comments
 (0)