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

node: Fix race condition with accessing stdioOptions #16670

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

anaisbetts
Copy link
Contributor

What does this PR do?

At Anthropic, we are using Bun for an internal CLI-based project that makes fairly significant use of node's child_process module. Intermittently across both macOS and Linux, we would see a crash in exec / spawn with a stack similar to:

TypeError: undefined is not an object (evaluating 'this.#stdioOptions')
      at #getBunSpawnIo (node:child_process:509:37)
      at <anonymous> (node:child_process:582:47)
      at #createStdioObject (node:child_process:566:27)
      at <anonymous> (node:child_process:591:57)
      at <anonymous> 

We believe that this is a race condition / ordering issue:

  1. The onExit callback accesses this.stdio if hasSocketsToEagerlyLoad is true
  2. this.stdio getter calls #createStdioObject
  3. #createStdioObject uses #getBunSpawnIo
  4. #getBunSpawnIo tries to access this.#stdioOptions

But at this point, this.#handle hasn't been set yet - it's set right after accessing stdio. So if the process exits very quickly, the onExit callback could run before the handle is properly set up, leading to this error.

This is particularly likely when:

  • The spawned process exits immediately
  • There are 3 or more stdio descriptors (which makes hasSocketsToEagerlyLoad true)

This change ensures that:

  1. We first set this.#handle = handle and update the pid
  2. Only then do we access this.stdio if needed
  3. Finally schedule the #handleOnExit callback

This should prevent the race condition where stdio is accessed before the handle is set, which was causing the TypeError: undefined is not an object (evaluating 'this.#stdioOptions') error

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

Because this is a fairly tight race condition, it is difficult to verify outside of certain machines. We have a coworker who for whatever reason would hit this issue every time, and this patch fixes it. The full release test suite was also run locally and child_process tests all pass.

@nektro
Copy link
Member

nektro commented Jan 23, 2025

approved CI to start

@Jarred-Sumner Jarred-Sumner merged commit 40d150b into oven-sh:main Jan 24, 2025
67 of 69 checks passed
@anaisbetts anaisbetts deleted the child-process-stdio-race branch January 24, 2025 15:45
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