From dc810eb73b08edfc445f2ce043806be00a236abf Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Tue, 20 Oct 2020 08:51:21 +0200 Subject: [PATCH 1/6] std: Avoid deadlocking in ChildProcess.exec Reading stdin&stderr at different times may lead to nasty deadlocks (eg. when stdout is read before stderr and the child process doesn't write anything onto stdout). Implement a polling mechanism to make sure this won't happen: we read data from stderr/stdout as it becomes ready and then it's copied into an ArrayList provided by the user, avoiding any kind of blocking read. --- lib/std/child_process.zig | 173 ++++++++++++++++++++++++++++++-- lib/std/os/windows/bits.zig | 13 +++ lib/std/os/windows/kernel32.zig | 33 ++++++ 3 files changed, 208 insertions(+), 11 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index b1dcc3737f..6e62063e26 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -186,6 +186,106 @@ pub const ChildProcess = struct { pub const exec2 = @compileError("deprecated: exec2 is renamed to exec"); + fn collectOutputPosix(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void { + var poll_fds = [_]os.pollfd{ + .{ .fd = child.stdout.?.handle, .events = os.POLLIN, .revents = undefined }, + .{ .fd = child.stderr.?.handle, .events = os.POLLIN, .revents = undefined }, + }; + + var dead_fds: usize = 0; + var loop_buf: [4096]u8 = undefined; + + while (dead_fds < poll_fds.len) { + const events = try os.poll(&poll_fds, std.math.maxInt(i32)); + if (events == 0) continue; + + // Try reading whatever is available before checking the error + // conditions. + if (poll_fds[0].revents & os.POLLIN != 0) { + // stdout is ready. + const n = try os.read(poll_fds[0].fd, &loop_buf); + try stdout.appendSlice(loop_buf[0..n]); + } + if (poll_fds[1].revents & os.POLLIN != 0) { + // stderr is ready. + const n = try os.read(poll_fds[1].fd, &loop_buf); + try stderr.appendSlice(loop_buf[0..n]); + } + + // Exclude the fds that signaled an error. + if (poll_fds[0].revents & (os.POLLERR | os.POLLNVAL | os.POLLHUP) != 0) { + poll_fds[0].fd = -1; + dead_fds += 1; + } + if (poll_fds[1].revents & (os.POLLERR | os.POLLNVAL | os.POLLHUP) != 0) { + poll_fds[1].fd = -1; + dead_fds += 1; + } + } + } + + fn collectOutputWindows(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void { + // The order of the objects here is important, WaitForMultipleObjects + // uses the same order when scanning the events. + var wait_objects = [_]windows.kernel32.HANDLE{ + child.handle, child.stdout.?.handle, child.stderr.?.handle, + }; + // XXX: Calling zeroes([2]windows.OVERLAPPED) causes the stage1 compiler + // to crash and burn. + var overlapped = [_]windows.OVERLAPPED{ + mem.zeroes(windows.OVERLAPPED), + mem.zeroes(windows.OVERLAPPED), + }; + var temp_buf: [2][4096]u8 = undefined; + + // Kickstart the loop by issuing two async reads. + // ReadFile returns false and GetLastError returns ERROR_IO_PENDING if + // everything is ok. + _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]); + _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]); + + while (true) { + const status = windows.kernel32.WaitForMultipleObjects(wait_objects.len, &wait_objects, 0, windows.INFINITE); + std.debug.print("status {x}\n", .{status}); + switch (status) { + windows.WAIT_OBJECT_0 + 0 => { + // The child process was terminated. + break; + }, + windows.WAIT_OBJECT_0 + 1 => { + // stdout is ready. + var read_bytes: u32 = undefined; + if (windows.kernel32.GetOverlappedResult(wait_objects[1], &overlapped[0], &read_bytes, 0) == 0) { + switch (windows.kernel32.GetLastError()) { + else => |err| return windows.unexpectedError(err), + } + } + try stdout.appendSlice(temp_buf[0][0..read_bytes]); + _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]); + }, + windows.WAIT_OBJECT_0 + 2 => { + // stderr is ready. + var read_bytes: u32 = undefined; + if (windows.kernel32.GetOverlappedResult(wait_objects[2], &overlapped[1], &read_bytes, 0) == 0) { + switch (windows.kernel32.GetLastError()) { + else => |err| return windows.unexpectedError(err), + } + } + try stdout.appendSlice(temp_buf[1][0..read_bytes]); + _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]); + }, + windows.WAIT_FAILED => { + switch (windows.kernel32.GetLastError()) { + else => |err| return windows.unexpectedError(err), + } + }, + // We're waiting with an infinite timeout + windows.WAIT_TIMEOUT => unreachable, + else => unreachable, + } + } + } + /// Spawns a child process, waits for it, collecting stdout and stderr, and then returns. /// If it succeeds, the caller owns result.stdout and result.stderr memory. pub fn exec(args: struct { @@ -210,19 +310,21 @@ pub const ChildProcess = struct { try child.spawn(); - const stdout_in = child.stdout.?.reader(); - const stderr_in = child.stderr.?.reader(); + var stdout = std.ArrayList(u8).init(args.allocator); + var stderr = std.ArrayList(u8).init(args.allocator); - // TODO https://github.com/ziglang/zig/issues/6343 - const stdout = try stdout_in.readAllAlloc(args.allocator, args.max_output_bytes); - errdefer args.allocator.free(stdout); - const stderr = try stderr_in.readAllAlloc(args.allocator, args.max_output_bytes); - errdefer args.allocator.free(stderr); + // XXX: Respect max_output_bytes + // XXX: Smarter reading logic, read directly into the ArrayList + if (builtin.os.tag == .windows) { + try collectOutputWindows(child, &stdout, &stderr, args.max_output_bytes); + } else { + try collectOutputPosix(child, &stdout, &stderr, args.max_output_bytes); + } return ExecResult{ .term = try child.wait(), - .stdout = stdout, - .stderr = stderr, + .stdout = stdout.toOwnedSlice(), + .stderr = stderr.toOwnedSlice(), }; } @@ -555,7 +657,7 @@ pub const ChildProcess = struct { var g_hChildStd_OUT_Wr: ?windows.HANDLE = null; switch (self.stdout_behavior) { StdIo.Pipe => { - try windowsMakePipeOut(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr); + try windowsMakePipe(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr); }, StdIo.Ignore => { g_hChildStd_OUT_Wr = nul_handle; @@ -575,7 +677,7 @@ pub const ChildProcess = struct { var g_hChildStd_ERR_Wr: ?windows.HANDLE = null; switch (self.stderr_behavior) { StdIo.Pipe => { - try windowsMakePipeOut(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr); + try windowsMakePipe(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr); }, StdIo.Ignore => { g_hChildStd_ERR_Wr = nul_handle; @@ -808,6 +910,55 @@ fn windowsDestroyPipe(rd: ?windows.HANDLE, wr: ?windows.HANDLE) void { if (wr) |h| os.close(h); } +var pipe_name_counter = std.atomic.Int(u32).init(1); + +fn windowsMakePipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void { + var tmp_buf: [128]u8 = undefined; + // Forge a random path for the pipe. + const pipe_path = std.fmt.bufPrintZ( + &tmp_buf, + "\\\\.\\pipe\\zig-childprocess-{d}-{d}", + .{ windows.kernel32.GetCurrentProcessId(), pipe_name_counter.fetchAdd(1) }, + ) catch unreachable; + + // Create the read handle that can be used with overlapped IO ops. + const read_handle = windows.kernel32.CreateNamedPipeA( + pipe_path, + windows.PIPE_ACCESS_INBOUND | windows.FILE_FLAG_OVERLAPPED, + windows.PIPE_TYPE_BYTE, + 1, + 0x1000, + 0x1000, + 0, + sattr, + ); + if (read_handle == windows.INVALID_HANDLE_VALUE) { + switch (windows.kernel32.GetLastError()) { + else => |err| return windows.unexpectedError(err), + } + } + + const write_handle = windows.kernel32.CreateFileA( + pipe_path, + windows.GENERIC_WRITE, + 0, + sattr, + windows.OPEN_EXISTING, + windows.FILE_ATTRIBUTE_NORMAL, + null, + ); + if (write_handle == windows.INVALID_HANDLE_VALUE) { + switch (windows.kernel32.GetLastError()) { + else => |err| return windows.unexpectedError(err), + } + } + + try windows.SetHandleInformation(read_handle, windows.HANDLE_FLAG_INHERIT, 0); + + rd.* = read_handle; + wr.* = write_handle; +} + fn windowsMakePipeIn(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void { var rd_h: windows.HANDLE = undefined; var wr_h: windows.HANDLE = undefined; diff --git a/lib/std/os/windows/bits.zig b/lib/std/os/windows/bits.zig index f5d520c580..0d69087a46 100644 --- a/lib/std/os/windows/bits.zig +++ b/lib/std/os/windows/bits.zig @@ -438,6 +438,19 @@ pub const SECURITY_ATTRIBUTES = extern struct { pub const PSECURITY_ATTRIBUTES = *SECURITY_ATTRIBUTES; pub const LPSECURITY_ATTRIBUTES = *SECURITY_ATTRIBUTES; +pub const PIPE_ACCESS_INBOUND = 0x00000001; +pub const PIPE_ACCESS_OUTBOUND = 0x00000002; +pub const PIPE_ACCESS_DUPLEX = 0x00000003; + +pub const PIPE_TYPE_BYTE = 0x00000000; +pub const PIPE_TYPE_MESSAGE = 0x00000004; + +pub const PIPE_READMODE_BYTE = 0x00000000; +pub const PIPE_READMODE_MESSAGE = 0x00000002; + +pub const PIPE_WAIT = 0x00000000; +pub const PIPE_NOWAIT = 0x00000001; + pub const GENERIC_READ = 0x80000000; pub const GENERIC_WRITE = 0x40000000; pub const GENERIC_EXECUTE = 0x20000000; diff --git a/lib/std/os/windows/kernel32.zig b/lib/std/os/windows/kernel32.zig index 444234876c..83431bb5b6 100644 --- a/lib/std/os/windows/kernel32.zig +++ b/lib/std/os/windows/kernel32.zig @@ -15,6 +15,29 @@ pub extern "kernel32" fn CloseHandle(hObject: HANDLE) callconv(WINAPI) BOOL; pub extern "kernel32" fn CreateDirectoryW(lpPathName: [*:0]const u16, lpSecurityAttributes: ?*SECURITY_ATTRIBUTES) callconv(WINAPI) BOOL; pub extern "kernel32" fn SetEndOfFile(hFile: HANDLE) callconv(WINAPI) BOOL; +pub extern "kernel32" fn GetCurrentProcessId() callconv(.Stdcall) DWORD; + +pub extern "kernel32" fn CreateNamedPipeA( + lpName: [*:0]const u8, + dwOpenMode: DWORD, + dwPipeMode: DWORD, + nMaxInstances: DWORD, + nOutBufferSize: DWORD, + nInBufferSize: DWORD, + nDefaultTimeOut: DWORD, + lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, +) callconv(.Stdcall) HANDLE; +pub extern "kernel32" fn CreateNamedPipeW( + lpName: LPCWSTR, + dwOpenMode: DWORD, + dwPipeMode: DWORD, + nMaxInstances: DWORD, + nOutBufferSize: DWORD, + nInBufferSize: DWORD, + nDefaultTimeOut: DWORD, + lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, +) callconv(.Stdcall) HANDLE; + pub extern "kernel32" fn CreateEventExW( lpEventAttributes: ?*SECURITY_ATTRIBUTES, lpName: [*:0]const u16, @@ -32,6 +55,16 @@ pub extern "kernel32" fn CreateFileW( hTemplateFile: ?HANDLE, ) callconv(WINAPI) HANDLE; +pub extern "kernel32" fn CreateFileA( + lpFileName: [*:0]const u8, + dwDesiredAccess: DWORD, + dwShareMode: DWORD, + lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, + dwCreationDisposition: DWORD, + dwFlagsAndAttributes: DWORD, + hTemplateFile: ?HANDLE, +) callconv(.Stdcall) HANDLE; + pub extern "kernel32" fn CreatePipe( hReadPipe: *HANDLE, hWritePipe: *HANDLE, From 2c16a9668632be83c5e1bb0baeb00d221bb0eca1 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Wed, 21 Oct 2020 16:29:15 +0200 Subject: [PATCH 2/6] std: Fix poll definitions for FreeBSD/Darwin --- lib/std/os/bits/darwin.zig | 2 +- lib/std/os/bits/freebsd.zig | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/lib/std/os/bits/darwin.zig b/lib/std/os/bits/darwin.zig index b9fe23eeb1..5016134e76 100644 --- a/lib/std/os/bits/darwin.zig +++ b/lib/std/os/bits/darwin.zig @@ -1461,7 +1461,7 @@ pub const LOCK_EX = 2; pub const LOCK_UN = 8; pub const LOCK_NB = 4; -pub const nfds_t = usize; +pub const nfds_t = u32; pub const pollfd = extern struct { fd: fd_t, events: i16, diff --git a/lib/std/os/bits/freebsd.zig b/lib/std/os/bits/freebsd.zig index 08713bb2e4..351e86caa1 100644 --- a/lib/std/os/bits/freebsd.zig +++ b/lib/std/os/bits/freebsd.zig @@ -1480,3 +1480,28 @@ pub const rlimit = extern struct { pub const SHUT_RD = 0; pub const SHUT_WR = 1; pub const SHUT_RDWR = 2; + +pub const nfds_t = u32; + +pub const pollfd = extern struct { + fd: fd_t, + events: i16, + revents: i16, +}; + +/// any readable data available. +pub const POLLIN = 0x0001; +/// OOB/Urgent readable data. +pub const POLLPRI = 0x0002; +/// file descriptor is writeable. +pub const POLLOUT = 0x0004; +/// non-OOB/URG data available. +pub const POLLRDNORM = 0x0040; +/// no write type differentiation. +pub const POLLWRNORM = POLLOUT; +/// OOB/Urgent readable data. +pub const POLLRDBAND = 0x0080; +/// OOB/Urgent data can be written. +pub const POLLWRBAND = 0x0100; +/// like POLLIN, except ignore EOF. +pub const POLLINIGNEOF = 0x2000; From 1667c937a0869cc266ce43b4ecbde4ad49ca23c5 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Wed, 21 Oct 2020 16:54:38 +0200 Subject: [PATCH 3/6] std: Uniform polling logic for Windows and Unix Keep polling until there are enough open handles, if the child process terminates closing the handles or explicitly closes them we just quit polling and wait for the process handle to signal the termination condition. --- lib/std/child_process.zig | 49 +++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 6e62063e26..e85ce83755 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -225,11 +225,11 @@ pub const ChildProcess = struct { } fn collectOutputWindows(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void { - // The order of the objects here is important, WaitForMultipleObjects - // uses the same order when scanning the events. var wait_objects = [_]windows.kernel32.HANDLE{ - child.handle, child.stdout.?.handle, child.stderr.?.handle, + child.stdout.?.handle, child.stderr.?.handle, }; + var waiting_objects: u32 = wait_objects.len; + // XXX: Calling zeroes([2]windows.OVERLAPPED) causes the stage1 compiler // to crash and burn. var overlapped = [_]windows.OVERLAPPED{ @@ -241,38 +241,31 @@ pub const ChildProcess = struct { // Kickstart the loop by issuing two async reads. // ReadFile returns false and GetLastError returns ERROR_IO_PENDING if // everything is ok. - _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]); - _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]); + _ = windows.kernel32.ReadFile(wait_objects[0], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]); + _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]); - while (true) { - const status = windows.kernel32.WaitForMultipleObjects(wait_objects.len, &wait_objects, 0, windows.INFINITE); - std.debug.print("status {x}\n", .{status}); + poll: while (waiting_objects > 0) { + const status = windows.kernel32.WaitForMultipleObjects(waiting_objects, &wait_objects, 0, windows.INFINITE); switch (status) { - windows.WAIT_OBJECT_0 + 0 => { - // The child process was terminated. - break; - }, - windows.WAIT_OBJECT_0 + 1 => { - // stdout is ready. + windows.WAIT_OBJECT_0 + 0...windows.WAIT_OBJECT_0 + 1 => { + // stdout (or stderr) is ready. + const object = status - windows.WAIT_OBJECT_0; + var read_bytes: u32 = undefined; - if (windows.kernel32.GetOverlappedResult(wait_objects[1], &overlapped[0], &read_bytes, 0) == 0) { + if (windows.kernel32.GetOverlappedResult(wait_objects[object], &overlapped[object], &read_bytes, 0) == 0) { switch (windows.kernel32.GetLastError()) { + .BROKEN_PIPE => { + // Move it to the end to remove it. + if (object != waiting_objects - 1) + mem.swap(windows.kernel32.HANDLE, &wait_objects[object], &wait_objects[waiting_objects - 1]); + waiting_objects -= 1; + continue :poll; + }, else => |err| return windows.unexpectedError(err), } } - try stdout.appendSlice(temp_buf[0][0..read_bytes]); - _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]); - }, - windows.WAIT_OBJECT_0 + 2 => { - // stderr is ready. - var read_bytes: u32 = undefined; - if (windows.kernel32.GetOverlappedResult(wait_objects[2], &overlapped[1], &read_bytes, 0) == 0) { - switch (windows.kernel32.GetLastError()) { - else => |err| return windows.unexpectedError(err), - } - } - try stdout.appendSlice(temp_buf[1][0..read_bytes]); - _ = windows.kernel32.ReadFile(wait_objects[2], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]); + try stdout.appendSlice(temp_buf[object][0..read_bytes]); + _ = windows.kernel32.ReadFile(wait_objects[object], &temp_buf[object], temp_buf[object].len, null, &overlapped[object]); }, windows.WAIT_FAILED => { switch (windows.kernel32.GetLastError()) { From b297c2eae92f25af006cd909d25e98672d39eddc Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Wed, 21 Oct 2020 18:24:24 +0200 Subject: [PATCH 4/6] std: Fix compilation on FreeBSD/Darwin --- lib/std/os.zig | 4 +++- lib/std/os/bits/freebsd.zig | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/std/os.zig b/lib/std/os.zig index 28d4f6bb1a..88f6292629 100644 --- a/lib/std/os.zig +++ b/lib/std/os.zig @@ -5269,7 +5269,9 @@ pub const PollError = error{ pub fn poll(fds: []pollfd, timeout: i32) PollError!usize { while (true) { - const rc = system.poll(fds.ptr, fds.len, timeout); + const fds_count = math.cast(nfds_t, fds.len) catch + return error.SystemResources; + const rc = system.poll(fds.ptr, fds_count, timeout); if (builtin.os.tag == .windows) { if (rc == windows.ws2_32.SOCKET_ERROR) { switch (windows.ws2_32.WSAGetLastError()) { diff --git a/lib/std/os/bits/freebsd.zig b/lib/std/os/bits/freebsd.zig index 351e86caa1..13e14df33c 100644 --- a/lib/std/os/bits/freebsd.zig +++ b/lib/std/os/bits/freebsd.zig @@ -1505,3 +1505,12 @@ pub const POLLRDBAND = 0x0080; pub const POLLWRBAND = 0x0100; /// like POLLIN, except ignore EOF. pub const POLLINIGNEOF = 0x2000; +/// some poll error occurred. +pub const POLLERR = 0x0008; +/// file descriptor was "hung up". +pub const POLLHUP = 0x0010; +/// requested events "invalid". +pub const POLLNVAL = 0x0020; + +pub const POLLSTANDARD = POLLIN | POLLPRI | POLLOUT | POLLRDNORM | POLLRDBAND | + POLLWRBAND | POLLERR | POLLHUP | POLLNVAL; From 892b37cdae89fe23d2a19b0b512fc1f7a73dd6ce Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sun, 6 Dec 2020 10:50:06 +0100 Subject: [PATCH 5/6] std: Use WINAPI instead of .Stdcall --- lib/std/os/windows/kernel32.zig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/std/os/windows/kernel32.zig b/lib/std/os/windows/kernel32.zig index 83431bb5b6..d7f2655687 100644 --- a/lib/std/os/windows/kernel32.zig +++ b/lib/std/os/windows/kernel32.zig @@ -15,7 +15,7 @@ pub extern "kernel32" fn CloseHandle(hObject: HANDLE) callconv(WINAPI) BOOL; pub extern "kernel32" fn CreateDirectoryW(lpPathName: [*:0]const u16, lpSecurityAttributes: ?*SECURITY_ATTRIBUTES) callconv(WINAPI) BOOL; pub extern "kernel32" fn SetEndOfFile(hFile: HANDLE) callconv(WINAPI) BOOL; -pub extern "kernel32" fn GetCurrentProcessId() callconv(.Stdcall) DWORD; +pub extern "kernel32" fn GetCurrentProcessId() callconv(WINAPI) DWORD; pub extern "kernel32" fn CreateNamedPipeA( lpName: [*:0]const u8, @@ -26,7 +26,7 @@ pub extern "kernel32" fn CreateNamedPipeA( nInBufferSize: DWORD, nDefaultTimeOut: DWORD, lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, -) callconv(.Stdcall) HANDLE; +) callconv(WINAPI) HANDLE; pub extern "kernel32" fn CreateNamedPipeW( lpName: LPCWSTR, dwOpenMode: DWORD, @@ -36,7 +36,7 @@ pub extern "kernel32" fn CreateNamedPipeW( nInBufferSize: DWORD, nDefaultTimeOut: DWORD, lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, -) callconv(.Stdcall) HANDLE; +) callconv(WINAPI) HANDLE; pub extern "kernel32" fn CreateEventExW( lpEventAttributes: ?*SECURITY_ATTRIBUTES, @@ -63,7 +63,7 @@ pub extern "kernel32" fn CreateFileA( dwCreationDisposition: DWORD, dwFlagsAndAttributes: DWORD, hTemplateFile: ?HANDLE, -) callconv(.Stdcall) HANDLE; +) callconv(WINAPI) HANDLE; pub extern "kernel32" fn CreatePipe( hReadPipe: *HANDLE, From 717cf00fe0d68dc1213fb645b184afe1cbf52104 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 29 Dec 2020 11:13:00 -0700 Subject: [PATCH 6/6] std.ChildProcess: improvements to collectOutputPosix * read directly into the ArrayList buffers. * respect max_output_bytes * std.ArrayList: - make `allocatedSlice` public. - add `unusedCapacitySlice`. I removed the Windows implementation of this stuff; I am doing a partial merge of LemonBoy's patch with the understanding that a later patch can add the Windows implementation after it is vetted. --- lib/std/array_list.zig | 16 +++- lib/std/child_process.zig | 158 ++++++++------------------------ lib/std/os.zig | 3 +- lib/std/os/windows/bits.zig | 13 --- lib/std/os/windows/kernel32.zig | 33 ------- 5 files changed, 53 insertions(+), 170 deletions(-) diff --git a/lib/std/array_list.zig b/lib/std/array_list.zig index 4a8e0059e9..292e421bb1 100644 --- a/lib/std/array_list.zig +++ b/lib/std/array_list.zig @@ -337,11 +337,21 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type { return self.pop(); } - // For a nicer API, `items.len` is the length, not the capacity. - // This requires "unsafe" slicing. - fn allocatedSlice(self: Self) Slice { + /// Returns a slice of all the items plus the extra capacity, whose memory + /// contents are undefined. + pub fn allocatedSlice(self: Self) Slice { + // For a nicer API, `items.len` is the length, not the capacity. + // This requires "unsafe" slicing. return self.items.ptr[0..self.capacity]; } + + /// Returns a slice of only the extra capacity after items. + /// This can be useful for writing directly into an `ArrayList`. + /// Note that such an operation must be followed up with a direct + /// modification of `self.items.len`. + pub fn unusedCapacitySlice(self: Self) Slice { + return self.allocatedSlice()[self.items.len..]; + } }; } diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index e85ce83755..acab303f73 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -186,14 +186,22 @@ pub const ChildProcess = struct { pub const exec2 = @compileError("deprecated: exec2 is renamed to exec"); - fn collectOutputPosix(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void { + fn collectOutputPosix( + child: *const ChildProcess, + stdout: *std.ArrayList(u8), + stderr: *std.ArrayList(u8), + max_output_bytes: usize, + ) !void { var poll_fds = [_]os.pollfd{ .{ .fd = child.stdout.?.handle, .events = os.POLLIN, .revents = undefined }, .{ .fd = child.stderr.?.handle, .events = os.POLLIN, .revents = undefined }, }; var dead_fds: usize = 0; - var loop_buf: [4096]u8 = undefined; + // We ask for ensureCapacity with this much extra space. This has more of an + // effect on small reads because once the reads start to get larger the amount + // of space an ArrayList will allocate grows exponentially. + const bump_amt = 512; while (dead_fds < poll_fds.len) { const events = try os.poll(&poll_fds, std.math.maxInt(i32)); @@ -203,13 +211,17 @@ pub const ChildProcess = struct { // conditions. if (poll_fds[0].revents & os.POLLIN != 0) { // stdout is ready. - const n = try os.read(poll_fds[0].fd, &loop_buf); - try stdout.appendSlice(loop_buf[0..n]); + const new_capacity = std.math.min(stdout.items.len + bump_amt, max_output_bytes); + if (new_capacity == stdout.capacity) return error.StdoutStreamTooLong; + try stdout.ensureCapacity(new_capacity); + stdout.items.len += try os.read(poll_fds[0].fd, stdout.unusedCapacitySlice()); } if (poll_fds[1].revents & os.POLLIN != 0) { // stderr is ready. - const n = try os.read(poll_fds[1].fd, &loop_buf); - try stderr.appendSlice(loop_buf[0..n]); + const new_capacity = std.math.min(stderr.items.len + bump_amt, max_output_bytes); + if (new_capacity == stderr.capacity) return error.StderrStreamTooLong; + try stderr.ensureCapacity(new_capacity); + stderr.items.len += try os.read(poll_fds[1].fd, stderr.unusedCapacitySlice()); } // Exclude the fds that signaled an error. @@ -224,61 +236,6 @@ pub const ChildProcess = struct { } } - fn collectOutputWindows(child: *const ChildProcess, stdout: *std.ArrayList(u8), stderr: *std.ArrayList(u8), max_output_bytes: usize) !void { - var wait_objects = [_]windows.kernel32.HANDLE{ - child.stdout.?.handle, child.stderr.?.handle, - }; - var waiting_objects: u32 = wait_objects.len; - - // XXX: Calling zeroes([2]windows.OVERLAPPED) causes the stage1 compiler - // to crash and burn. - var overlapped = [_]windows.OVERLAPPED{ - mem.zeroes(windows.OVERLAPPED), - mem.zeroes(windows.OVERLAPPED), - }; - var temp_buf: [2][4096]u8 = undefined; - - // Kickstart the loop by issuing two async reads. - // ReadFile returns false and GetLastError returns ERROR_IO_PENDING if - // everything is ok. - _ = windows.kernel32.ReadFile(wait_objects[0], &temp_buf[0], temp_buf[0].len, null, &overlapped[0]); - _ = windows.kernel32.ReadFile(wait_objects[1], &temp_buf[1], temp_buf[1].len, null, &overlapped[1]); - - poll: while (waiting_objects > 0) { - const status = windows.kernel32.WaitForMultipleObjects(waiting_objects, &wait_objects, 0, windows.INFINITE); - switch (status) { - windows.WAIT_OBJECT_0 + 0...windows.WAIT_OBJECT_0 + 1 => { - // stdout (or stderr) is ready. - const object = status - windows.WAIT_OBJECT_0; - - var read_bytes: u32 = undefined; - if (windows.kernel32.GetOverlappedResult(wait_objects[object], &overlapped[object], &read_bytes, 0) == 0) { - switch (windows.kernel32.GetLastError()) { - .BROKEN_PIPE => { - // Move it to the end to remove it. - if (object != waiting_objects - 1) - mem.swap(windows.kernel32.HANDLE, &wait_objects[object], &wait_objects[waiting_objects - 1]); - waiting_objects -= 1; - continue :poll; - }, - else => |err| return windows.unexpectedError(err), - } - } - try stdout.appendSlice(temp_buf[object][0..read_bytes]); - _ = windows.kernel32.ReadFile(wait_objects[object], &temp_buf[object], temp_buf[object].len, null, &overlapped[object]); - }, - windows.WAIT_FAILED => { - switch (windows.kernel32.GetLastError()) { - else => |err| return windows.unexpectedError(err), - } - }, - // We're waiting with an infinite timeout - windows.WAIT_TIMEOUT => unreachable, - else => unreachable, - } - } - } - /// Spawns a child process, waits for it, collecting stdout and stderr, and then returns. /// If it succeeds, the caller owns result.stdout and result.stderr memory. pub fn exec(args: struct { @@ -303,16 +260,28 @@ pub const ChildProcess = struct { try child.spawn(); + // TODO collect output in a deadlock-avoiding way on Windows. + // https://github.com/ziglang/zig/issues/6343 + if (builtin.os.tag == .windows) { + const stdout_in = child.stdout.?.reader(); + const stderr_in = child.stderr.?.reader(); + + const stdout = try stdout_in.readAllAlloc(args.allocator, args.max_output_bytes); + errdefer args.allocator.free(stdout); + const stderr = try stderr_in.readAllAlloc(args.allocator, args.max_output_bytes); + errdefer args.allocator.free(stderr); + + return ExecResult{ + .term = try child.wait(), + .stdout = stdout, + .stderr = stderr, + }; + } + var stdout = std.ArrayList(u8).init(args.allocator); var stderr = std.ArrayList(u8).init(args.allocator); - // XXX: Respect max_output_bytes - // XXX: Smarter reading logic, read directly into the ArrayList - if (builtin.os.tag == .windows) { - try collectOutputWindows(child, &stdout, &stderr, args.max_output_bytes); - } else { - try collectOutputPosix(child, &stdout, &stderr, args.max_output_bytes); - } + try collectOutputPosix(child, &stdout, &stderr, args.max_output_bytes); return ExecResult{ .term = try child.wait(), @@ -650,7 +619,7 @@ pub const ChildProcess = struct { var g_hChildStd_OUT_Wr: ?windows.HANDLE = null; switch (self.stdout_behavior) { StdIo.Pipe => { - try windowsMakePipe(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr); + try windowsMakePipeOut(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr); }, StdIo.Ignore => { g_hChildStd_OUT_Wr = nul_handle; @@ -670,7 +639,7 @@ pub const ChildProcess = struct { var g_hChildStd_ERR_Wr: ?windows.HANDLE = null; switch (self.stderr_behavior) { StdIo.Pipe => { - try windowsMakePipe(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr); + try windowsMakePipeOut(&g_hChildStd_ERR_Rd, &g_hChildStd_ERR_Wr, &saAttr); }, StdIo.Ignore => { g_hChildStd_ERR_Wr = nul_handle; @@ -903,55 +872,6 @@ fn windowsDestroyPipe(rd: ?windows.HANDLE, wr: ?windows.HANDLE) void { if (wr) |h| os.close(h); } -var pipe_name_counter = std.atomic.Int(u32).init(1); - -fn windowsMakePipe(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void { - var tmp_buf: [128]u8 = undefined; - // Forge a random path for the pipe. - const pipe_path = std.fmt.bufPrintZ( - &tmp_buf, - "\\\\.\\pipe\\zig-childprocess-{d}-{d}", - .{ windows.kernel32.GetCurrentProcessId(), pipe_name_counter.fetchAdd(1) }, - ) catch unreachable; - - // Create the read handle that can be used with overlapped IO ops. - const read_handle = windows.kernel32.CreateNamedPipeA( - pipe_path, - windows.PIPE_ACCESS_INBOUND | windows.FILE_FLAG_OVERLAPPED, - windows.PIPE_TYPE_BYTE, - 1, - 0x1000, - 0x1000, - 0, - sattr, - ); - if (read_handle == windows.INVALID_HANDLE_VALUE) { - switch (windows.kernel32.GetLastError()) { - else => |err| return windows.unexpectedError(err), - } - } - - const write_handle = windows.kernel32.CreateFileA( - pipe_path, - windows.GENERIC_WRITE, - 0, - sattr, - windows.OPEN_EXISTING, - windows.FILE_ATTRIBUTE_NORMAL, - null, - ); - if (write_handle == windows.INVALID_HANDLE_VALUE) { - switch (windows.kernel32.GetLastError()) { - else => |err| return windows.unexpectedError(err), - } - } - - try windows.SetHandleInformation(read_handle, windows.HANDLE_FLAG_INHERIT, 0); - - rd.* = read_handle; - wr.* = write_handle; -} - fn windowsMakePipeIn(rd: *?windows.HANDLE, wr: *?windows.HANDLE, sattr: *const windows.SECURITY_ATTRIBUTES) !void { var rd_h: windows.HANDLE = undefined; var wr_h: windows.HANDLE = undefined; diff --git a/lib/std/os.zig b/lib/std/os.zig index 88f6292629..66f65b5b5d 100644 --- a/lib/std/os.zig +++ b/lib/std/os.zig @@ -5269,8 +5269,7 @@ pub const PollError = error{ pub fn poll(fds: []pollfd, timeout: i32) PollError!usize { while (true) { - const fds_count = math.cast(nfds_t, fds.len) catch - return error.SystemResources; + const fds_count = math.cast(nfds_t, fds.len) catch return error.SystemResources; const rc = system.poll(fds.ptr, fds_count, timeout); if (builtin.os.tag == .windows) { if (rc == windows.ws2_32.SOCKET_ERROR) { diff --git a/lib/std/os/windows/bits.zig b/lib/std/os/windows/bits.zig index 0d69087a46..f5d520c580 100644 --- a/lib/std/os/windows/bits.zig +++ b/lib/std/os/windows/bits.zig @@ -438,19 +438,6 @@ pub const SECURITY_ATTRIBUTES = extern struct { pub const PSECURITY_ATTRIBUTES = *SECURITY_ATTRIBUTES; pub const LPSECURITY_ATTRIBUTES = *SECURITY_ATTRIBUTES; -pub const PIPE_ACCESS_INBOUND = 0x00000001; -pub const PIPE_ACCESS_OUTBOUND = 0x00000002; -pub const PIPE_ACCESS_DUPLEX = 0x00000003; - -pub const PIPE_TYPE_BYTE = 0x00000000; -pub const PIPE_TYPE_MESSAGE = 0x00000004; - -pub const PIPE_READMODE_BYTE = 0x00000000; -pub const PIPE_READMODE_MESSAGE = 0x00000002; - -pub const PIPE_WAIT = 0x00000000; -pub const PIPE_NOWAIT = 0x00000001; - pub const GENERIC_READ = 0x80000000; pub const GENERIC_WRITE = 0x40000000; pub const GENERIC_EXECUTE = 0x20000000; diff --git a/lib/std/os/windows/kernel32.zig b/lib/std/os/windows/kernel32.zig index d7f2655687..444234876c 100644 --- a/lib/std/os/windows/kernel32.zig +++ b/lib/std/os/windows/kernel32.zig @@ -15,29 +15,6 @@ pub extern "kernel32" fn CloseHandle(hObject: HANDLE) callconv(WINAPI) BOOL; pub extern "kernel32" fn CreateDirectoryW(lpPathName: [*:0]const u16, lpSecurityAttributes: ?*SECURITY_ATTRIBUTES) callconv(WINAPI) BOOL; pub extern "kernel32" fn SetEndOfFile(hFile: HANDLE) callconv(WINAPI) BOOL; -pub extern "kernel32" fn GetCurrentProcessId() callconv(WINAPI) DWORD; - -pub extern "kernel32" fn CreateNamedPipeA( - lpName: [*:0]const u8, - dwOpenMode: DWORD, - dwPipeMode: DWORD, - nMaxInstances: DWORD, - nOutBufferSize: DWORD, - nInBufferSize: DWORD, - nDefaultTimeOut: DWORD, - lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, -) callconv(WINAPI) HANDLE; -pub extern "kernel32" fn CreateNamedPipeW( - lpName: LPCWSTR, - dwOpenMode: DWORD, - dwPipeMode: DWORD, - nMaxInstances: DWORD, - nOutBufferSize: DWORD, - nInBufferSize: DWORD, - nDefaultTimeOut: DWORD, - lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, -) callconv(WINAPI) HANDLE; - pub extern "kernel32" fn CreateEventExW( lpEventAttributes: ?*SECURITY_ATTRIBUTES, lpName: [*:0]const u16, @@ -55,16 +32,6 @@ pub extern "kernel32" fn CreateFileW( hTemplateFile: ?HANDLE, ) callconv(WINAPI) HANDLE; -pub extern "kernel32" fn CreateFileA( - lpFileName: [*:0]const u8, - dwDesiredAccess: DWORD, - dwShareMode: DWORD, - lpSecurityAttributes: ?*const SECURITY_ATTRIBUTES, - dwCreationDisposition: DWORD, - dwFlagsAndAttributes: DWORD, - hTemplateFile: ?HANDLE, -) callconv(WINAPI) HANDLE; - pub extern "kernel32" fn CreatePipe( hReadPipe: *HANDLE, hWritePipe: *HANDLE,