Repository navigation
fix(ext/fs): run open_async on the blocking pool so FIFO opens don't stall the runtime - #33667
Conversation
…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]>
Signed-off-by: Divy Srivastava <[email protected]>
fibibot
left a comment
There was a problem hiding this comment.
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_blockingalready imported atext/fs/std_fs.rs:15and 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 bothoptions(by value) andpath: CheckedPathBuf.path.as_checked_path()returns a short-lived&CheckedPathborrow used only inside the open call, thenpathis moved back out via.map(|f| (f, path)). The(std_file, path) = std_filedestructure on the outside recovers it for the subsequentpath.to_path_buf(). - Double
??:.await?unwraps tokio'sJoinError, the second?unwrapsFsResultfrom 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_asynccalls (not just FIFO ones) throughspawn_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 astatthat 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.
Summary
Enables
test-http2-respond-file-error-pipe-offsetin node_compat suite.Test plan
cargo test --test node_compat -- test-http2-respond-file-error-pipe-offset