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

fix(windows spawn): use Job Object to manage subprocesses of subprocesses #11240

Merged
merged 7 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/bun.js/api/bun/process.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1671,7 +1671,7 @@ pub fn spawnProcessWindows(
uv_process_options.stdio_count = @intCast(stdio_containers.items.len);

uv_process_options.exit_cb = &Process.onExitUV;
var process = Process.new(.{
const process = Process.new(.{
.event_loop = options.windows.loop,
.pid = 0,
});
Expand Down Expand Up @@ -1716,6 +1716,13 @@ pub fn spawnProcessWindows(
.extra_pipes = try std.ArrayList(WindowsSpawnResult.StdioResult).initCapacity(bun.default_allocator, options.extra_fds.len),
};

// By putting this process in a Job Object, any child process will be caught
// so that killing this process will kill all children processes.
//
// It seems that just associating the process does the work neccecary.
if (bun.windows.CreateJobObjectA(null, null)) |job|
_ = bun.windows.AssignProcessToJobObject(job, process.poller.uv.process_handle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the process has already spawned a child process and then exited by the time it's being assigned to the job object? See this blog for context. Not sure if this is an edge case we should care about, but might be relevant.

To do it the "right" way we might need to mess with libuv internals for process spawning, not sure if we want to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i moved this to just the code for --watch (libuv already does this and actually did it more correct than what i have here), and i followed the linked guide so that we spawn the process without this race. thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now 👍


const result_stdios = .{ &result.stdin, &result.stdout, &result.stderr };
inline for (0..3) |i| {
const stdio = stdio_containers.items[i];
Expand Down
1 change: 1 addition & 0 deletions test/js/bun/spawn/empty.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// empty file
14 changes: 14 additions & 0 deletions test/js/bun/spawn/job-object-bug.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { spawn } from "bun";
import { it, expect } from "bun:test";
import { bunEnv, bunExe } from "harness";
import { join } from "node:path";

it("does not hang", async () => {
const subprocess = spawn({
cmd: [bunExe(), "test", join(import.meta.dirname, "job-object-bug.ts")],
env: bunEnv,
stdio: ["ignore", "pipe", "pipe"],
});
await Bun.readableStreamToText(subprocess.stdout);
expect(await subprocess.exited).toBe(0);
});
16 changes: 16 additions & 0 deletions test/js/bun/spawn/job-object-bug.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { spawn } from "bun";
import { bunEnv, bunExe } from "harness";
import { join } from "node:path";

// prior, this would hang on Windows if you ran this with a pipe

const run = spawn({
cmd: [bunExe(), "--watch", join(import.meta.dirname, "empty.js")],
stdout: "inherit",
stderr: "inherit",
stdin: "ignore",
env: bunEnv,
});
await Bun.sleep(250);
run.kill(9);
await run.exited;
Loading