fix(ext/node): support fd 3 pipes in spawned Deno children#34133
Conversation
|
@bartlomieju CI is green. |
bartlomieju
changed the title
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Clean fix for #33368. The key trick — duping the inherited fd into a separate handle so node:fs operations go through the dup while the original fd stays free for libuv (net.Socket({ fd })) to claim later via pipe_wrap.rs::open removing it from the table — is the right shape and matches what Node expects.
CI is green. A few non-blocking suggestions inline; the env-var leak is the only one with real teeth, the rest are quality/test-coverage nits.
# Conflicts: # ext/process/lib.rs
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Traced the full mechanism — this is a solid fix, and the dup-vs-original split (node:fs uses a dup(), the original numeric fd stays open so libuv can reclaim it via net.Socket({ fd })) is the right design. Rc refcounting also correctly avoids a premature close when the fs stream and socket overlap. Windows cfg gating checks out.
Gate note: the API shows "No checks found" and the "CI is green" comment predates the two later merges from main (last on 2026-07-01). Please confirm CI is green on the current head before merge — I can't verify it from here.
Details inline below. Nothing blocking beyond #1 + CI confirmation.
- PipeWrap::open: consume the InheritedExtraStdio entry only after uv_pipe_open succeeds. Previously the entry (and its dup) was removed up front, so a failed uv_pipe_open left the fd untracked and unusable by node:fs. Now the fd stays usable when the open fails. - fd_table: document that InheritedExtraStdio holds a dup(), so dropping the entry closes only the dup and deliberately keeps the original fd open for libuv to reclaim (a trade-off vs Node's autoClose behavior). - io init: make the remove_var SAFETY comment accurate (runs synchronously on the main thread during extension init, before any code that could touch the environment concurrently).
Follow-up to #34133, which registers inherited extra stdio fds in the child's FdTable so node:fs fd APIs can use them. That registration broke adopting an inherited fd that is a TCP socket: `net.Socket({ fd })` routes through `TCPWrap::open`, whose duplicate-fd check was not taught about `InheritedExtraStdio` entries and rejected the fd with `EEXIST` (`PipeWrap` got the exemption, `TCPWrap` did not). This mirrors the `PipeWrap` logic in `TCPWrap::open`: inherited extra stdio fds are allowed, and their entry (with the node:fs dup it holds) is consumed only once `uv_tcp_open` actually claims the fd. The new regression test fails with `EEXIST` without this change. The startup dup is now also created with `F_DUPFD_CLOEXEC` instead of `dup()`. It exists purely for in-process node:fs use, but without close-on-exec it leaked into spawned grandchildren, holding pipe ends open and delaying EOF for the parent past what Node exhibits.
2 participants
Fixes #33368.
Closes bartlomieju/orchid-inbox#105.
This registers extra stdio fds installed by Deno's Node child_process spawn path in the child process FdTable. The parent annotates Deno children with the exact inherited fd slots it creates, and startup validates those descriptors before exposing them to node:fs fd APIs.
Inherited extra stdio fds are backed by duplicated handles for node:fs, so the original fd can still be claimed by libuv APIs such as net.Socket({ fd }) used by fork and cluster stdio tests.
This avoids scanning arbitrary process fds and clears the marker for child processes that do not receive extra stdio. The regression test is Unix-only because this bug is about inherited numeric fd slots installed with dup2.
Tests:
AI assistance was used to prepare this PR.