From 76400adee947eb673a5e710bd328fc5c7ff2feb1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 29 May 2026 17:40:28 -0400 Subject: [PATCH] fix: do not lock process.stdin and tty fallback --- deno.json | 2 +- deno.lock | 14 ++- src/console/ttyFallback.ts | 88 ++++++++++--------- src/test/aborted-prompts-visual-inspection.ts | 59 +++++++++++++ 4 files changed, 112 insertions(+), 51 deletions(-) create mode 100644 src/test/aborted-prompts-visual-inspection.ts diff --git a/deno.json b/deno.json index 2a330f5..b4fc1d7 100644 --- a/deno.json +++ b/deno.json @@ -37,7 +37,7 @@ "exports": "./mod.ts", "imports": { "@david/console-static-text": "jsr:@david/console-static-text@^0.3.4", - "@david/shell": "jsr:@david/shell@^0.5.0", + "@david/shell": "jsr:@david/shell@^0.5.1", "@deno/dnt": "jsr:@deno/dnt@^0.42.3", "@david/path": "jsr:@david/path@^0.3.2", "@std/assert": "jsr:@std/assert@1", diff --git a/deno.lock b/deno.lock index aa483ca..66849d1 100644 --- a/deno.lock +++ b/deno.lock @@ -5,7 +5,7 @@ "jsr:@david/console-static-text@~0.3.4": "0.3.4", "jsr:@david/path@0.3": "0.3.2", "jsr:@david/path@~0.3.2": "0.3.2", - "jsr:@david/shell@0.5": "0.5.0", + "jsr:@david/shell@~0.5.1": "0.5.1", "jsr:@david/which-runtime@~0.2.1": "0.2.1", "jsr:@david/which@0.7": "0.7.0", "jsr:@davidbonnet/astring@1.8.6": "1.8.6", @@ -77,8 +77,8 @@ "@david/path@0.3.2": { "integrity": "f25aaa3de4d3b9edfe25ca5edf15e3014fee7df4f77d64327e5300c27c1a0497" }, - "@david/shell@0.5.0": { - "integrity": "4184b233b56b8417908616297b418332bfe56b1247177d3f331e43fbbd337198", + "@david/shell@0.5.1": { + "integrity": "2469318121505246c0633fd0cc34e907d8ee1716cbe41264eb78026f31b53d6c", "dependencies": [ "jsr:@david/console-static-text", "jsr:@david/path@0.3", @@ -248,11 +248,7 @@ ] }, "@ts-morph/common@0.27.0": { - "integrity": "c7b73592d78ce8479b356fd4f3d6ec3c460d77753a8680ff196effea7a939052", - "dependencies": [ - "jsr:@std/fs@1", - "jsr:@std/path@1" - ] + "integrity": "c7b73592d78ce8479b356fd4f3d6ec3c460d77753a8680ff196effea7a939052" } }, "npm": { @@ -724,7 +720,7 @@ "dependencies": [ "jsr:@david/console-static-text@~0.3.4", "jsr:@david/path@~0.3.2", - "jsr:@david/shell@0.5", + "jsr:@david/shell@~0.5.1", "jsr:@david/which-runtime@~0.2.1", "jsr:@david/which@0.7", "jsr:@deno/dnt@~0.42.3", diff --git a/src/console/ttyFallback.ts b/src/console/ttyFallback.ts index 1038da3..f5015d6 100644 --- a/src/console/ttyFallback.ts +++ b/src/console/ttyFallback.ts @@ -1,5 +1,4 @@ import * as fs from "node:fs"; -import { Readable } from "node:stream"; import * as tty from "node:tty"; import process from "node:process"; @@ -17,13 +16,10 @@ export interface TtyStdin { interface OpenedTty { fd: number; stream: tty.ReadStream; - reader: ReadableStreamDefaultReader; } // undefined = not yet attempted; null = attempted and failed let cached: OpenedTty | null | undefined; -let pendingTtyRead: Promise> | undefined; -let ttyLeftover: Uint8Array | undefined; function tryOpen(): OpenedTty | null { if (cached !== undefined) return cached; @@ -33,12 +29,7 @@ function tryOpen(): OpenedTty | null { // and on some platforms that's only granted when opened for writing too. const fd = fs.openSync(path, "r+"); const stream = new tty.ReadStream(fd); - // wrapping the stream locks it to one consumer; cache so an aborted - // read doesn't lose bytes — the in-flight read and leftover buffer - // are tied to this reader, same pattern as @david/shell's stdin. - const readable = Readable.toWeb(stream) as ReadableStream; - const reader = readable.getReader(); - cached = { fd, stream, reader }; + cached = { fd, stream }; } catch { cached = null; } @@ -54,26 +45,12 @@ export function hasFallbackTty(): boolean { * use if no controlling terminal is available — guard with * {@link hasFallbackTty} first. */ export const ttyStdin: TtyStdin = { - async read(p: Uint8Array, options?: { signal?: AbortSignal }): Promise { + read(p: Uint8Array, options?: { signal?: AbortSignal }): Promise { const signal = options?.signal; signal?.throwIfAborted(); const handle = tryOpen(); - if (handle == null) throw new Error("No controlling terminal available."); - - if (ttyLeftover === undefined) { - // share a single in-flight read across callers so an aborted read - // doesn't drop bytes — the next call awaits the same promise. - pendingTtyRead ??= handle.reader.read(); - const result = await (signal ? raceAbort(pendingTtyRead, signal) : pendingTtyRead); - pendingTtyRead = undefined; - if (result.done || result.value === undefined) return null; - ttyLeftover = result.value; - } - - const len = Math.min(ttyLeftover.length, p.length); - p.set(ttyLeftover.subarray(0, len)); - ttyLeftover = ttyLeftover.length > len ? ttyLeftover.subarray(len) : undefined; - return len; + if (handle == null) return Promise.reject(new Error("No controlling terminal available.")); + return readTty(handle.stream, p, signal); }, setRaw(mode: boolean): void { const handle = tryOpen(); @@ -82,19 +59,48 @@ export const ttyStdin: TtyStdin = { }, }; -function raceAbort(promise: Promise, signal: AbortSignal): Promise { - return new Promise((resolve, reject) => { - const onAbort = () => reject(signal.reason); - signal.addEventListener("abort", onAbort, { once: true }); - promise.then( - (v) => { - signal.removeEventListener("abort", onAbort); - resolve(v); - }, - (e) => { - signal.removeEventListener("abort", onAbort); - reject(e); - }, - ); +// reads directly from the tty.ReadStream in paused mode by listening for a +// single `readable` event and detaching afterwards. Same pattern @david/shell +// uses for stdin: leaves the stream in a clean, handoff-able state, and is +// cleanly abortable — aborting just removes the listener, with no read syscall +// left outstanding to steal a byte from the next reader. +function readTty(stream: tty.ReadStream, p: Uint8Array, signal?: AbortSignal): Promise { + return new Promise((resolve, reject) => { + const onReadable = () => { + const chunk = stream.read() as Uint8Array | null; + if (chunk === null) return; // nothing buffered yet; wait for the next event + cleanup(); + const len = Math.min(chunk.length, p.length); + p.set(chunk.subarray(0, len)); + // put any bytes we didn't consume back into the stream rather than a + // private buffer, so the next reader still sees them. + if (chunk.length > len) stream.unshift(chunk.subarray(len)); + resolve(len); + }; + const onEnd = () => { + cleanup(); + resolve(null); + }; + const onError = (err: Error) => { + cleanup(); + reject(err); + }; + const onAbort = () => { + cleanup(); + reject(signal!.reason); + }; + const cleanup = () => { + stream.off("readable", onReadable); + stream.off("end", onEnd); + stream.off("error", onError); + signal?.removeEventListener("abort", onAbort); + }; + + stream.on("readable", onReadable); + stream.on("end", onEnd); + stream.on("error", onError); + signal?.addEventListener("abort", onAbort, { once: true }); + // data may already be buffered, so attempt a read right away. + onReadable(); }); } diff --git a/src/test/aborted-prompts-visual-inspection.ts b/src/test/aborted-prompts-visual-inspection.ts new file mode 100644 index 0000000..53d7de8 --- /dev/null +++ b/src/test/aborted-prompts-visual-inspection.ts @@ -0,0 +1,59 @@ +import process from "node:process"; +import $ from "../../mod.ts"; + +// reproduces the bug where aborted maybePrompts left dangling stdin reads +// that would silently swallow keystrokes from the next prompt. +// +// before the fix: the visible prompt at the end took N+1 enters to accept +// (one per aborted prompt that had a pending read, plus the real one). +// after the fix: a single enter accepts. + +const ABORTED_COUNT = 3; + +for (let i = 0; i < ABORTED_COUNT; i++) { + const ac = new AbortController(); + // give the prompt a moment to display and call reader.read, then abort. + setTimeout(() => ac.abort(), 500); + const result = await $.maybePrompt({ + message: `prompt #${i + 1} (will be aborted)`, + signal: ac.signal, + noClear: true, + }); + $.log(` -> aborted prompt #${i + 1} resolved to:`, result); +} + +$.log( + `\nnow type something and press enter ONCE.\n` + + `pre-fix: needed ~${ABORTED_COUNT + 1} enters before this prompt accepted input.\n`, +); + +const answer = await $.prompt("final prompt"); +$.log("got:", answer); + +// after the prompts are done, stdin should be in a clean handoff-able +// state — read another line directly via process.stdin to confirm we +// haven't left listeners or a held lock that would block another reader. +$.log("\nnow type another line and press enter — read directly from process.stdin:"); +const line = await new Promise((resolve, reject) => { + let buf = ""; + const onData = (chunk: Buffer) => { + buf += chunk.toString("utf8"); + const newline = buf.indexOf("\n"); + if (newline === -1) return; + cleanup(); + resolve(buf.slice(0, newline).replace(/\r$/, "")); + }; + const onError = (err: Error) => { + cleanup(); + reject(err); + }; + const cleanup = () => { + process.stdin.off("data", onData); + process.stdin.off("error", onError); + process.stdin.pause(); + }; + process.stdin.on("data", onData); + process.stdin.on("error", onError); + process.stdin.resume(); +}); +$.log("read via process.stdin:", line);