bun icon indicating copy to clipboard operation
bun copied to clipboard
Published 5 days ago • verified publisher icon oven-sh

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

Open paperclover opened this issue 1 year ago • 4 comments

What does this PR do?

See test case, note how --watch spawns two processes, but before, the kill in the outermost process would keep the innermost subprocess alive. Now, calling kill in 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.

paperclover avatar May 21 '24 21:05 paperclover

❌ @paperdave, your commit has failing tests :(

💪 2 failing tests Darwin AARCH64

  • test/cli/install/bun-install.test.ts 1 failing
  • test/js/node/tls/node-tls-context.test.ts 1 failing

💻 4 failing tests Darwin x64

  • test/cli/install/bun-create.test.ts 1 failing
  • test/cli/install/bun-upgrade.test.ts 1 failing
  • test/js/node/tls/node-tls-context.test.ts 1 failing
  • test/js/web/workers/worker.test.ts 1 failing

🐧💪 3 failing tests Linux AARCH64

  • test/cli/install/registry/bun-install-registry.test.ts 3 failing
  • test/integration/mysql2/mysql2.test.ts 2 failing
  • test/js/bun/http/serve.test.ts 1 failing

🪟💻 8 failing tests Windows x64 baseline

  • test/cli/install/bun-install.test.ts 41 failing
  • test/cli/install/bunx.test.ts 2 failing
  • test/js/bun/shell/bunshell.test.ts 1 failing
  • test/js/bun/spawn/spawn.test.ts SIGKILL
  • test/js/node/dns/node-dns.test.js 2 failing
  • test/js/node/http/node-http.test.ts 1 failing
  • test/js/node/process/process.test.js 2 failing
  • test/js/node/watch/fs.watchFile.test.ts 3 failing

🪟💻 9 failing tests Windows x64

  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/migration/migrate.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts 3 failing
  • test/integration/esbuild/esbuild.test.ts 1 failing
  • test/js/bun/shell/bunshell.test.ts 1 failing
  • test/js/node/dns/node-dns.test.js 2 failing
  • test/js/node/http/node-http.test.ts 1 failing
  • test/js/node/process/process.test.js 2 failing
  • test/js/node/watch/fs.watchFile.test.ts SIGKILL

View logs

github-actions[bot] avatar May 21 '24 22:05 github-actions[bot]

Changes look good now, however there seems to be a test failure in watch.test.ts on windows - I'm not sure if it was there before, but I think it wasn't

gvilums avatar May 22 '24 17:05 gvilums

I think the hot and watch tests need to pass before this PR can be merged

Jarred-Sumner avatar May 22 '24 18:05 Jarred-Sumner

these tests are definitely regressions. its a panic from failing to create the job object

image

paperclover avatar May 22 '24 20:05 paperclover