Skip to content

Commit

Permalink
Support AbortSignal in Bun.spawn (#14180)
Browse files Browse the repository at this point in the history
  • Loading branch information
Jarred-Sumner authored Sep 26, 2024
1 parent 7b058e2 commit 18822b9
Show file tree
Hide file tree
Showing 3 changed files with 171 additions and 1 deletion.
26 changes: 26 additions & 0 deletions packages/bun-types/bun.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4615,6 +4615,32 @@ declare module "bun" {
* @default cmds[0]
*/
argv0?: string;

/**
* An {@link AbortSignal} that can be used to abort the subprocess.
*
* This is useful for aborting a subprocess when some other part of the
* program is aborted, such as a `fetch` response.
*
* Internally, this works by calling `subprocess.kill(1)`.
*
* @example
* ```ts
* const controller = new AbortController();
* const { signal } = controller;
* const start = performance.now();
* const subprocess = Bun.spawn({
* cmd: ["sleep", "100"],
* signal,
* });
* await Bun.sleep(1);
* controller.abort();
* await subprocess.exited;
* const end = performance.now();
* console.log(end - start); // 1ms instead of 101ms
* ```
*/
signal?: AbortSignal;
}

type OptionsToSubprocess<Opts extends OptionsObject> =
Expand Down
53 changes: 52 additions & 1 deletion src/bun.js/api/bun/subprocess.zig
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ pub const Subprocess = struct {

weak_file_sink_stdin_ptr: ?*JSC.WebCore.FileSink = null,
ref_count: u32 = 1,
abort_signal: ?*JSC.AbortSignal = null,

usingnamespace bun.NewRefCounted(@This(), Subprocess.deinit);

Expand All @@ -226,6 +227,12 @@ pub const Subprocess = struct {
poll_ref: Async.KeepAlive = .{},
};

pub fn onAbortSignal(subprocess_ctx: ?*anyopaque, _: JSC.JSValue) callconv(.C) void {
var this: *Subprocess = @ptrCast(@alignCast(subprocess_ctx.?));
this.clearAbortSignal();
_ = this.tryKill(SignalCode.default);
}

pub fn resourceUsage(
this: *Subprocess,
globalObject: *JSGlobalObject,
Expand Down Expand Up @@ -1437,6 +1444,7 @@ pub const Subprocess = struct {
this_jsvalue.ensureStillAlive();
this.pid_rusage = rusage.*;
const is_sync = this.flags.is_sync;
this.clearAbortSignal();
defer this.deref();
defer this.disconnectIPC(true);

Expand Down Expand Up @@ -1587,13 +1595,24 @@ pub const Subprocess = struct {
this.destroy();
}

fn clearAbortSignal(this: *Subprocess) void {
if (this.abort_signal) |signal| {
this.abort_signal = null;
signal.pendingActivityUnref();
signal.cleanNativeBindings(this);
signal.unref();
}
}

pub fn finalize(this: *Subprocess) callconv(.C) void {
log("finalize", .{});
// Ensure any code which references the "this" value doesn't attempt to
// access it after it's been freed We cannot call any methods which
// access GC'd values during the finalizer
this.this_jsvalue = .zero;

this.clearAbortSignal();

bun.assert(!this.hasPendingActivity() or JSC.VirtualMachine.get().isShuttingDown());
this.finalizeStreams();

Expand Down Expand Up @@ -1702,6 +1721,13 @@ pub const Subprocess = struct {

var windows_hide: bool = false;
var windows_verbatim_arguments: bool = false;
var abort_signal: ?*JSC.WebCore.AbortSignal = null;
defer {
// Ensure we clean it up on error.
if (abort_signal) |signal| {
signal.unref();
}
}

{
if (args.isEmptyOrUndefinedOrNull()) {
Expand Down Expand Up @@ -1846,6 +1872,14 @@ pub const Subprocess = struct {
}
}

if (args.getOwnTruthy(globalThis, "signal")) |signal_val| {
if (signal_val.as(JSC.WebCore.AbortSignal)) |signal| {
abort_signal = signal.ref();
} else {
return globalThis.throwInvalidArgumentTypeValue("signal", "AbortSignal", signal_val);
}
}

if (args.getOwnTruthy(globalThis, "onDisconnect")) |onDisconnect_| {
if (!onDisconnect_.isCell() or !onDisconnect_.isCallable(globalThis.vm())) {
globalThis.throwInvalidArguments("onDisconnect must be a function or undefined", .{});
Expand Down Expand Up @@ -2280,12 +2314,29 @@ pub const Subprocess = struct {
should_close_memfd = false;

if (comptime !is_sync) {
// Once everything is set up, we can add the abort listener
// Adding the abort listener may call the onAbortSignal callback immediately if it was already aborted
// Therefore, we must do this at the very end.
if (abort_signal) |signal| {
signal.pendingActivityRef();
subprocess.abort_signal = signal.addListener(subprocess, onAbortSignal);
abort_signal = null;
}
return out;
}

if (comptime is_sync) {
switch (subprocess.process.watchOrReap()) {
.result => {},
.result => {
// Once everything is set up, we can add the abort listener
// Adding the abort listener may call the onAbortSignal callback immediately if it was already aborted
// Therefore, we must do this at the very end.
if (abort_signal) |signal| {
signal.pendingActivityRef();
subprocess.abort_signal = signal.addListener(subprocess, onAbortSignal);
abort_signal = null;
}
},
.err => {
subprocess.process.wait(true);
},
Expand Down
93 changes: 93 additions & 0 deletions test/js/bun/spawn/spawn-signal.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import { test, expect } from "bun:test";
import { spawn, sleep } from "bun";
import { bunEnv, bunExe } from "harness";

test("spawn AbortSignal works after spawning", async () => {
const controller = new AbortController();
const { signal } = controller;
const start = performance.now();
const subprocess = Bun.spawn({
cmd: [bunExe(), "--eval", "await Bun.sleep(100000)"],
env: bunEnv,
stdout: "inherit",
stderr: "inherit",
stdin: "inherit",
signal,
});
await Bun.sleep(1);
controller.abort();
expect(await subprocess.exited).not.toBe(0);
const end = performance.now();
expect(end - start).toBeLessThan(100);
});

test("spawn AbortSignal works if already aborted", async () => {
const controller = new AbortController();
const { signal } = controller;
const start = performance.now();
const subprocess = Bun.spawn({
cmd: [bunExe(), "--eval", "await Bun.sleep(100000)"],
env: bunEnv,
stdout: "inherit",
stderr: "inherit",
stdin: "inherit",
signal,
});
await Bun.sleep(1);
controller.abort();
expect(await subprocess.exited).not.toBe(0);
const end = performance.now();
expect(end - start).toBeLessThan(100);
});

test("spawn AbortSignal args validation", async () => {
expect(() =>
Bun.spawn({
cmd: [bunExe(), "--eval", "await Bun.sleep(100000)"],
env: bunEnv,
stdout: "inherit",
stderr: "inherit",
stdin: "inherit",
signal: 123,
}),
).toThrow();
});

test("spawnSync AbortSignal works as timeout", async () => {
const start = performance.now();
const subprocess = Bun.spawnSync({
cmd: [bunExe(), "--eval", "await Bun.sleep(100000)"],
env: bunEnv,
stdout: "inherit",
stderr: "inherit",
stdin: "inherit",
signal: AbortSignal.timeout(10),
});

expect(subprocess.success).toBeFalse();
const end = performance.now();
expect(end - start).toBeLessThan(100);
});

// TODO: this test should fail.
// It passes because we are ticking the event loop incorrectly in spawnSync.
// it should be ticking a different event loop.
test("spawnSync AbortSignal...executes javascript?", async () => {
const start = performance.now();
var signal = AbortSignal.timeout(10);
signal.addEventListener("abort", () => {
console.log("abort", performance.now());
});
const subprocess = Bun.spawnSync({
cmd: [bunExe(), "--eval", "await Bun.sleep(100000)"],
env: bunEnv,
stdout: "inherit",
stderr: "inherit",
stdin: "inherit",
signal,
});
console.log("after", performance.now());
expect(subprocess.success).toBeFalse();
const end = performance.now();
expect(end - start).toBeLessThan(100);
});

0 comments on commit 18822b9

Please sign in to comment.