Skip to content

repl: disable blocking completions by default#36564

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.

Sign up for GitHub

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

Jump to bottom
Closed
addaleax wants to merge 1 commit into from
Closed

repl: disable blocking completions by default #36564

addaleax wants to merge 1 commit into from

Conversation

@addaleax
Copy link
Member

@addaleaxaddaleax 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)
@addaleaxaddaleax added repl Issues and PRs related to the REPL subsystem. lts-watch-v14.x labels Dec 18, 2020
@addaleaxaddaleax 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
MemberAuthor

@addaleaxaddaleaxDec 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.

@addaleaxaddaleax 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-actionsgithub-actionsbot 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
MemberAuthor

addaleax commented Dec 21, 2020

Landed in 82dd23f

@addaleaxaddaleax 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]>
@targostargos 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]>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@cjihrigcjihrigcjihrig approved these changes

@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.replIssues 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