Skip to content

repl: disable blocking completions by default#36564

Closed
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:no-repl-blocking-completion
Closed

repl: disable blocking completions by default#36564
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:no-repl-blocking-completion

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Dec 18, 2020 •
edited
Loading

It’s not okay for the REPL to be blocked for multiple seconds after
entering require(' because the completion is performing blocking
fs operations on potentially huge directories. Turning the REPL
completion function asynchronous would be the right thing to do here,
but unfortunately the way the code is structured doesn’t play well
with that (in particular, it breaks the preview feature).
Therefore, disable these blocking calls by default.

Refs: #33282 (comment)

Example:

$ ls node_modules | wc
   1161    1161   14776
$ sync; echo 3 | sudo tee /proc/sys/vm/drop_caches
3
$ echo 'console.time(); repl.repl.complete(`require("`, () => {}); console.timeEnd();'|node -i
Welcome to Node.js v16.0.0-pre.
Type ".help" for more information.
> console.time(); repl.repl.complete(`require("`, () => {}); console.timeEnd();
default: 6.768s
undefined
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax
repl: disable blocking completions by default
200ae1d 
It’s not okay for the REPL to be blocked for multiple seconds after
entering `require('` because the completion is performing blocking
fs operations on potentially huge directories. Turning the REPL
completion function asynchronous would be the right thing to do here,
but unfortunately the way the code is structured doesn’t play well
with that (in particular, it breaks the preview feature).
Therefore, disable these blocking calls by default.

Refs: nodejs#33282 (comment)
@addaleax addaleax added repl Issues and PRs related to the REPL subsystem. lts-watch-v14.x labels Dec 18, 2020
@addaleax addaleax mentioned this pull request Dec 18, 2020
Closed
4 tasks
addaleax
addaleax commented Dec 18, 2020
View reviewed changes
lib/repl.js
configurable: true
});

this.allowBlockingCompletions = !!options.allowBlockingCompletions;
Copy link
Member Author

@addaleax addaleax Dec 18, 2020

Choose a reason for hiding this comment

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

I’m not publicly documenting this because this is a) supposed to be temporary and b) that would make it semver-minor, and this should be backported to v14.x without the extensive process that the LTS team has for semver-minor commits.

@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 18, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 18, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/34999/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 19, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/35007/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 21, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/35020/

@addaleax
Copy link
Member Author

addaleax commented Dec 21, 2020

Landed in 82dd23f

@addaleax addaleax closed this Dec 21, 2020
@addaleax addaleax deleted the no-repl-blocking-completion branch December 21, 2020 11:56
addaleax added a commit that referenced this pull request Dec 21, 2020
@addaleax
repl: disable blocking completions by default
82dd23f 
It’s not okay for the REPL to be blocked for multiple seconds after
entering `require('` because the completion is performing blocking
fs operations on potentially huge directories. Turning the REPL
completion function asynchronous would be the right thing to do here,
but unfortunately the way the code is structured doesn’t play well
with that (in particular, it breaks the preview feature).
Therefore, disable these blocking calls by default.

Refs: #33282 (comment)

PR-URL: #36564
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Dec 21, 2020
@addaleax @targos
repl: disable blocking completions by default
7c903ec 
It’s not okay for the REPL to be blocked for multiple seconds after
entering `require('` because the completion is performing blocking
fs operations on potentially huge directories. Turning the REPL
completion function asynchronous would be the right thing to do here,
but unfortunately the way the code is structured doesn’t play well
with that (in particular, it breaks the preview feature).
Therefore, disable these blocking calls by default.

Refs: #33282 (comment)

PR-URL: #36564
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos mentioned this pull request Dec 22, 2020
Merged
danielleadams pushed a commit that referenced this pull request Apr 29, 2021
@addaleax @danielleadams
repl: disable blocking completions by default
1ac07b2 
It’s not okay for the REPL to be blocked for multiple seconds after
entering `require('` because the completion is performing blocking
fs operations on potentially huge directories. Turning the REPL
completion function asynchronous would be the right thing to do here,
but unfortunately the way the code is structured doesn’t play well
with that (in particular, it breaks the preview feature).
Therefore, disable these blocking calls by default.

Refs: #33282 (comment)

PR-URL: #36564
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@addaleax @nodejs-github-bot @Trott @benjamingr @cjihrig @aduh95 @targos