Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce std.io.poll #14744

Merged
merged 8 commits into from
Mar 1, 2023
Merged

introduce std.io.poll #14744

merged 8 commits into from
Mar 1, 2023

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Feb 28, 2023

Motivation

I need the logic from std.ChildProcess.collectOutput for some other work I'm doing in #14647. This is my attempt to extract it into a reusable abstraction. In short summary, the usage looks like this:

    // ...

    try child.spawn();

    var poller = std.io.poll(gpa, enum { stdout, stderr }, .{
        .stdout = child.stdout.?,
        .stderr = child.stderr.?,
    });
    defer poller.deinit();

    while (!poller.done()) try poller.poll();

    // ...

Each stream gets a std.fifo.LinearFifo which can be accessed by e.g. poller.fifo(.stdout). You can do this inside the while loop, or you can just let it accumulate as I have done above.

Testing

Here is the code I used to test this. Before merging this PR I will break it up into parent.zig and child.zig and move it to be a standalone test.

zig build-exe test.zig
./test
const std = @import("std");

pub fn main() !void {
    var general_purpose_allocator: std.heap.GeneralPurposeAllocator(.{}) = .{};
    defer if (general_purpose_allocator.deinit()) std.process.exit(1);
    const gpa = general_purpose_allocator.allocator();

    var arena_instance = std.heap.ArenaAllocator.init(gpa);
    defer arena_instance.deinit();
    const arena = arena_instance.allocator();

    const args = try std.process.argsAlloc(arena);

    if (args.len >= 2) {
        return childMain();
    } else {
        return parent(gpa, arena);
    }
}

fn parent(gpa: std.mem.Allocator, arena: std.mem.Allocator) !void {
    const self_exe_path = try std.fs.selfExePathAlloc(arena);
    var child = std.ChildProcess.init(&.{ self_exe_path, "child" }, gpa);

    child.stdin_behavior = .Pipe;
    child.stdout_behavior = .Pipe;
    child.stderr_behavior = .Pipe;

    try child.spawn();

    var poller = std.io.poll(gpa, enum { stdout, stderr }, .{
        .stdout = child.stdout.?,
        .stderr = child.stderr.?,
    });
    defer poller.deinit();

    try child.stdin.?.writeAll("the input");
    child.stdin.?.close(); // send EOF
    child.stdin = null;

    while (!poller.done()) try poller.poll();

    const term = try child.wait();
    switch (term) {
        .Exited => |code| {
            if (code != 0) @panic("bad child exit code");
        },
        else => @panic("child crash"),
    }

    const stdout = poller.fifo(.stdout).readableSlice(0);
    const stderr = poller.fifo(.stderr).readableSlice(0);

    for (0..10000) |i| {
        if (!std.mem.eql(u8, stderr[i * "Garbage".len ..][0.."Garbage".len], "Garbage")) {
            @panic("Garbage failure");
        }
        if (!std.mem.eql(u8, stdout[i * "Trash".len ..][0.."Trash".len], "Trash")) {
            @panic("Trash failure");
        }
    }
}

fn childMain() !void {
    const stdout = std.io.getStdOut();
    const stderr = std.io.getStdErr();
    const stdin = std.io.getStdIn();

    for (0..10000) |_| {
        try stderr.writeAll("Garbage");
        try stdout.writeAll("Trash");
    }

    var buf: [1000]u8 = undefined;
    const amt = try stdin.readAll(&buf);
    if (!std.mem.eql(u8, buf[0..amt], "the input")) {
        @panic("test failure");
    }
}

Merge Checklist

  • Add the Windows implementation using overlapped I/O
  • Rework std.ChildProcess.collectOutput to use this abstraction. Most of that code can be deleted in theory, just need to add logic to limit to max_output_bytes.
  • Rework the above test file into a proper test case, probably a standalone one.

It's not OK to half-ass this function. Please implement it correctly, or
not at all.
I think having inputs is problematic here, it should only be for
outputs.
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library. labels Feb 28, 2023
@andrewrk andrewrk requested a review from Hejsil February 28, 2023 05:49
@andrewrk andrewrk mentioned this pull request Feb 28, 2023
52 tasks
@uael
Copy link

uael commented Feb 28, 2023

You can consider using select instead, it is available on Macos, Linux and Windows

@ifreund
Copy link
Member

ifreund commented Feb 28, 2023

You can consider using select instead, it is available on Macos, Linux and Windows

No, nobody should use select for anything. See the warning right at the top of the linux man page:

   WARNING: select() can monitor only file descriptors numbers that
   are less than FD_SETSIZE (1024)—an unreasonably low limit for
   many modern applications—and this limitation will not change.
   All modern applications should instead use poll(2) or epoll(7),
   which do not suffer this limitation.

@marler8997
Copy link
Contributor

Just FYI, Windows uses a different ABI for fd_set which doesn't have this limit, however, it only works with sockets so not useful here.

@matu3ba
Copy link
Contributor

matu3ba commented Feb 28, 2023

overlapped I/O

That sounds pretty much like https://github.com/ziglang/zig/pull/14152/files#diff-e0931af874560c650fde2b1a0f92b99f2d32edb04a823cf99b6161f69f5997eaR1552-R1562 except for a socket instead of pipe.
Bear in mind that stdin can not be async without hacks and that it can be annoying for the child not introspect its calling arguments, which is why I have created the linked PR.

@marler8997
Copy link
Contributor

Here's the initial windows implementation: marler8997@82ff9da

@andrewrk
Copy link
Member Author

andrewrk commented Mar 1, 2023

Would be nice to have the additional test, but I think this is safe to merge given how much test coverage it gets from the build system. Thanks @marler8997!

@andrewrk andrewrk enabled auto-merge March 1, 2023 20:28
@andrewrk andrewrk merged commit 874d3a1 into master Mar 1, 2023
@andrewrk andrewrk deleted the std.io.poll branch March 1, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants