Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All versions of npm init hang on Node 8.1.0 #13557

Closed
iarna opened this issue Jun 8, 2017 · 19 comments
Closed

All versions of npm init hang on Node 8.1.0 #13557

iarna opened this issue Jun 8, 2017 · 19 comments
Labels
confirmed-bug Issues with confirmed bugs. readline Issues and PRs related to the built-in readline module. regression Issues related to regressions.

Comments

@iarna
Copy link
Member

iarna commented Jun 8, 2017

  • Version: 8.1.0
  • Platform: Darwin
  • Subsystem: Unsure

To reproduce, with Node 8.1.0 and any version of npm (we've explicitly tested w/ 2, 3, 4 & 5):

$ npm init
This utility will walk you through creating a package.json file.
It only covers the most common items, and tries to guess sensible defaults.

See `npm help json` for definitive documentation on these fields
and exactly what they do.

Use `npm install <pkg> --save` afterwards to install a package and
save it as a dependency in the package.json file.

Press ^C at any time to quit.
name: (x) 
version: (1.2.0) 

At the prompt for version Node stops accepting input and doesn't respond to ^Z or ^C. (It does respond to kill -STOP and kill -INT. Continuing the process after a STOP results in keyboard input being echoed but the process still does not run.)

@addaleax addaleax added the confirmed-bug Issues with confirmed bugs. label Jun 8, 2017
@krokofant
Copy link

yarn behaves the same

@mscdex mscdex added the v8.x label Jun 8, 2017
@addaleax
Copy link
Member

addaleax commented Jun 8, 2017

This was caused by 81ddeb9, /cc @gibfahn @jasnell

@mscdex I’m removing the v8.x label because this is an issue on master as well.

@addaleax addaleax added readline Issues and PRs related to the built-in readline module. and removed v8.x labels Jun 8, 2017
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 8, 2017

I can't seem to reproduce on master or 8.1.0?

The WARN does seem to be on the wrong line though?

Press ^C at any time to quit.
package name: (npm-test) npm WARN init canceled
🐟  ~/D/h/npm-test>

@Fishrock123
Copy link
Contributor

I'd be curious to see what e.g. the following patch prints pre/post 81ddeb9

diff --git a/lib/readline.js b/lib/readline.js
index 6a1ed150d7..6113846c91 100644
--- a/lib/readline.js
+++ b/lib/readline.js
@@ -1039,6 +1039,8 @@ function emitKeypressEvents(stream, iface) {
   } else {
     stream.on('newListener', onNewListener);
   }
+  iface.on('close', () => process._rawDebug('## CLOSE'))
+  stream.on('data', () => process._rawDebug('## DATA'))
   if (iface) {
     iface.once('close', () => { stream.removeListener('data', onData); });
   }

@addaleax
Copy link
Member

addaleax commented Jun 8, 2017

@Fishrock123 I’m not sure I’m reading your output correctly – you first have to enter the package name, only after that I can’t quite anymore.

@iarna
Copy link
Member Author

iarna commented Jun 8, 2017

@addaleax lol! I opened this issue and turned to @zkat and said "I bet Anna sees this and knows exactly the cause"

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 8, 2017

Oops. 🙊

It appears a close is emitted after pressing enter under normal circumstances...

package name: (npm-test) ## DATA

## CLOSE
version: (0.0.0) ## DATA

description: ## DATA
npm WARN init canceled

@addaleax
Copy link
Member

addaleax commented Jun 8, 2017

@Fishrock123 Apparently, npm init (or rather the read module) creates a new readline instance per question that it asks…

@jasnell You’re right, reverting is a bit silly, this is easy to fix (in a somewhat obvious way, in hindsight): okay, no, the original patch was incorrect and should be reverted.

--- a/lib/readline.js
+++ b/lib/readline.js
@@ -1040,7 +1040,11 @@ function emitKeypressEvents(stream, iface) {
     stream.on('newListener', onNewListener);
   }
   if (iface) {
-    iface.once('close', () => { stream.removeListener('data', onData); });
+    iface.once('close', () => {
+      stream[KEYPRESS_DECODER] = null;
+      stream[ESCAPE_DECODER] = null;
+      stream.removeListener('data', onData);
+    });
   }
 }

I’ll have a PR and tests up in a bit.

@iarna Naaaaw! 💙

@Fishrock123
Copy link
Contributor

Apparently, npm init (or rather the read module) creates a new readline instance per question that it asks…

While that seems inefficient, readline does apparently emit close at the end of each line: https://nodejs.org/dist/latest-v8.x/docs/api/readline.html#readline_event_close

(Which leads me to believe the patch above may be a bit inefficient too? We can discuss in a PR.)

@addaleax
Copy link
Member

addaleax commented Jun 8, 2017

While that seems inefficient, readline does apparently emit close at the end of each line: https://nodejs.org/dist/latest-v8.x/docs/api/readline.html#readline_event_close

Are you sure? I don’t really read the docs as stating that…

@jasnell
Copy link
Member

jasnell commented Jun 8, 2017

That definitely seems like a bug then. It really should not be emitting close at that point. Let's see if we can fix that issue quickly before reverting the other commit

@addaleax
Copy link
Member

addaleax commented Jun 8, 2017

It really should not be emitting close at that point.

It doesn’t, it’s the read module that closes the readline instance explicitly after having read the answer.

@addaleax
Copy link
Member

addaleax commented Jun 8, 2017

PR to fix/revert: #13560

@131
Copy link

131 commented Jun 10, 2017

Following sample code is broken, waiting for you @addaleax !

const readline = require('readline')

function prompt(opts, cb) {
  var input = process.stdin
  var output = process.stdout
  var terminal = !!(output.isTTY)
  var rlOpts = { input, output, terminal }
  var rl = readline.createInterface(rlOpts)
  rl.setPrompt(opts.prompt)
  rl.prompt()
  rl.on('line', function(line){
    rl.close()
    cb(null, line);
  })
}
const foo = function() {
  prompt( {prompt : "#>" }, function(err, line ){
    console.log("Got", err, line);
    foo();
  })
}

foo();

@gibfahn
Copy link
Member

gibfahn commented Jun 10, 2017

@131 see #13560 (comment), it'll be fixed in the next v8.x release, scheduled for next Tuesday.

addaleax added a commit that referenced this issue Jun 10, 2017
Fixes: #13557
PR-URL: #13560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Bamieh
Copy link
Contributor

Bamieh commented Jun 29, 2017

i'm still facing this issue guys

@addaleax
Copy link
Member

@Bamieh As described above, upgrade Node to the latest version and you will be fine.

@Bamieh
Copy link
Contributor

Bamieh commented Jun 29, 2017

@addaleax true, it gets fixed when upgrading node, i only upgraded npm before and it didnt work. thanks

@gezquinndesign
Copy link

This is still affecting the loopback-cli

It freezes after giving the application name...

node version - v10.7.0
npm version=6.1.0
loopback- 4.2.0
OS- windows10

anyone help please?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. readline Issues and PRs related to the built-in readline module. regression Issues related to regressions.
Projects
None yet
Development

No branches or pull requests