From 9fe88441f58bdc00cc7ac040ea755dfbcc7beb65 Mon Sep 17 00:00:00 2001 From: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com> Date: Thu, 7 Mar 2024 02:35:11 -0800 Subject: [PATCH] Avoid sigpipe + optimize + delete dead code --- src/bun.js/api/bun/process.zig | 51 ++++++++++++++++++++++++++----- src/bun.js/api/bun/subprocess.zig | 11 +++++-- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/src/bun.js/api/bun/process.zig b/src/bun.js/api/bun/process.zig index 17db21b198..6995441cf8 100644 --- a/src/bun.js/api/bun/process.zig +++ b/src/bun.js/api/bun/process.zig @@ -1128,7 +1128,7 @@ pub fn spawnProcessPosix( const stdios: [3]*?bun.FileDescriptor = .{ &spawned.stdin, &spawned.stdout, &spawned.stderr }; var dup_stdout_to_stderr: bool = false; - var stderr_write_end: ?bun.FileDescriptor = null; + for (0..3) |i| { const stdio = stdios[i]; const fileno = bun.toFD(i); @@ -1164,12 +1164,48 @@ pub fn spawnProcessPosix( return error.SystemResources; } - const before = std.c.fcntl(fds_[if (i == 0) 1 else 0], std.os.F.GETFL); + var before = std.c.fcntl(fds_[if (i == 0) 1 else 0], std.os.F.GETFL); + _ = std.c.fcntl(fds_[if (i == 0) 1 else 0], std.os.F.SETFL, before | bun.C.FD_CLOEXEC); + if (comptime Environment.isMac) { + // SO_NOSIGPIPE + before = 1; + _ = std.c.setsockopt(fds_[if (i == 0) 1 else 0], std.os.SOL.SOCKET, std.os.SO.NOSIGPIPE, &before, @sizeOf(c_int)); + } + break :brk .{ bun.toFD(fds_[if (i == 0) 1 else 0]), bun.toFD(fds_[if (i == 0) 0 else 1]) }; }; + if (i == 0) { + // their copy of stdin should be readable + _ = std.c.shutdown(@intCast(fds[1].cast()), std.os.SHUT.WR); + + // our copy of stdin should be writable + _ = std.c.shutdown(@intCast(fds[0].cast()), std.os.SHUT.RD); + + if (comptime Environment.isMac) { + // macOS seems to default to around 8 KB for the buffer size + // this is comically small. + const so_recvbuf: c_int = 1024 * 512; + const so_sendbuf: c_int = 1024 * 512; + _ = std.c.setsockopt(fds[1].cast(), std.os.SOL.SOCKET, std.os.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int)); + _ = std.c.setsockopt(fds[0].cast(), std.os.SOL.SOCKET, std.os.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int)); + } + } else { + + // their copy of stdout or stderr should be writable + _ = std.c.shutdown(@intCast(fds[1].cast()), std.os.SHUT.RD); + + // our copy of stdout or stderr should be readable + _ = std.c.shutdown(@intCast(fds[0].cast()), std.os.SHUT.WR); + + const so_recvbuf: c_int = 1024 * 512; + const so_sendbuf: c_int = 1024 * 512; + _ = std.c.setsockopt(fds[0].cast(), std.os.SOL.SOCKET, std.os.SO.RCVBUF, &so_recvbuf, @sizeOf(c_int)); + _ = std.c.setsockopt(fds[1].cast(), std.os.SOL.SOCKET, std.os.SO.SNDBUF, &so_sendbuf, @sizeOf(c_int)); + } + try to_close_at_end.append(fds[1]); try to_close_on_error.append(fds[0]); @@ -1177,9 +1213,6 @@ pub fn spawnProcessPosix( try actions.close(fds[1]); stdio.* = fds[0]; - if (i == 2) { - stderr_write_end = fds[1]; - } }, .pipe => |fd| { try actions.dup2(fd, fileno); @@ -1189,7 +1222,6 @@ pub fn spawnProcessPosix( } if (dup_stdout_to_stderr) { - // try actions.dup2(stderr_write_end.?, stdio_options[1].dup2.out.toFd()); try actions.dup2(stdio_options[1].dup2.to.toFd(), stdio_options[1].dup2.out.toFd()); } @@ -1217,10 +1249,15 @@ pub fn spawnProcessPosix( } // enable non-block - const before = std.c.fcntl(fds_[0], std.os.F.GETFL); + var before = std.c.fcntl(fds_[0], std.os.F.GETFL); _ = std.c.fcntl(fds_[0], std.os.F.SETFL, before | std.os.O.NONBLOCK | bun.C.FD_CLOEXEC); + if (comptime Environment.isMac) { + // SO_NOSIGPIPE + _ = std.c.setsockopt(fds_[if (i == 0) 1 else 0], std.os.SOL.SOCKET, std.os.SO.NOSIGPIPE, &before, @sizeOf(c_int)); + } + break :brk .{ bun.toFD(fds_[0]), bun.toFD(fds_[1]) }; }; diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index 32ce3778ec..051067d17a 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -234,6 +234,11 @@ pub const Subprocess = struct { return true; } + // TODO: investigate further if we can free the Subprocess before the process has exited. + if (!this.process.hasExited()) { + return true; + } + if (comptime Environment.isWindows) { if (this.process.poller == .uv) { if (this.process.poller.uv.isActive()) { @@ -336,7 +341,7 @@ pub const Subprocess = struct { } /// This disables the keeping process alive flag on the poll and also in the stdin, stdout, and stderr - pub fn unref(this: *Subprocess, comptime _: bool) void { + pub fn unref(this: *Subprocess) void { this.process.disableKeepingEventLoopAlive(); if (!this.hasCalledGetter(.stdin)) { @@ -613,7 +618,7 @@ pub const Subprocess = struct { } pub fn doUnref(this: *Subprocess, _: *JSC.JSGlobalObject, _: *JSC.CallFrame) callconv(.C) JSValue { - this.unref(false); + this.unref(); return JSC.JSValue.jsUndefined(); } @@ -905,8 +910,8 @@ pub const Subprocess = struct { .result => { if (comptime Environment.isPosix) { const poll = this.reader.handle.poll; - poll.flags.insert(.nonblocking); poll.flags.insert(.socket); + this.reader.flags.socket = true; } return .{ .result = {} };