Skip to content

fix(ext/fs): run open_async on the blocking pool so FIFO opens don't stall the runtime - #33667

Merged
littledivy merged 2 commits into
denoland:mainfrom
divybot:claude/test-http2-respond-file-error-pipe-offset
Apr 29, 2026
Merged

littledivy merged 2 commits into
denoland:mainfrom
divybot:claude/test-http2-respond-file-error-pipe-offset

Conversation

@divybot

@divybot divybot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Enables test-http2-respond-file-error-pipe-offset in node_compat suite.

Test plan

  • cargo test --test node_compat -- test-http2-respond-file-error-pipe-offset

divybot and others added 2 commits April 29, 2026 09:12
…stall the runtime

Enables tests/node_compat/runner/suite/test/parallel/test-http2-respond-file-error-pipe-offset.js

Co-authored-by: Divy Srivastava <[email protected]>

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Substance is right. The diagnosis is exact: opening a FIFO (named pipe) with O_RDONLY or O_WRONLY blocks until the other end is opened, and open_async was previously running open_with_checked_path (which goes through std::fs::File::open, a synchronous syscall) on the runtime thread. So a fs.open("/path/to/fifo", "r") would hang the whole event loop until something writes to the FIFO.

Verified:

  • spawn_blocking already imported at ext/fs/std_fs.rs:15 and used extensively elsewhere in this file (mkdir, chmod, chown, etc. — lines 130, 144, 148, 165, …). The fix is consistent with the existing pattern.
  • Closure captures: move || { ... } takes ownership of both options (by value) and path: CheckedPathBuf. path.as_checked_path() returns a short-lived &CheckedPath borrow used only inside the open call, then path is moved back out via .map(|f| (f, path)). The (std_file, path) = std_file destructure on the outside recovers it for the subsequent path.to_path_buf().
  • Double ??: .await? unwraps tokio's JoinError, the second ? unwraps FsResult from the closure body. Standard pattern.
  • No effect on open_sync: sync APIs are explicitly synchronous, so they shouldn't and don't go through the blocking pool. ✓
  • Trade-off: this routes all open_async calls (not just FIFO ones) through spawn_blocking. That's a few microseconds of thread-pool dispatch overhead per regular-file open vs hanging the event loop on FIFOs. The alternative — detecting FIFO upfront — requires a stat that itself can block. Right call.

Test enrollment alphabetically positioned correctly (respond-file-error-dir < respond-file-error-pipe-offset < respond-file-errors). ✓

CI

105 success, 0 failure, 17 still pending. Holding to COMMENT per the CI-must-complete rule. I'm 95%+ on substance; happy to flip to APPROVE once test node_compat debug × 6 platforms (the shard that runs test-http2-respond-file-error-pipe-offset) and test fs shards land green.

@littledivy littledivy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch

@littledivy
littledivy merged commit de79a32 into denoland:main Apr 29, 2026
136 checks passed
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