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

child_process: increase default maxBuffer to 1024*1024 bytes #11196

Conversation

seppevs
Copy link
Contributor

@seppevs seppevs commented Feb 6, 2017

child_process: increase default maxBuffer to 1024*1024

Increase the default maxBuffer for child_process.exec from 200 * 1024 bytes to 1024 KB.

This is a fix for #9829

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Feb 6, 2017
@seppevs seppevs changed the title child_process: increase default maxBuffer to 1024*1024 child_process: increase default maxBuffer to 1024*1024 bytes Feb 6, 2017
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

I'm +0 on the change... but if this passes in CI then I'm shocked that there is no test!

@sam-github sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 11, 2017
@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

@cjihrig @trevnorris ... PTAL
This would definitely need a test

@cjihrig
Copy link
Contributor

cjihrig commented Feb 14, 2017

There are a few other places in the documentation that need to be updated. I don't really have an opinion on whether this needs to be done or not, but the changes LGTM.

@trevnorris
Copy link
Contributor

the commit message will need to conform to standards, and could you include information in the referenced issue in the git commit message body? also, it should be possible to detect when the process is attempting to write more than maxBuffer. Dropping some sort of error message when this happens would be useful.

@seppevs
Copy link
Contributor Author

seppevs commented Feb 22, 2017

@trevnorris I will change the commit message.
What do you mean with 'dropping some sort of error message'? Isn't that out of scope for this ticket / PR?

@TimothyGu
Copy link
Member

Ping on this.

@seppevs seppevs force-pushed the child_process_maxBuffer_default_too_small branch from 0947cf0 to 9d9af5a Compare March 27, 2017 07:43
Increase the default maxBuffer for child_process.exec
from 200 * 1024 bytes to 1024 KB so child processes
don't get terminated too soon.

This is a fix for nodejs#9829
@seppevs seppevs force-pushed the child_process_maxBuffer_default_too_small branch from 9d9af5a to fefd78a Compare March 27, 2017 08:48
@bnoordhuis bnoordhuis added the stalled Issues and PRs that are stalled. label Aug 14, 2017
@bnoordhuis
Copy link
Member

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants