fix(windows spawn): use Job Object to manage subprocesses of subprocesses #11240
Conversation
|
❌ @paperdave, your commit has failing tests :( 💪 2 failing tests Darwin AARCH64
💻 4 failing tests Darwin x64
🐧💪 3 failing tests Linux AARCH64
🪟💻 8 failing tests Windows x64 baseline
🪟💻 9 failing tests Windows x64
|
src/bun.js/api/bun/process.zig
Outdated
| if (bun.windows.CreateJobObjectA(null, null)) |job| | ||
| _ = bun.windows.AssignProcessToJobObject(job, process.poller.uv.process_handle); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Changes look good now, however there seems to be a test failure in |
|
I think the hot and watch tests need to pass before this PR can be merged |
This comment was marked as resolved.
This comment was marked as resolved.
What does this PR do?
See test case, note how --watch spawns two processes, but before, the
killin the outermost process would keep the innermost subprocess alive. Now, callingkillin the outermost process kills the whole tree. This was an issue if the caller of this process tree depended on standard io, which would not be closed before. Now, since the processes are dead, the standard io is closed.