authorgravatar for mlugg@mlugg.co.ukMatthew Lugg <mlugg@mlugg.co.uk> 2025-01-10 10:27:32+00:00
committergravatar for mlugg@mlugg.co.ukMatthew Lugg <mlugg@mlugg.co.uk> 2025-01-20 00:27:19+00:00
log048e85f27e805f51c214e8e87e91a367a2873328
tree9c159bc7888a9b3d758aa0c4ec6145f773354165
parentb8e568504e1dca38273a5845ac20d056cd7db5bb
signaturelock-open Commit is signed but in an unrecognized format.

std.process.Child: add `waitForSpawn`

`std.Build.Step.Run` makes the very reasonable assumption that `error.InvalidExe` will be reported on `spawn` if it will happen. However, this property does not currently hold on POSIX targets. This is, through a slightly convoluted series of events, partially responsible for the sporadic `BrokenPipe` errors we've been seeing more and more in CI runs. Making `spawn` wait for the child to exec in the POSIX path introduces a block of up to 400us. So, instead of doing that, we add a new API for this particular case: `waitForSpawn`. This function is a nop on Windows, but on POSIX it blocks until the child successfully (or otherwise) calls `execvpe`, and reports the error if necessary. `std.Build.Step.Run` calls this function, so that it can get `error.InvalidExe` when it wants it. I'm not convinced that this API is optimal. However, I think this entire API needs to be either heavily refactored or straight-up redesigned (related: #22504), so I'm not too worried about hitting the perfect API: I'd rather just fix this bug for now, and figure out the long-term goal a bit later.

2 files changed, 50 insertions(+), 80 deletions(-)

lib/std/Build/Step/Run.zig+3
......@@ -1354,6 +1354,9 @@ fn spawnChildAndCollect(
13541354 _ = child.kill() catch {};
13551355 }
13561356
1357 // We need to report `error.InvalidExe` *now* if applicable.
1358 try child.waitForSpawn();
1359
13571360 var timer = try std.time.Timer.start();
13581361
13591362 const result = if (run.stdio == .zig_test)
lib/std/process/Child.zig+47-80
......@@ -73,7 +73,7 @@ cwd: ?[]const u8,
7373/// Once that is done, `cwd` will be deprecated in favor of this field.
7474cwd_dir: ?fs.Dir = null,
7575
76err_pipe: ?if (native_os == .windows) void else [2]posix.fd_t,
76err_pipe: if (native_os == .windows) void else ?posix.fd_t,
7777
7878expand_arg0: Arg0Expand,
7979
......@@ -211,7 +211,7 @@ pub fn init(argv: []const []const u8, allocator: mem.Allocator) ChildProcess {
211211 .argv = argv,
212212 .id = undefined,
213213 .thread_handle = undefined,
214 .err_pipe = null,
214 .err_pipe = if (native_os == .windows) {} else null,
215215 .term = null,
216216 .env_map = null,
217217 .cwd = null,
......@@ -293,17 +293,49 @@ pub fn killPosix(self: *ChildProcess) !Term {
293293 error.ProcessNotFound => return error.AlreadyTerminated,
294294 else => return err,
295295 };
296 self.waitUnwrapped();
296 self.waitUnwrappedPosix();
297297 return self.term.?;
298298}
299299
300300pub const WaitError = SpawnError || std.os.windows.GetProcessMemoryInfoError;
301301
302/// On some targets, `spawn` may not report all spawn errors, such as `error.InvalidExe`.
303/// This function will block until any spawn errors can be reported, and return them.
304pub fn waitForSpawn(self: *ChildProcess) SpawnError!void {
305 if (native_os == .windows) return; // `spawn` reports everything
306 if (self.term) |term| {
307 _ = term catch |spawn_err| return spawn_err;
308 return;
309 }
310
311 const err_pipe = self.err_pipe orelse return;
312 self.err_pipe = null;
313
314 // Wait for the child to report any errors in or before `execvpe`.
315 if (readIntFd(err_pipe)) |child_err_int| {
316 posix.close(err_pipe);
317 const child_err: SpawnError = @errorCast(@errorFromInt(child_err_int));
318 self.term = child_err;
319 return child_err;
320 } else |_| {
321 // Write end closed by CLOEXEC at the time of the `execvpe` call, indicating success!
322 posix.close(err_pipe);
323 }
324}
325
302326/// Blocks until child process terminates and then cleans up all resources.
303327pub fn wait(self: *ChildProcess) WaitError!Term {
304 const term = if (native_os == .windows) try self.waitWindows() else self.waitPosix();
328 try self.waitForSpawn(); // report spawn errors
329 if (self.term) |term| {
330 self.cleanupStreams();
331 return term;
332 }
333 switch (native_os) {
334 .windows => try self.waitUnwrappedWindows(),
335 else => self.waitUnwrappedPosix(),
336 }
305337 self.id = undefined;
306 return term;
338 return self.term.?;
307339}
308340
309341pub const RunResult = struct {
......@@ -405,26 +437,6 @@ pub fn run(args: struct {
405437 };
406438}
407439
408fn waitWindows(self: *ChildProcess) WaitError!Term {
409 if (self.term) |term| {
410 self.cleanupStreams();
411 return term;
412 }
413
414 try self.waitUnwrappedWindows();
415 return self.term.?;
416}
417
418fn waitPosix(self: *ChildProcess) SpawnError!Term {
419 if (self.term) |term| {
420 self.cleanupStreams();
421 return term;
422 }
423
424 self.waitUnwrapped();
425 return self.term.?;
426}
427
428440fn waitUnwrappedWindows(self: *ChildProcess) WaitError!void {
429441 const result = windows.WaitForSingleObjectEx(self.id, windows.INFINITE, false);
430442
......@@ -447,7 +459,7 @@ fn waitUnwrappedWindows(self: *ChildProcess) WaitError!void {
447459 return result;
448460}
449461
450fn waitUnwrapped(self: *ChildProcess) void {
462fn waitUnwrappedPosix(self: *ChildProcess) void {
451463 const res: posix.WaitPidResult = res: {
452464 if (self.request_resource_usage_statistics) {
453465 switch (native_os) {
......@@ -469,7 +481,7 @@ fn waitUnwrapped(self: *ChildProcess) void {
469481}
470482
471483fn handleWaitResult(self: *ChildProcess, status: u32) void {
472 self.term = self.cleanupAfterWait(status);
484 self.term = statusToTerm(status);
473485}
474486
475487fn cleanupStreams(self: *ChildProcess) void {
......@@ -487,46 +499,6 @@ fn cleanupStreams(self: *ChildProcess) void {
487499 }
488500}
489501
490fn cleanupAfterWait(self: *ChildProcess, status: u32) !Term {
491 if (self.err_pipe) |err_pipe| {
492 defer destroyPipe(err_pipe);
493
494 if (native_os == .linux) {
495 var fd = [1]posix.pollfd{posix.pollfd{
496 .fd = err_pipe[0],
497 .events = posix.POLL.IN,
498 .revents = undefined,
499 }};
500
501 // Check if the eventfd buffer stores a non-zero value by polling
502 // it, that's the error code returned by the child process.
503 _ = posix.poll(&fd, 0) catch unreachable;
504
505 // According to eventfd(2) the descriptor is readable if the counter
506 // has a value greater than 0
507 if ((fd[0].revents & posix.POLL.IN) != 0) {
508 const err_int = try readIntFd(err_pipe[0]);
509 return @as(SpawnError, @errorCast(@errorFromInt(err_int)));
510 }
511 } else {
512 // Write maxInt(ErrInt) to the write end of the err_pipe. This is after
513 // waitpid, so this write is guaranteed to be after the child
514 // pid potentially wrote an error. This way we can do a blocking
515 // read on the error pipe and either get maxInt(ErrInt) (no error) or
516 // an error code.
517 try writeIntFd(err_pipe[1], maxInt(ErrInt));
518 const err_int = try readIntFd(err_pipe[0]);
519 // Here we potentially return the fork child's error from the parent
520 // pid.
521 if (err_int != maxInt(ErrInt)) {
522 return @as(SpawnError, @errorCast(@errorFromInt(err_int)));
523 }
524 }
525 }
526
527 return statusToTerm(status);
528}
529
530502fn statusToTerm(status: u32) Term {
531503 return if (posix.W.IFEXITED(status))
532504 Term{ .Exited = posix.W.EXITSTATUS(status) }
......@@ -636,18 +608,9 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void {
636608 }
637609 };
638610
639 // This pipe is used to communicate errors between the time of fork
640 // and execve from the child process to the parent process.
641 const err_pipe = blk: {
642 if (native_os == .linux) {
643 const fd = try posix.eventfd(0, linux.EFD.CLOEXEC);
644 // There's no distinction between the readable and the writeable
645 // end with eventfd
646 break :blk [2]posix.fd_t{ fd, fd };
647 } else {
648 break :blk try posix.pipe2(.{ .CLOEXEC = true });
649 }
650 };
611 // This pipe communicates to the parent errors in the child between `fork` and `execvpe`.
612 // It is closed by the child (via CLOEXEC) without writing if `execvpe` succeeds.
613 const err_pipe: [2]posix.fd_t = try posix.pipe2(.{ .CLOEXEC = true });
651614 errdefer destroyPipe(err_pipe);
652615
653616 const pid_result = try posix.fork();
......@@ -687,6 +650,11 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void {
687650 }
688651
689652 // we are the parent
653 errdefer comptime unreachable; // The child is forked; we must not error from now on
654
655 posix.close(err_pipe[1]); // make sure only the child holds the write end open
656 self.err_pipe = err_pipe[0];
657
690658 const pid: i32 = @intCast(pid_result);
691659 if (self.stdin_behavior == .Pipe) {
692660 self.stdin = .{ .handle = stdin_pipe[1] };
......@@ -705,7 +673,6 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void {
705673 }
706674
707675 self.id = pid;
708 self.err_pipe = err_pipe;
709676 self.term = null;
710677
711678 if (self.stdin_behavior == .Pipe) {