process.stdin: leave the ref of the paused stdin source for its next reader

Closed
RO
15d ago
+287-1

Stacked on #46528. It needs the resume that #46528 adds when a reader attaches.

Problem

  • After process.stdin.pause() or rl.close(), for await (const line of console) reads nothing and the process exits with code 0. A Bun.stdin.stream() reader and Readable.fromWeb(Bun.stdin.stream()) do the same.
  • #46528 fixes these only under a top-level await.
  • The cause is the last line of disown() (src/js/builtins/ProcessObjectInternals.ts:154), source?.updateRef?.(false). The next reader resumes the source, but its poll keeps nothing alive.

Fix

  • disown() ends in source?.updateRef?.(!forceUnref). That is the whole source change.
  • Correct because a paused source has no armed poll. The ref keeps nothing alive until a reader resumes the source, and that reader's release drops it.
  • Verified: 32 new tests in test/js/node/process/process-stdin.test.ts. 27 fail without the line. The other 5 check that the process still exits.
  • Self-reviewed: 8 concerns raised, 7 addressed (Notes).

Background

  • process.stdin reads through a reader on Bun.stdin.stream(). disown() runs when it lets go: one tick after 'pause', and on 'close'.
  • setFlowing(false) unregisters the poll of the native source on fd 0. updateRef() sets whether that poll keeps the process alive when armed. forceUnref is true after process.stdin.unref().
  • Considered a ref where #46528 resumes the source. It changes every file-backed stream (#41578).

Downsides

  • A reader taken after the pause and never read keeps the process alive until stdin closes, as an idle reader does when nothing paused.
  • Still broken: a reader that holds the lock when pause() runs reads nothing.
  • No new call, allocation or syscall. The builtin text grows by 9 bytes.
Notes

What is left of this PR. Until b3356db46a this PR had three parts: a resume when a reader locks the stream (C++), a started check in FileReader::set_flowing (Rust), and two lines in disown(). #46528 has the first two since 44e485859c, as resumeNativeSource() in setUpReadableStreamDefaultReader and if !self.started.get() || ... in set_flowing. They are gone from here, and so is the segfault of pause() followed by Readable.fromWeb(Bun.stdin.stream()), which #46528 fixes. One line of disown() is left.

Measured (Linux x64, debug builds of 04af03e149 with and without the line). The child runs the first column and then reads. The parent writes the input 500 ms after the child prints a marker, and then closes the pipe.

The child runsmain 7d240c1dd5 (release)#46528#46528 with the line
pause(), one setImmediate, the console iteratornothing, exit 0nothing, exit 0reads all
the same with a getReader() loop, for await, pipeTo() or .text()nothing, exit 0nothing, exit 0reads all
the same with Readable.fromWeb(Bun.stdin.stream())SIGSEGVnothing, exit 0reads all
rl.close(), resume() then pause(), or a 'data' listener that pauses, then a readernothing more, exit 0nothing more, exit 0reads all
top-level await, pause(), a reader that waits 1 s: CPU timenever reads979 to 1004 ms10 to 11 ms

The last row: with nothing ref'd, a pending top-level await spins in EventLoop::wait_for_promise until the input arrives. await new Promise(() => {}) alone does the same on 1.3.13 (#14951), so that part is older and not changed here.

History: the pause() rows read everything on 1.3.13 and nothing on 1.4.2 (SIGSEGV for Readable.fromWeb). Since #34971 (1.4.0) a reader remembers an unref that comes before its first poll registration, and disown() unrefs a source that never started. The rl.close() rows read nothing on 1.3.13 too.

The tests. One describe block, four ways for process.stdin to let go (pause(), rl.close(), destroy(), a break out of for await (const chunk of process.stdin)) and three readers (the console iterator, a Bun.stdin.stream() reader, Readable.fromWeb). No child has a top-level await.

  • 12 rows: the reader reads the rest of stdin.
  • 12 rows: the reader reads one chunk and lets go. The parent leaves stdin open, and the child has to exit by itself.
  • 4 rows: nothing reads stdin again, stdin stays open, the child has to exit.
  • 1 row each: the same over a shell pipe (bun | bun), process.stdin takes stdin back after the reader, a readline prompt on a pty followed by the console iterator, and process.stdin.unref() before the pause. The writer of the pipe is not cat: busybox cat copies with sendfile(), and a sendfile() that waits for input keeps the pipe locked, so the reader blocks in read().

The child prints its marker from an unref'd 50 ms timer. That timer fires only while stdin keeps the process alive, so without the line the marker never comes and the parent never writes. On debug builds of 04af03e149, 26 rows failed in 5 of 5 runs without the line and all rows passed in 5 of 5 runs with it. The shell pipe row failed in 3 of 3 runs without the line and passed in 3 of 3 with it (release builds). The 5 rows that pass both ways are the 4 exit rows and the unref() row. With updateRef(true) in place of the line, the unref() row fails.

Self-review. Concerns and what happened to each:

  1. The resume at lock time can run user JS (getReader() in a beforeExit listener runs a queued process.nextTick callback before it returns). This PR no longer has a resume. The same holds for resumeNativeSource in #46528, reported there.
  2. The lock check made pause() a no-op under every lock holder. Removed, see below.
  3. The rows raced the exit of the child. Now the marker comes from the unref'd timer.
  4. No row could fail on a leaked keep-alive. Now 17 rows leave stdin open.
  5. The forceUnref half had no test. Now the unref() row.
  6. The native.locked half had no test. Removed with the lock check.
  7. The 'close' path had no test. Now two of the four doors.
  8. Only socketpair and pty stdin. Added the shell pipe row. Not added: rows with stdin from a file. They pass without the line, because a regular file has no poll.

The lock check left this PR. if (!reader && native.locked) return; made process.stdin leave a source alone while another reader holds the lock. It is a new ownership rule with a cost: after pause() under a lock holder, a child with stdin: "inherit" got nothing in 5 of 10 runs (0 of 10 without the check), because the holder still reads fd 0. That is the contract of #13978 and #23333. It needs its own decision. #43951 points at the same case.

Not changed.

  • A reader that holds the lock when pause() or destroy() runs reads nothing more (Readable.fromWeb in the same tick as pause() too). Under a top-level await it hangs.
  • process.stdin.unref() still applies to the shared source, so the next reader does not keep the process alive.
  • While another reader holds the lock, process.stdin.resume() throws ReadableStream is locked (#45068, #7541).
  • A second reader after another reader's release (not after process.stdin) still exits early (#41578).
  • pause() still frees fd 0 for a child: test/js/node/readline/stdin-pause-pty.test.ts passes on release builds with and without the line.

Suites. Debug build with the line, 0 failures: process-stdin.test.ts (63), console-iterator.test.ts (17), readline.node.test.ts (81), readline_promises.node.test.ts, pause_stdin_should_exit, readline_never_unrefs, stdin_fell_asleep, process-stdio.test.ts, process-stdin-stale-hup, terminal.test.ts (101), terminal-spawn.test.ts, test/js/node/tty, spawn-stdin-readable-stream.test.ts (54), spawn-socketpair-shutdown, spawn-streaming-stdin, node-stream.test.js (162), and the 33 Node test-stdin-*, test-tty-stdin-*, test-readline-* files. On this machine stdin-fixtures.test.ts (a 1 s kill timer) and stdin-pause-pty.test.ts fail on debug builds with and without the line, and pass 3 of 3 on release builds both ways. Nothing ran on macOS or Windows here.


no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/process/process-stdin.test.ts