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

Introduce deathSig option in Bun.spawn #15210

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

Jarred-Sumner
Copy link
Collaborator

What does this PR do?

Introduce deathSig option in Bun.spawn

This is a Linux-only option that lets you automatically make spawned processes get killed when Bun exits (as long as they don't set detached: true, or call setsid()).

This enables this option in bun run <package.json script>, bun install for lifecycle scripts, and almost everywhere else we spawn processes.

How did you verify your code works?

WIP.

@robobun
Copy link

robobun commented Nov 18, 2024

@Jarred-Sumner, your commit dc21a40 has 10 failures in #6304:

  • test/integration/next-pages/test/dev-server.test.ts - 1 failing on :alpine: 3.20 x64
  • test/integration/next-pages/test/dev-server.test.ts - 1 failing on :alpine: 3.20 x64-baseline
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on :alpine: 3.20 aarch64
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on :alpine: 3.20 x64
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on :alpine: 3.20 x64-baseline
  • test/v8/v8.test.ts - 22 failing on :alpine: 3.20 aarch64
  • test/v8/v8.test.ts - 22 failing on :alpine: 3.20 x64
  • test/v8/v8.test.ts - 22 failing on :alpine: 3.20 x64-baseline
  • test/js/bun/http/serve.test.ts - segmentation fault on :alpine: 3.20 aarch64
  • test/js/bun/http/serve.test.ts - segmentation fault on :alpine: 3.20 x64
  • test/js/bun/http/serve.test.ts - segmentation fault on :alpine: 3.20 x64-baseline
  • test/js/bun/ffi/cc.test.ts - 1 failing on :alpine: 3.20 aarch64
  • test/js/bun/ffi/cc.test.ts - 1 failing on :alpine: 3.20 x64
  • test/js/bun/ffi/cc.test.ts - 1 failing on :alpine: 3.20 x64-baseline
  • test/js/node/child_process/child_process.test.ts - 1 failing on :alpine: 3.20 aarch64
  • test/js/node/child_process/child_process.test.ts - 1 failing on :alpine: 3.20 x64
  • test/js/node/child_process/child_process.test.ts - 1 failing on :alpine: 3.20 x64-baseline
  • test/cli/install/bun-link.test.ts - 4 failing on :alpine: 3.20 aarch64
  • test/cli/install/bun-link.test.ts - 4 failing on :alpine: 3.20 x64
  • test/cli/install/bun-link.test.ts - 4 failing on :alpine: 3.20 x64-baseline
  • test/js/web/fetch/fetch.tls.test.ts - 1 failing on 🍎 14 aarch64
  • test/js/web/timers/setInterval.test.js - 1 failing on 🪟 2019 x64-baseline
  • test/js/node/test/parallel/child-process-exec-timeout-kill.test.js - 1 failing on 🍎 13 aarch64
  • Copy link
    Member

    @paperdave paperdave left a comment

    Choose a reason for hiding this comment

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

    in addition to review nitpicks, this feature, as is, will worsen the footgun because it only solves this problem on linux, and not on windows or macOS. someone will see this, briefly check the first paragaph, see it works on linux, and declare the problem solved. in reality, the problem persists on the other operating systems.

    that being said, that shouldn't block this pr. the other platforms can be done in another pr.

    alternate name suggestion: killOnParentExit: true | string (i think having an easy true aliasing for KILL would be helpful, it would also be the only possible way to emulate this on windows as it does not have kill signals)

    @@ -970,6 +970,11 @@ pub const PosixSpawnOptions = struct {
    argv0: ?[*:0]const u8 = null,
    stream: bool = true,

    /// Linux-only.
    ///
    /// Send a signal to the parent process when the child process exits.
    Copy link
    Member

    Choose a reason for hiding this comment

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

    this comment is reversed from the typescript documentation

    /// Linux-only.
    ///
    /// Send a signal to the parent process when the child process exits.
    deathsig: bun.SignalCode = @enumFromInt(0),
    Copy link
    Member

    Choose a reason for hiding this comment

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

    bad practice doing hiding an invalid state in a valid enum. should zero be added to the enum as none?

    @@ -1226,6 +1234,10 @@ pub fn spawnProcessPosix(

    if (options.detached) {
    flags |= bun.C.POSIX_SPAWN_SETSID;
    } else if (Environment.isLinux) {
    if (@intFromEnum(options.deathsig) > 0) {
    Copy link
    Member

    Choose a reason for hiding this comment

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

    options.deathsig.valid()?

    @@ -299,6 +300,7 @@ pub const PosixSpawn = struct {
    const BunSpawnRequest = extern struct {
    chdir_buf: ?[*:0]u8 = null,
    detached: bool = false,
    deathsig: i32 = 0,
    Copy link
    Member

    Choose a reason for hiding this comment

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

    c_int

    @@ -637,6 +637,7 @@ pub const BunxCommand = struct {
    .stderr = .inherit,
    .stdout = .inherit,
    .stdin = .inherit,
    .deathsig = .SIGTERM,
    Copy link
    Member

    Choose a reason for hiding this comment

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

    should our default be .SIGTERM?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants