From 9e4cd1f4e6c3195665daa80c6a438cfd4daa92b1 Mon Sep 17 00:00:00 2001 From: Rabin Gaire Date: Wed, 20 Apr 2022 18:02:41 +0545 Subject: [PATCH 1/4] fix child process spawn on macos hangs issue When a child process with stdin, stdout behavior set to pipe is ran on macos it used to hang which has been fixed. Issue existed because we forgot to call `posix_spawn_file_actions_addclose` syscall on user exposed file descriptor which resulted on file descriptor not closing properly. --- lib/std/child_process.zig | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 8171ff7eea..aef96cbde3 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -593,9 +593,9 @@ pub const ChildProcess = struct { var actions = try os.posix_spawn.Actions.init(); defer actions.deinit(); - try setUpChildIoPosixSpawn(self.stdin_behavior, &actions, stdin_pipe[0], os.STDIN_FILENO, dev_null_fd); - try setUpChildIoPosixSpawn(self.stdout_behavior, &actions, stdout_pipe[1], os.STDOUT_FILENO, dev_null_fd); - try setUpChildIoPosixSpawn(self.stderr_behavior, &actions, stderr_pipe[1], os.STDERR_FILENO, dev_null_fd); + try setUpChildIoPosixSpawn(self.stdin_behavior, &actions, stdin_pipe, os.STDIN_FILENO, dev_null_fd); + try setUpChildIoPosixSpawn(self.stdout_behavior, &actions, stdout_pipe, os.STDOUT_FILENO, dev_null_fd); + try setUpChildIoPosixSpawn(self.stderr_behavior, &actions, stderr_pipe, os.STDERR_FILENO, dev_null_fd); if (self.cwd_dir) |cwd| { try actions.fchdir(cwd.fd); @@ -650,12 +650,16 @@ pub const ChildProcess = struct { fn setUpChildIoPosixSpawn( stdio: StdIo, actions: *os.posix_spawn.Actions, - pipe_fd: i32, + pipe_fd: [2]i32, std_fileno: i32, dev_null_fd: i32, ) !void { switch (stdio) { - .Pipe => try actions.dup2(pipe_fd, std_fileno), + .Pipe => { + const idx: usize = if (std_fileno == 0) 0 else 1; + try actions.dup2(pipe_fd[idx], std_fileno); + try actions.close(pipe_fd[1-idx]); + }, .Close => try actions.close(std_fileno), .Inherit => {}, .Ignore => try actions.dup2(dev_null_fd, std_fileno), From d976456ef665bf0aba3a83a8e7fccb4a92b2d3b2 Mon Sep 17 00:00:00 2001 From: Rabin Gaire Date: Wed, 20 Apr 2022 18:37:10 +0545 Subject: [PATCH 2/4] add test case for child_process spawn logic tests creating a child process with stdin/stdout behavior set to StdIo.Pipe. --- lib/std/child_process.zig | 46 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index aef96cbde3..41d2cf2850 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -1379,3 +1379,49 @@ test "build and call child_process" { const ret_val = try child_proc.spawnAndWait(); try testing.expectEqual(ret_val, .{ .Exited = 0 }); } + +test "creating a child process with stdin/stdout behavior set to StdIo.Pipe" { + if (builtin.os.tag == .wasi) return error.SkipZigTest; + const testing = std.testing; + const allocator = testing.allocator; + + var child_process = try std.ChildProcess.init( + &[_][]const u8{ testing.zig_exe_path, "fmt", "--stdin" }, + allocator, + ); + defer child_process.deinit(); + child_process.stdin_behavior = .Pipe; + child_process.stdout_behavior = .Pipe; + + try child_process.spawn(); + + const input_program = + \\ const std = @import("std"); + \\ pub fn main() void { + \\ std.debug.print("Hello World", .{}); + \\ } + ; + + try child_process.stdin.?.writer().writeAll(input_program); + child_process.stdin.?.close(); + child_process.stdin = null; + + const out_bytes = try child_process.stdout.?.reader().readAllAlloc(allocator, std.math.maxInt(usize)); + defer allocator.free(out_bytes); + + switch (try child_process.wait()) { + .Exited => |code| if (code == 0) { + const expected_program = + \\const std = @import("std"); + \\pub fn main() void { + \\ std.debug.print("Hello World", .{}); + \\} + \\ + ; + try testing.expectEqualStrings(expected_program, out_bytes); + }, + else => { + try testing.expect(false); + } + } +} From 4ece507b5aa3d89adb3b99258f089aa8f48184b2 Mon Sep 17 00:00:00 2001 From: Rabin Gaire Date: Thu, 21 Apr 2022 13:28:41 +0545 Subject: [PATCH 3/4] update test message and using unreachable keyword for unintended test code path --- lib/std/child_process.zig | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 41d2cf2850..62d141bf16 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -1380,7 +1380,7 @@ test "build and call child_process" { try testing.expectEqual(ret_val, .{ .Exited = 0 }); } -test "creating a child process with stdin/stdout behavior set to StdIo.Pipe" { +test "creating a child process with stdin and stdout behavior set to StdIo.Pipe" { if (builtin.os.tag == .wasi) return error.SkipZigTest; const testing = std.testing; const allocator = testing.allocator; @@ -1420,8 +1420,6 @@ test "creating a child process with stdin/stdout behavior set to StdIo.Pipe" { ; try testing.expectEqualStrings(expected_program, out_bytes); }, - else => { - try testing.expect(false); - } + else => unreachable } } From 50ec55faaf4b218c4ded8a73864bbd0c85571e23 Mon Sep 17 00:00:00 2001 From: Rabin Gaire Date: Thu, 21 Apr 2022 14:12:08 +0545 Subject: [PATCH 4/4] ran zig fmt on changes --- lib/std/child_process.zig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 62d141bf16..f2b978ba9f 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -658,7 +658,7 @@ pub const ChildProcess = struct { .Pipe => { const idx: usize = if (std_fileno == 0) 0 else 1; try actions.dup2(pipe_fd[idx], std_fileno); - try actions.close(pipe_fd[1-idx]); + try actions.close(pipe_fd[1 - idx]); }, .Close => try actions.close(std_fileno), .Inherit => {}, @@ -1408,7 +1408,7 @@ test "creating a child process with stdin and stdout behavior set to StdIo.Pipe" const out_bytes = try child_process.stdout.?.reader().readAllAlloc(allocator, std.math.maxInt(usize)); defer allocator.free(out_bytes); - + switch (try child_process.wait()) { .Exited => |code| if (code == 0) { const expected_program = @@ -1420,6 +1420,6 @@ test "creating a child process with stdin and stdout behavior set to StdIo.Pipe" ; try testing.expectEqualStrings(expected_program, out_bytes); }, - else => unreachable + else => unreachable, } }