From 69f97cecf087fd428c91d2f905c985e405f60114 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Fri, 6 Sep 2024 16:28:50 -0700 Subject: [PATCH] Ensure shell keeps process alive while running (#13777) --- src/shell/interpreter.zig | 76 ++++++++++++++++------- test/cli/run/shell-keepalive-fixture-1.js | 8 +++ test/cli/run/shell-keepalive-fixture-2.js | 6 ++ test/cli/run/shell-keepalive.test.ts | 11 ++++ 4 files changed, 77 insertions(+), 24 deletions(-) create mode 100644 test/cli/run/shell-keepalive-fixture-1.js create mode 100644 test/cli/run/shell-keepalive-fixture-2.js create mode 100644 test/cli/run/shell-keepalive.test.ts diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index 5b2f33b630..c8210534df 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -863,8 +863,10 @@ pub const Interpreter = struct { root_shell: ShellState, root_io: IO, - has_pending_activity: std.atomic.Value(usize) = std.atomic.Value(usize).init(0), + has_pending_activity: std.atomic.Value(u32) = std.atomic.Value(u32).init(0), started: std.atomic.Value(bool) = std.atomic.Value(bool).init(false), + // Necessary for builtin commands. + keep_alive: bun.Async.KeepAlive = .{}, vm_args_utf8: std.ArrayList(JSC.ZigString.Slice), async_commands_executing: u32 = 0, @@ -1252,19 +1254,29 @@ pub const Interpreter = struct { if (cwd) |*cc| cc.deref(); shargs.deinit(); throwShellErr(e, .{ .js = globalThis.bunVM().event_loop }); - return .undefined; + return .zero; }, }; + if (globalThis.hasException()) { + jsobjs.deinit(); + if (export_env) |*ee| ee.deinit(); + if (cwd) |*cc| cc.deref(); + shargs.deinit(); + interpreter.finalize(); + return .zero; + } + interpreter.flags.quiet = quiet; - interpreter.globalThis = globalThis; - interpreter.this_jsvalue = JSC.Codegen.JSShellInterpreter.toJS(interpreter, globalThis); - JSC.Codegen.JSShellInterpreter.resolveSetCached(interpreter.this_jsvalue, globalThis, resolve); - JSC.Codegen.JSShellInterpreter.rejectSetCached(interpreter.this_jsvalue, globalThis, reject); + const js_value = JSC.Codegen.JSShellInterpreter.toJS(interpreter, globalThis); + interpreter.this_jsvalue = js_value; + JSC.Codegen.JSShellInterpreter.resolveSetCached(js_value, globalThis, resolve); + JSC.Codegen.JSShellInterpreter.rejectSetCached(js_value, globalThis, reject); + interpreter.keep_alive.ref(globalThis.bunVM()); bun.Analytics.Features.shell += 1; - return interpreter.this_jsvalue; + return js_value; } pub fn parse( @@ -1648,7 +1660,7 @@ pub const Interpreter = struct { return .undefined; } - fn ioToJSValue(this: *ThisInterpreter, buf: *bun.ByteList) JSValue { + fn ioToJSValue(globalThis: *JSGlobalObject, buf: *bun.ByteList) JSValue { const bytelist = buf.*; buf.* = .{}; const value = JSC.MarkedArrayBuffer.toNodeBuffer( @@ -1656,7 +1668,7 @@ pub const Interpreter = struct { .allocator = bun.default_allocator, .buffer = JSC.ArrayBuffer.fromBytes(@constCast(bytelist.slice()), .Uint8Array), }, - this.event_loop.js.global, + globalThis, ); return value; @@ -1690,10 +1702,17 @@ pub const Interpreter = struct { defer this.deinitAfterJSRun(); this.exit_code = exit_code; if (this.this_jsvalue != .zero) { - if (JSC.Codegen.JSShellInterpreter.resolveGetCached(this.this_jsvalue)) |resolve| { - _ = resolve.call(this.globalThis, .undefined, &.{ JSValue.jsNumberFromU16(exit_code), this.getBufferedStdout(), this.getBufferedStderr() }); - JSC.Codegen.JSShellInterpreter.resolveSetCached(this.this_jsvalue, this.globalThis, .undefined); - JSC.Codegen.JSShellInterpreter.rejectSetCached(this.this_jsvalue, this.globalThis, .undefined); + const this_jsvalue = this.this_jsvalue; + if (JSC.Codegen.JSShellInterpreter.resolveGetCached(this_jsvalue)) |resolve| { + this.this_jsvalue = .zero; + const globalThis = this.globalThis; + const loop = this.event_loop.js; + this.keep_alive.disable(); + loop.enter(); + _ = resolve.call(globalThis, .undefined, &.{ JSValue.jsNumberFromU16(exit_code), this.getBufferedStdout(globalThis), this.getBufferedStderr(globalThis) }); + JSC.Codegen.JSShellInterpreter.resolveSetCached(this_jsvalue, globalThis, .undefined); + JSC.Codegen.JSShellInterpreter.rejectSetCached(this_jsvalue, globalThis, .undefined); + loop.exit(); } } } else { @@ -1707,12 +1726,20 @@ pub const Interpreter = struct { defer decrPendingActivityFlag(&this.has_pending_activity); if (this.event_loop == .js) { - if (this.this_jsvalue != .zero) { - if (JSC.Codegen.JSShellInterpreter.rejectGetCached(this.this_jsvalue)) |reject| { - reject.call(this.globalThis, &[_]JSValue{ JSValue.jsNumberFromChar(1), this.getBufferedStdout(), this.getBufferedStderr() }); - JSC.Codegen.JSShellInterpreter.resolveSetCached(this.this_jsvalue, this.globalThis, .undefined); - JSC.Codegen.JSShellInterpreter.rejectSetCached(this.this_jsvalue, this.globalThis, .undefined); + const this_jsvalue = this.this_jsvalue; + if (this_jsvalue != .zero) { + if (JSC.Codegen.JSShellInterpreter.rejectGetCached(this_jsvalue)) |reject| { + const loop = this.event_loop.js; + const globalThis = this.globalThis; this.this_jsvalue = .zero; + this.keep_alive.disable(); + + loop.enter(); + reject.call(globalThis, &[_]JSValue{ JSValue.jsNumberFromChar(1), this.getBufferedStdout(globalThis), this.getBufferedStderr(globalThis) }); + JSC.Codegen.JSShellInterpreter.resolveSetCached(this_jsvalue, globalThis, .undefined); + JSC.Codegen.JSShellInterpreter.rejectSetCached(this_jsvalue, globalThis, .undefined); + + loop.exit(); } } } @@ -1724,6 +1751,7 @@ pub const Interpreter = struct { jsobj.unprotect(); } this.root_io.deref(); + this.keep_alive.disable(); this.root_shell.deinitImpl(false, false); this.this_jsvalue = .zero; } @@ -1837,16 +1865,16 @@ pub const Interpreter = struct { pub fn getBufferedStdout( this: *ThisInterpreter, + globalThis: *JSGlobalObject, ) JSC.JSValue { - const stdout = this.ioToJSValue(this.root_shell.buffered_stdout()); - return stdout; + return ioToJSValue(globalThis, this.root_shell.buffered_stdout()); } pub fn getBufferedStderr( this: *ThisInterpreter, + globalThis: *JSGlobalObject, ) JSC.JSValue { - const stdout = this.ioToJSValue(this.root_shell.buffered_stderr()); - return stdout; + return ioToJSValue(globalThis, this.root_shell.buffered_stderr()); } pub fn finalize( @@ -1861,13 +1889,13 @@ pub const Interpreter = struct { return this.has_pending_activity.load(.seq_cst) > 0; } - fn incrPendingActivityFlag(has_pending_activity: *std.atomic.Value(usize)) void { + fn incrPendingActivityFlag(has_pending_activity: *std.atomic.Value(u32)) void { @fence(.seq_cst); _ = has_pending_activity.fetchAdd(1, .seq_cst); log("Interpreter incr pending activity {d}", .{has_pending_activity.load(.seq_cst)}); } - fn decrPendingActivityFlag(has_pending_activity: *std.atomic.Value(usize)) void { + fn decrPendingActivityFlag(has_pending_activity: *std.atomic.Value(u32)) void { @fence(.seq_cst); _ = has_pending_activity.fetchSub(1, .seq_cst); log("Interpreter decr pending activity {d}", .{has_pending_activity.load(.seq_cst)}); diff --git a/test/cli/run/shell-keepalive-fixture-1.js b/test/cli/run/shell-keepalive-fixture-1.js new file mode 100644 index 0000000000..8e7780ece9 --- /dev/null +++ b/test/cli/run/shell-keepalive-fixture-1.js @@ -0,0 +1,8 @@ +process.exitCode = 1; + +(async () => { + console.log("here 1"); + await Bun.$`ls .`; + console.log("here 2"); + process.exit(0); +})(); diff --git a/test/cli/run/shell-keepalive-fixture-2.js b/test/cli/run/shell-keepalive-fixture-2.js new file mode 100644 index 0000000000..4a9497cdbd --- /dev/null +++ b/test/cli/run/shell-keepalive-fixture-2.js @@ -0,0 +1,6 @@ +process.exitCode = 1; + +(async () => { + await Bun.$`${process.execPath} -e "console.log('hi')"`; + process.exit(0); +})(); diff --git a/test/cli/run/shell-keepalive.test.ts b/test/cli/run/shell-keepalive.test.ts new file mode 100644 index 0000000000..fb082cb74a --- /dev/null +++ b/test/cli/run/shell-keepalive.test.ts @@ -0,0 +1,11 @@ +import { expect, test } from "bun:test"; +import { join } from "path"; +import "harness"; + +test("shell should stay alive while a builtin command is in progress", async () => { + expect([join(import.meta.dir, "shell-keepalive-fixture-1.js")]).toRun(); +}); + +test("shell should stay alive while a non-builtin command is in progress", async () => { + expect([join(import.meta.dir, "shell-keepalive-fixture-2.js")]).toRun(); +});