Skip to content

fix: settle the promise when the input stream ends - #157

Open
1yakub wants to merge 2 commits into
npm:mainfrom
1yakub:fix/settle-on-eof
Open

1yakub wants to merge 2 commits into
npm:mainfrom
1yakub:fix/settle-on-eof

Conversation

@1yakub

@1yakub 1yakub commented Aug 8, 2026

Copy link
Copy Markdown

read() returns a promise that never settles if the input stream ends before a line arrives. There is a handler for line, one for error and one for SIGINT, but none for close, and readline emits close on its own the moment the input hits EOF. Nothing rejects, nothing resolves, so the caller waits on a promise that can never finish. Once nothing else is holding the event loop the process just exits, and whatever was waiting is gone with no error anywhere.

That happens any time stdin is not something a person can type into, which is common: < /dev/null, an already closed pipe, a child process spawned without stdin, most CI shells.

Where I hit it: npm login in a non interactive shell. npm prompts for a username, the prompt can never be answered, and npm exits 1 printing no error at all. Its own debug log says "Exit handler never called! This is an error with npm itself", which is npm noticing that the command promise never resolved. Repro with npm 12.0.2 on Node 22.23.2:

npm login --auth-type=legacy < /dev/null
# prints "Username:", exits 1, no error message

Proof that the promise is the thing left hanging, run against npm's own bundled copy of read:

const { read } = require('read')
let settled = false
process.on('exit', c => console.log(`exit=${c} settled=${settled}`))
read({ prompt: 'Username:' }).then(() => settled = true, () => settled = true)
$ node t.js < /dev/null
Username: exit=0 settled=false

The fix adds the missing close handler and rejects with canceled, the same error SIGINT already uses, since both mean the same thing: no answer is coming. A finished flag keeps the deliberate rl.close() in done() from rejecting a promise that already resolved.

Two tests: one that a stream ending with no line rejects, and one that a stream ending after a line still resolves, which is the case that would break if the close handler fired unconditionally. Without the source change the first test hangs until the runner cancels it and the suite exits 1; with it the suite is 6/6 and lint is clean.

Checked with npm login too: patching only this package inside an installed npm 12.0.2 turns that silent exit 1 into npm error canceled.

Written with AI assistance (Claude Code); the repro, the fix and the test runs are my own and verified locally.

@1yakub

1yakub commented Aug 14, 2026

Copy link
Copy Markdown
Author

Bump. The workflows here are still waiting on first time contributor approval, so CI hasn't run. The fix settles the promise when stdin ends without an answer, two tests cover it.

Comment thread src/read.ts Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new close listener makes this explicit rl.close() reentrant.readline.close() emits close synchronously while finished is still false, so the new listener calls onError()/done(). After that returns, this handler calls onError() again, causing cleanup, including m.end() to run twice.

onError() already calls done(), and done() sets finished = true before calling rl.close().
Could we remove this explicit close?

rl.on('SIGINT', () => {
  onError(new Error('canceled'))
})

Please also add a SIGINT regression test so this interaction remains covered.

@1yakub

1yakub commented Sep 4, 2026

Copy link
Copy Markdown
Author

Good catch on the reentrancy. Dropped the explicit close in the SIGINT handler, so onError -> done -> rl.close is the only path now, and removed the c8 ignore since the path is covered. Added a SIGINT test: ctrl+c through readline in terminal mode rejects with canceled, and a fresh read right after still works. Suite is green locally on node 24.

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.

2 participants