From 53d8a25dab5ddcea16ac70cdcdf28cb3e4944cbb Mon Sep 17 00:00:00 2001 From: Jonathan Marler Date: Sat, 5 Feb 2022 18:59:09 -0700 Subject: [PATCH 1/4] child_process: collectOutputWindows handle broken_pipe from ReadFile This was found on a user's machine when calling "git" as a child process from msys. Instead of getting BROKEN_PIPE on GetOverlappedREsult, it would occur on ReadFile which would then cause the function to hang because the async operation was never started. --- lib/std/child_process.zig | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 7808dcd1e5..3e12312d5d 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -277,10 +277,19 @@ pub const ChildProcess = struct { const new_capacity = std.math.min(outs[i].items.len + bump_amt, max_output_bytes); try outs[i].ensureTotalCapacity(new_capacity); const buf = outs[i].unusedCapacitySlice(); - _ = windows.kernel32.ReadFile(handles[i], buf.ptr, math.cast(u32, buf.len) catch maxInt(u32), null, &overlapped[i]); - wait_objects[wait_object_count] = handles[i]; - wait_object_count += 1; + const read_result = windows.kernel32.ReadFile(handles[i], buf.ptr, math.cast(u32, buf.len) catch maxInt(u32), null, &overlapped[i]); + std.debug.assert(read_result == 0); + switch (windows.kernel32.GetLastError()) { + .IO_PENDING => { + wait_objects[wait_object_count] = handles[i]; + wait_object_count += 1; + }, + .BROKEN_PIPE => {}, // don't add to the wait_objects list + else => |err| return windows.unexpectedError(err), + } } + if (wait_object_count == 0) + return; while (true) { const status = windows.kernel32.WaitForMultipleObjects(wait_object_count, &wait_objects, 0, windows.INFINITE); @@ -320,9 +329,16 @@ pub const ChildProcess = struct { try outs[i].ensureTotalCapacity(new_capacity); const buf = outs[i].unusedCapacitySlice(); if (buf.len == 0) return if (i == 0) error.StdoutStreamTooLong else error.StderrStreamTooLong; - _ = windows.kernel32.ReadFile(handles[i], buf.ptr, math.cast(u32, buf.len) catch maxInt(u32), null, &overlapped[i]); - wait_objects[wait_object_count] = handles[i]; - wait_object_count += 1; + const read_result = windows.kernel32.ReadFile(handles[i], buf.ptr, math.cast(u32, buf.len) catch maxInt(u32), null, &overlapped[i]); + std.debug.assert(read_result == 0); + switch (windows.kernel32.GetLastError()) { + .IO_PENDING => { + wait_objects[wait_object_count] = handles[i]; + wait_object_count += 1; + }, + .BROKEN_PIPE => {}, // don't add to the wait_objects list + else => |err| return windows.unexpectedError(err), + } } } From 8f830207c47987de9767130d54abff79c8ec257d Mon Sep 17 00:00:00 2001 From: Jonathan Marler Date: Sun, 6 Feb 2022 06:05:40 -0700 Subject: [PATCH 2/4] fix bug I think I found while manually reviewing --- lib/std/child_process.zig | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 3e12312d5d..4ccfcb4029 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -288,10 +288,8 @@ pub const ChildProcess = struct { else => |err| return windows.unexpectedError(err), } } - if (wait_object_count == 0) - return; - while (true) { + while (wait_object_count > 0) { const status = windows.kernel32.WaitForMultipleObjects(wait_object_count, &wait_objects, 0, windows.INFINITE); if (status == windows.WAIT_FAILED) { switch (windows.kernel32.GetLastError()) { @@ -315,11 +313,7 @@ pub const ChildProcess = struct { var read_bytes: u32 = undefined; if (windows.kernel32.GetOverlappedResult(handles[i], &overlapped[i], &read_bytes, 0) == 0) { switch (windows.kernel32.GetLastError()) { - .BROKEN_PIPE => { - if (wait_object_count == 0) - break; - continue; - }, + .BROKEN_PIPE => continue, else => |err| return windows.unexpectedError(err), } } From 4fddb591e2377186ef942084cae2e93d5de448b3 Mon Sep 17 00:00:00 2001 From: Jonathan Marler Date: Sun, 6 Feb 2022 15:20:15 -0700 Subject: [PATCH 3/4] rework to allow ReadFile to complete synchronously --- lib/std/child_process.zig | 55 +++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 4ccfcb4029..10aeacf755 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -252,6 +252,33 @@ pub const ChildProcess = struct { } } + const WindowsAsyncReadResult = enum { + pending, + closed, + full, + }; + + fn windowsAsyncRead( + handle: windows.HANDLE, + overlapped: *windows.OVERLAPPED, + buf: *std.ArrayList(u8), + bump_amt: usize, + max_output_bytes: usize, + ) !WindowsAsyncReadResult { + while (true) { + const new_capacity = std.math.min(buf.items.len + bump_amt, max_output_bytes); + try buf.ensureTotalCapacity(new_capacity); + const next_buf = buf.unusedCapacitySlice(); + if (next_buf.len == 0) return .full; + const read_result = windows.kernel32.ReadFile(handle, next_buf.ptr, math.cast(u32, next_buf.len) catch maxInt(u32), null, overlapped); + if (read_result == 0) return switch (windows.kernel32.GetLastError()) { + .IO_PENDING => .pending, + .BROKEN_PIPE => .closed, + else => |err| windows.unexpectedError(err), + }; + } + } + fn collectOutputWindows(child: *const ChildProcess, outs: [2]*std.ArrayList(u8), max_output_bytes: usize) !void { const bump_amt = 512; const handles = [_]windows.HANDLE{ @@ -274,18 +301,13 @@ pub const ChildProcess = struct { // Windows Async IO requires an initial call to ReadFile before waiting on the handle for ([_]u1{ 0, 1 }) |i| { - const new_capacity = std.math.min(outs[i].items.len + bump_amt, max_output_bytes); - try outs[i].ensureTotalCapacity(new_capacity); - const buf = outs[i].unusedCapacitySlice(); - const read_result = windows.kernel32.ReadFile(handles[i], buf.ptr, math.cast(u32, buf.len) catch maxInt(u32), null, &overlapped[i]); - std.debug.assert(read_result == 0); - switch (windows.kernel32.GetLastError()) { - .IO_PENDING => { + switch (try windowsAsyncRead(handles[i], &overlapped[i], outs[i], bump_amt, max_output_bytes)) { + .pending => { wait_objects[wait_object_count] = handles[i]; wait_object_count += 1; }, - .BROKEN_PIPE => {}, // don't add to the wait_objects list - else => |err| return windows.unexpectedError(err), + .closed => {}, // don't add to the wait_objects list + .full => return if (i == 0) error.StdoutStreamTooLong else error.StderrStreamTooLong, } } @@ -319,19 +341,14 @@ pub const ChildProcess = struct { } outs[i].items.len += read_bytes; - const new_capacity = std.math.min(outs[i].items.len + bump_amt, max_output_bytes); - try outs[i].ensureTotalCapacity(new_capacity); - const buf = outs[i].unusedCapacitySlice(); - if (buf.len == 0) return if (i == 0) error.StdoutStreamTooLong else error.StderrStreamTooLong; - const read_result = windows.kernel32.ReadFile(handles[i], buf.ptr, math.cast(u32, buf.len) catch maxInt(u32), null, &overlapped[i]); - std.debug.assert(read_result == 0); - switch (windows.kernel32.GetLastError()) { - .IO_PENDING => { + + switch (try windowsAsyncRead(handles[i], &overlapped[i], outs[i], bump_amt, max_output_bytes)) { + .pending => { wait_objects[wait_object_count] = handles[i]; wait_object_count += 1; }, - .BROKEN_PIPE => {}, // don't add to the wait_objects list - else => |err| return windows.unexpectedError(err), + .closed => {}, // don't add to the wait_objects list + .full => return if (i == 0) error.StdoutStreamTooLong else error.StderrStreamTooLong, } } } From 2cc33367ebee202462d6bef7f07120dd38ff6912 Mon Sep 17 00:00:00 2001 From: Jonathan Marler Date: Mon, 7 Feb 2022 01:49:15 -0700 Subject: [PATCH 4/4] fix bug when ReadFile returns synchronously in collectOutputWindows --- lib/std/child_process.zig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index 10aeacf755..ace58ecf4f 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -270,12 +270,14 @@ pub const ChildProcess = struct { try buf.ensureTotalCapacity(new_capacity); const next_buf = buf.unusedCapacitySlice(); if (next_buf.len == 0) return .full; - const read_result = windows.kernel32.ReadFile(handle, next_buf.ptr, math.cast(u32, next_buf.len) catch maxInt(u32), null, overlapped); + var read_bytes: u32 = undefined; + const read_result = windows.kernel32.ReadFile(handle, next_buf.ptr, math.cast(u32, next_buf.len) catch maxInt(u32), &read_bytes, overlapped); if (read_result == 0) return switch (windows.kernel32.GetLastError()) { .IO_PENDING => .pending, .BROKEN_PIPE => .closed, else => |err| windows.unexpectedError(err), }; + buf.items.len += read_bytes; } }