Skip to content

Commit 230fc0c

Browse files
committed
fix(platform): 未捕获的有界运行必须继承 stdio,而不是先缓冲后回放
自审发现的真回归。`run_exec_deadline` 是 `mcpp test` 非 JSON 模式跑测试二进制 的路径,原本继承调用方的 stdio —— 输出实时出现,且子进程的 stdout 是终端。 把它改成「捕获后在结束时一次性回放」有两个后果: 1. 长测试的输出全部憋到退出才出现,恰好抵消了 `mcpp test` 可观察性那一整 轮工作(「只有子进程输出、mcpp 一行没有」正是缓冲问题的指纹); 2. 子进程的 stdout 变成管道而非终端,gtest 之类会静默关掉彩色输出。 两侧启动器现在共用一条契约:`sink == nullptr` 表示「不捕获」,子进程直接继承 调用方的 stdio,但**仍然有界**。POSIX 侧不建管道也不设 dup2 file action; Windows 侧不设 STARTF_USESTDHANDLES,也不加 CREATE_NO_WINDOW(未捕获的运行 本来就是要给人看的)。`dispatch_bounded` 多一个 capture 形参把这个选择传下去。
1 parent 6ce62fd commit 230fc0c

3 files changed

Lines changed: 81 additions & 44 deletions

File tree

src/platform/process.cppm

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -582,11 +582,15 @@ struct BoundedOutcome {
582582
std::string output;
583583
};
584584

585+
// `capture == false` runs the child on the caller's stdio: live output, and a
586+
// real terminal for anything that checks. `run_exec_deadline` needs that; the
587+
// capturing variants need the pipe.
585588
BoundedOutcome dispatch_bounded(
586589
const std::vector<std::string>& argv,
587590
const std::vector<std::pair<std::string, std::string>>& extraEnv,
588591
std::string_view cwd,
589-
std::chrono::milliseconds deadline)
592+
std::chrono::milliseconds deadline,
593+
bool capture)
590594
{
591595
BoundedOutcome outcome;
592596

@@ -603,10 +607,14 @@ BoundedOutcome dispatch_bounded(
603607
const char* cwdArg = cwdStore.empty() ? nullptr : cwdStore.c_str();
604608
const auto ms = static_cast<long long>(deadline.count());
605609

606-
// One sink for both, appending into the outcome's own buffer.
607-
const auto sink = +[](void* ctx, const char* data, unsigned long len) {
608-
static_cast<std::string*>(ctx)->append(data, len);
609-
};
610+
// One sink for both, appending into the outcome's own buffer. Null when the
611+
// caller wants the child on its own stdio.
612+
using Sink = void (*)(void*, const char*, unsigned long);
613+
const Sink sink = capture
614+
? +[](void* ctx, const char* data, unsigned long len) {
615+
static_cast<std::string*>(ctx)->append(data, len);
616+
}
617+
: nullptr;
610618

611619
if constexpr (mcpp::platform::is_windows) {
612620
const auto cmd = windows_command_from_argv(argv);
@@ -639,12 +647,13 @@ int run_exec_deadline(const std::vector<std::string>& argv,
639647
if (deadline.count() <= 0) return run_exec(argv, extraEnv);
640648
if (argv.empty()) return 127;
641649

642-
auto r = dispatch_bounded(argv, extraEnv, {}, deadline);
650+
// capture=false: identical stdio behaviour to `run_exec` — the child writes
651+
// straight to our terminal as it goes. `mcpp test`'s non-JSON path runs
652+
// test binaries through here, and buffering their output until exit would
653+
// undo the observability work that path exists for (and would hide gtest's
654+
// colors by making its stdout a pipe).
655+
auto r = dispatch_bounded(argv, extraEnv, {}, deadline, /*capture=*/false);
643656
if (!r.supported) return run_exec(argv, extraEnv);
644-
// `run_exec` streams to the terminal; the bounded launchers capture. The
645-
// output is replayed here rather than dropped — a bounded run that must
646-
// ALSO stream live has no implementation and, today, no caller.
647-
if (!r.output.empty()) std::fputs(r.output.c_str(), stdout);
648657
if (timed_out) *timed_out = r.timed_out;
649658
return r.exit_code;
650659
}
@@ -661,7 +670,7 @@ RunResult capture_exec_deadline(
661670
RunResult result;
662671
if (argv.empty()) { result.exit_code = 127; return result; }
663672

664-
auto r = dispatch_bounded(argv, extraEnv, cwd, deadline);
673+
auto r = dispatch_bounded(argv, extraEnv, cwd, deadline, /*capture=*/true);
665674
// `supported == false` means the child COULD NOT BE SPAWNED — not that it
666675
// ran and failed. Reporting those the same way would hide a launcher
667676
// problem behind a child's exit code, so fall back to the untimed path and

src/platform/unix/bounded_process.cppm

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,12 @@ using OutputSink = void (*)(void* ctx, const char* data, unsigned long len);
7272
// `cwd` may be null. A non-positive `deadlineMs` is rejected with
7373
// supported=false: "no bound" belongs on the caller's untimed path.
7474
//
75-
// When `sink` is null the output is discarded but the child is still bounded —
76-
// that is the `run_exec_deadline` shape.
75+
// A NULL `sink` means "do not capture": the child INHERITS the caller's stdio
76+
// and is still bounded. That is not an optimization — it is the
77+
// `run_exec_deadline` contract. Routing an uncaptured run through a pipe would
78+
// (a) delay every line until the child exits, which is the opposite of what a
79+
// bounded `mcpp test` run is for, and (b) make the child's stdout a pipe
80+
// rather than a terminal, so gtest and friends silently drop their colors.
7781
DeadlineRun capture_with_deadline(const char* const* argvEntries,
7882
unsigned long argvCount,
7983
const char* const* envEntries,
@@ -149,8 +153,9 @@ DeadlineRun capture_with_deadline(const char* const* argvEntries,
149153
cargv.push_back(const_cast<char*>(argvEntries[i]));
150154
cargv.push_back(nullptr);
151155

152-
int fds[2];
153-
if (::pipe(fds) != 0) return out;
156+
const bool capture = (sink != nullptr);
157+
int fds[2] = {-1, -1};
158+
if (capture && ::pipe(fds) != 0) return out;
154159

155160
posix_spawn_file_actions_t fa;
156161
::posix_spawn_file_actions_init(&fa);
@@ -159,44 +164,50 @@ DeadlineRun capture_with_deadline(const char* const* argvEntries,
159164
// silently move where a build program's relative writes go.
160165
if (cwd && *cwd)
161166
::posix_spawn_file_actions_addchdir_np(&fa, cwd);
162-
::posix_spawn_file_actions_adddup2(&fa, fds[1], 1);
163-
::posix_spawn_file_actions_adddup2(&fa, fds[1], 2);
164-
::posix_spawn_file_actions_addclose(&fa, fds[0]);
165-
::posix_spawn_file_actions_addclose(&fa, fds[1]);
167+
if (capture) {
168+
::posix_spawn_file_actions_adddup2(&fa, fds[1], 1);
169+
::posix_spawn_file_actions_adddup2(&fa, fds[1], 2);
170+
::posix_spawn_file_actions_addclose(&fa, fds[0]);
171+
::posix_spawn_file_actions_addclose(&fa, fds[1]);
172+
}
173+
// else: no file actions for stdio at all — the child inherits ours, which
174+
// keeps its output live AND keeps it a terminal.
166175

167176
pid_t pid = 0;
168177
int sp = ::posix_spawnp(&pid, cargv[0], &fa, nullptr, cargv.data(), envp.data());
169178
::posix_spawn_file_actions_destroy(&fa);
170-
::close(fds[1]);
171-
if (sp != 0) { ::close(fds[0]); return out; }
179+
if (capture) ::close(fds[1]);
180+
if (sp != 0) { if (capture) ::close(fds[0]); return out; }
172181

173182
// Non-blocking reads so the deadline is still checked while the child is
174183
// quiet. A blocking read on a silent, hung child is exactly the hang this
175184
// whole mechanism exists to stop.
176-
::fcntl(fds[0], F_SETFL, ::fcntl(fds[0], F_GETFL, 0) | O_NONBLOCK);
185+
if (capture)
186+
::fcntl(fds[0], F_SETFL, ::fcntl(fds[0], F_GETFL, 0) | O_NONBLOCK);
177187

178188
const auto until = std::chrono::steady_clock::now()
179189
+ std::chrono::milliseconds(deadlineMs);
180190
std::array<char, 4096> buf{};
181191
bool killed = false;
182192
int status = 0;
183193

184-
for (;;) {
194+
auto drain = [&]() -> bool {
195+
if (!capture) return false;
185196
ssize_t n;
186-
bool drained = false;
197+
bool any = false;
187198
while ((n = ::read(fds[0], buf.data(), buf.size())) > 0) {
188-
if (sink) sink(ctx, buf.data(),
189-
static_cast<unsigned long>(n));
190-
drained = true;
199+
sink(ctx, buf.data(), static_cast<unsigned long>(n));
200+
any = true;
191201
}
192-
if (drained) continue;
202+
return any;
203+
};
204+
205+
for (;;) {
206+
if (drain()) continue;
193207

194208
pid_t r = ::waitpid(pid, &status, WNOHANG);
195209
if (r == pid) {
196-
// Drain the tail: the child is gone, so this terminates.
197-
while ((n = ::read(fds[0], buf.data(), buf.size())) > 0)
198-
if (sink) sink(ctx, buf.data(),
199-
static_cast<unsigned long>(n));
210+
while (drain()) { /* tail — the child is gone, so this ends */ }
200211
break;
201212
}
202213
if (r < 0 && errno != EINTR && errno != ECHILD) break;
@@ -209,7 +220,7 @@ DeadlineRun capture_with_deadline(const char* const* argvEntries,
209220
struct timespec ts{0, 20'000'000}; // 20ms
210221
::nanosleep(&ts, nullptr);
211222
}
212-
::close(fds[0]);
223+
if (capture) ::close(fds[0]);
213224

214225
out.exit_code = normalize_status(status);
215226
out.timed_out = killed;

src/platform/windows/bounded_process.cppm

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ struct DeadlineRun {
7676
};
7777

7878
// Receives stdout+stderr as it arrives. Called on the calling thread only.
79+
//
80+
// A NULL sink means "do not capture": the child inherits the caller's stdio and
81+
// is still bounded. Same contract as the POSIX peer, and for the same reason —
82+
// an uncaptured bounded run (`run_exec_deadline`) must keep its output LIVE and
83+
// keep the child's stdout a console, or console-detecting children drop their
84+
// colors and every line waits for exit.
7985
using OutputSink = void (*)(void* ctx, const char* data, unsigned long len);
8086

8187
// `commandLine` is already quoted for CreateProcess (callers pass the output
@@ -176,16 +182,20 @@ DeadlineRun capture_with_deadline(const char* commandLine,
176182
DeadlineRun out;
177183
if (deadlineMs <= 0 || !commandLine || !*commandLine) return out;
178184

185+
const bool capture = (sink != nullptr);
186+
179187
SECURITY_ATTRIBUTES sa{};
180188
sa.nLength = sizeof(sa);
181189
sa.bInheritHandle = TRUE;
182190

183191
Handle readEnd, writeEnd;
184-
if (!::CreatePipe(&readEnd.h, &writeEnd.h, &sa, 0)) return out;
185-
// Only the WRITE end may cross into the child. An inheritable read end
186-
// there would keep the pipe alive past the child's exit and the drain
187-
// below would never see EOF.
188-
if (!::SetHandleInformation(readEnd.h, HANDLE_FLAG_INHERIT, 0)) return out;
192+
if (capture) {
193+
if (!::CreatePipe(&readEnd.h, &writeEnd.h, &sa, 0)) return out;
194+
// Only the WRITE end may cross into the child. An inheritable read end
195+
// there would keep the pipe alive past the child's exit and the drain
196+
// below would never see EOF.
197+
if (!::SetHandleInformation(readEnd.h, HANDLE_FLAG_INHERIT, 0)) return out;
198+
}
189199

190200
Handle job;
191201
job.h = ::CreateJobObjectA(nullptr, nullptr);
@@ -198,21 +208,27 @@ DeadlineRun capture_with_deadline(const char* commandLine,
198208
}
199209

200210
STARTUPINFOA si{};
201-
si.cb = sizeof(si);
202-
si.dwFlags = STARTF_USESTDHANDLES;
203-
si.hStdOutput = writeEnd.h;
204-
si.hStdError = writeEnd.h;
205-
si.hStdInput = ::GetStdHandle(STD_INPUT_HANDLE);
211+
si.cb = sizeof(si);
212+
if (capture) {
213+
si.dwFlags = STARTF_USESTDHANDLES;
214+
si.hStdOutput = writeEnd.h;
215+
si.hStdError = writeEnd.h;
216+
si.hStdInput = ::GetStdHandle(STD_INPUT_HANDLE);
217+
}
218+
// else: no STARTF_USESTDHANDLES — the child inherits our console.
206219

207220
PROCESS_INFORMATION pi{};
208221
std::string cmdBuf(commandLine); // CreateProcessA may modify it
209222
auto envBlock = environment_block(envEntries, envCount);
210223

211224
// CREATE_SUSPENDED so the child joins the job BEFORE it can spawn
212225
// anything — a grandchild created in that gap would escape the kill.
226+
// CREATE_NO_WINDOW only when capturing: an uncaptured run is meant to be
227+
// seen, and suppressing the console for it would hide the output this
228+
// branch exists to show.
213229
BOOL ok = ::CreateProcessA(
214230
nullptr, cmdBuf.data(), nullptr, nullptr, /*bInheritHandles=*/TRUE,
215-
CREATE_SUSPENDED | CREATE_NO_WINDOW,
231+
CREATE_SUSPENDED | (capture ? CREATE_NO_WINDOW : 0u),
216232
envBlock.data(),
217233
(cwd && *cwd) ? cwd : nullptr,
218234
&si, &pi);
@@ -233,6 +249,7 @@ DeadlineRun capture_with_deadline(const char* commandLine,
233249
bool killed = false;
234250

235251
auto drain_available = [&]() -> bool {
252+
if (!capture) return false;
236253
DWORD avail = 0;
237254
if (!::PeekNamedPipe(readEnd.h, nullptr, 0, nullptr, &avail, nullptr))
238255
return false;

0 commit comments

Comments
 (0)