ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
34.04k stars 2.49k forks source link

ChildProcess design flaw: impossible to concurrently read a piped stream and kill the child #16820

Open matklad opened 1 year ago

matklad commented 1 year ago

ChildProcess.kill not only kills the process, but also helpfully waits for it and closes the streams:

https://github.com/ziglang/zig/blob/197d9a9eb3557b1feae7580a82ab60defc9e12a1/lib/std/child_process.zig#L234-L236 https://github.com/ziglang/zig/blob/197d9a9eb3557b1feae7580a82ab60defc9e12a1/lib/std/child_process.zig#L386 https://github.com/ziglang/zig/blob/197d9a9eb3557b1feae7580a82ab60defc9e12a1/lib/std/child_process.zig#L420-L423

Unfortunately, this DWIM behavior makes some useful programs impossible (as far as I am aware, but maybe I am missing something).

Consider a program which spawns a child with stderr=.Pipe, then spawns a thread to read out stderr, and then, some time later, decides to kill the child on the main thread.

When one thread kills the child, the other is using the stderr file descriptor. As killing also waits and closes the stderr file descriptor, the reading thread might end up operating on an invalid FD!. As far as I am aware, a correct solution here would be an intricate dance of:

I think this can be worked-around by duping the relevant FD, but that's an extra FD!

matklad commented 1 year ago

Luckily, this seems easy to work-arond, by killing the child manually:

pub fn deinit(tb: *TmpTigerBeetle, gpa: std.mem.Allocator) void {
    // Signal to the `stderr_reader_thread` that it can exit
    // TODO(Zig) https://github.com/ziglang/zig/issues/16820
    if (builtin.os.tag == .windows) {
        std.os.windows.TerminateProcess(tb.process.handle, 1) catch {};
    } else {
        std.os.kill(tb.process.pid, std.os.SIG.TERM) catch {};
    }

    tb.stderr_reader_thread.join();
    tb.stderr_reader.destroy(gpa);
    _ = tb.process.wait() catch unreachable;
    tb.tmp_dir.cleanup();
}