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

[rush-lib] increase maxBuffer to 10mb when executeCommand #3105

Merged
merged 2 commits into from
Jan 5, 2022

Conversation

F3n67u
Copy link
Contributor

@F3n67u F3n67u commented Dec 21, 2021

Summary

The default maxBuffer 1MB is too small.

we should increase to decrease the ENOBUFS error when executing git command. for example:

rush build --from git:master will throw ENOBUFS error when pnpm-lock.yaml is larger than 1MB.

rush build --from git:master execute git cat-file blob common/config/rush/pnpm-lock.yaml command using child_process.spawnSync. because pnpm-lock.yaml is larger than 1MB so the ENOBUFS is throwed.

$ ls -alh common/config/rush/pnpm-lock.yaml
-rw-r--r--  1 feng  staff   1.9M Dec 21 11:22 common/config/rush/pnpm-lock.yaml

$ rush --debug build --from git:master


Rush Multi-Project Build Tool 5.58.0 - https://rushjs.io
Node.js version is 14.18.1 (LTS)


Trying to acquire lock for pnpm-6.10.0
Acquired lock for pnpm-6.10.0
Found pnpm version 6.10.0 in /Users/feng/.rush/node-v14.18.1/pnpm-6.10.0

Symlinking "/Users/feng/Projects/test-infra-monorepo/common/temp/pnpm-local"
  --> "/Users/feng/.rush/node-v14.18.1/pnpm-6.10.0"
Acquiring lock for "common/autoinstallers/command-plugins" folder...
Autoinstaller folder is already up to date

Starting "rush build"


ERROR: spawnSync /bin/sh ENOBUFS


Error: spawnSync /bin/sh ENOBUFS


    at Object.spawnSync (internal/child_process.js:1077:20)
    at Object.spawnSync (child_process.js:776:24)
    at Function._executeCommandInternal (/Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/utilities/Utilities.js:546:36)
    at Function.executeCommandAndCaptureOutput (/Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/utilities/Utilities.js:199:34)
    at Git._executeGitCommandAndCaptureOutput (/Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/logic/Git.js:464:42)
    at Git.getBlobContent (/Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/logic/Git.js:218:29)
    at ProjectChangeAnalyzer.getChangedProjectsAsync (/Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/logic/ProjectChangeAnalyzer.js:168:57)
    at GitChangedProjectSelectorParser.evaluateSelectorAsync (/Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/logic/selectors/GitChangedProjectSelectorParser.js:13:44)
    at SelectionParameterSet._evaluateProjectParameterAsync (/Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/cli/SelectionParameterSet.js:276:49)
    at /Users/feng/.rush/node-v14.18.1/rush-5.58.0/node_modules/@microsoft/rush-lib/lib/cli/SelectionParameterSet.js:166:25

Details

There is a issue about default maxBuffer is too small in node. the popular execa package is also using 10MB maxBuffer as a default when start(now they increase to 100MB). I think 10MB will be a more reasonable default for rush

How it was tested

local. using the project above which rush build --from git:master fail.

$ node ~/Projects/rushstack/apps/rush-lib/lib/start.js --debug build --from git:master

Rush Multi-Project Build Tool 5.58.0 (unmanaged) - https://rushjs.io
Node.js version is 14.18.1 (LTS)


Trying to acquire lock for pnpm-6.10.0
Acquired lock for pnpm-6.10.0
Found pnpm version 6.10.0 in /Users/bytedance/.rush/node-v14.18.1/pnpm-6.10.0

Symlinking "/Users/bytedance/Projects/test-infra-monorepo/common/temp/pnpm-local"
  --> "/Users/bytedance/.rush/node-v14.18.1/pnpm-6.10.0"
Acquiring lock for "common/autoinstallers/command-plugins" folder...
Autoinstaller folder is already up to date

Starting "rush build"

Executing a maximum of 12 simultaneous processes...

==[ project  1 ]==========================[ 1 of 32 ]==

@iclanton
Copy link
Member

Are there performance implications when the maxBuffer is set to a large value than necessary? Should this option only be set when Rush grabs the lockfile from Git, and not for every command invocation?

@F3n67u
Copy link
Contributor Author

F3n67u commented Dec 25, 2021

Are there performance implications when the maxBuffer is set to a large value than necessary?

i think nope. maxbuffer is just a limit. it will not increase the memory usage if child process only use small memory. correct me if i am wrong. see nodejs/node#9829 (comment)

Should this option only be set when Rush grabs the lockfile from Git, and not for every command invocation?

@iclanton if this option only set when rush grab lockfile from git, other git command exection will still suffer from small buffer problem. I also encounter ENOBUFS error when rush invoke git ls-files command (git ls-files output many untracked files). see

private _getUntrackedChanges(): string[] {

instead of fix the ENOBUFS case by case. i think we would better make 10mb as a default. this solution will decrease ENOBUFS error for all git command execution.

@lianghx-319
Copy link

#3106

@lianghx-319
Copy link

Maybe provide an environment variable RUSH_CHILD_PROCESS_MAX_BUFFER is a better solution? People who suffers from this issue can set maxBuffer via environment variable.

@iclanton iclanton merged commit b79967b into microsoft:master Jan 5, 2022
@F3n67u F3n67u deleted the feat/increase-maxBuffer branch January 5, 2022 07:45
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.

3 participants