doc,test: fix default maxBuffer size by koh110 · Pull Request #22894 · nodejs/node · GitHub

Fixed document default maxBuffer size at spawnSync. I changed 200 * 1024 to Infinity.
And add test to spawnSync, exec and execFile according to the document.

I think that maxBuffer size should be 200 * 1024 at spawnSync, but comment said that maxBuffer size allowed Infinity on test/parallel/test-child-process-spawnsync-maxbuf.js. And it is working now.

Therefore, I just change document and add test.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

That test only verify if maxBuffer will allow Infinity but the default is still 200 * 1024.

Doesn't it throw error if buffer size over default ?
I think that 'a'.repeat(200 * 1024 + 1) is over default maxBuffer.

I thought that it was cause the size allocated, but actually it is accepting really long buffers and if surpass extremely large values, the buffer gets a small size again, it should be fixed instead of documented imo

Thank you for all review. I created new PR that work default maxBuffer.
#23027

Please review it.
/cc @BridgeAR @thefourtheye @lovinglyy

LGTM. See #23027 (comment) for discussion.

@koh110 or whoever lands this: please add this information from the PR description to the commit message body before landing:

Correctly document the default maxBuffer size for execSync, execFileSync, and spawnSync. It is 200 * 1024, not Infinity. Add tests to verify behaviour is as documented.

@koh110 Are you sure the documentation changes here are complete? Since execSync and execFileSync call spawnSync, doesn't their documentation need to be changed as well?

Can you manually confirm that these tests pass as-is, but fail when the default is changed, as it is in #23027?

@sam-github execSync and execFileSync document need to be changed. Thank for your pointing out. I fixed it.

This code is passed Node.js v10.15.0.

const args = ['-e', "console.log('a'.repeat(200 * 1024))"];
spawnSync(process.execPath, args);

It will fail when the default is changed. This test shows it.
https://github.com/nodejs/node/pull/23027/files#diff-acbe1702bfaca50628c88dc64e0b8e7eR41

cc @nodejs/child_process

@nodejs/documentation Anyone else approve?

/cc @nodejs/documentation

Anyone else approve?
/cc @nodejs/documentation

@thefourtheye If you don't have other comments, would you approve?

@thefourtheye ping

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

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

@sam-github CI failed, but test is not wrong. This reason is timeout on Travis. Would you kick CI?

The job exceeded the maximum time limit for jobs, and has been terminated.

@sam-github ping

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

Failed on travis, looked like a timeout, test may have ran too long. I restarted. Is there any chance the tests you added to prove that the maxBuffer is "infinite" are excessively time/cpu/memory consuming?

https://travis-ci.com/nodejs/node/jobs/188673708

I'm building locally, to get a sense for the performance of this. It might use too much data. I might see if the console.logs that write giant strings can be written in a way that runs faster, or uses less memory.

We can't merge tests that make travis ci flaky. There is another set of tests, test/pummel, that some of these could perhaps go into if its not stable in test/parallel. They get run less frequently, but would still serve to check if the default behaviour changed.

 test/parallel/test-child-process-exec-maxBuffer.js     | 23 ++++++
 test/parallel/test-child-process-execfile-maxBuffer.js | 94 ++++++++++++++++++++++++
 .../parallel/test-child-process-execfilesync-maxbuf.js | 46 ++++++++++++
 test/parallel/test-child-process-execsync-maxbuf.js    | 37 ++++++++++
 test/parallel/test-child-process-spawnsync-maxbuf.js   |  8 ++

Please lower-case the filenames, and use consistent suffixes: change the maxBuffer to maxbuf.

Failed on travis, looked like a timeout, test may have ran too long. I restarted. Is there any chance the tests you added to prove that the maxBuffer is "infinite" are excessively time/cpu/memory consuming?

https://travis-ci.com/nodejs/node/jobs/188673708

There was an issue with Travis builds in general timing out that was fixed a few days ago in #27002.

Builds tend to take 24-30 minutes, see https://travis-ci.com/nodejs/node/builds

https://travis-ci.com/nodejs/node/builds/106252842 for this PR is up to 50

Btw, node -p '"a".repeat(50)' might be more concise.

Builds tend to take 24-30 minutes, see https://travis-ci.com/nodejs/node/builds

https://travis-ci.com/nodejs/node/builds/106252842 for this PR is up to 50

The Travis build for this PR was timing out compiling V8/Node.js and didn't even start running the tests. This is unlikely to be caused by this PR, which only touches documentation and tests.

I've wiped out the cache in Travis for this PR so that it instead it picks up the cache for the master branch and restarted the build. It's in progress but looks to be much faster (compilation has actually succeeded and it's started running tests now).

Update: Travis build passed.

@richardlau so you think this is stable enough to merge?

I think it needs one more approval, @nodejs/child_process , though since its been open for much more than 7 days, maybe my approval alone is sufficient?

@richardlau so you think this is stable enough to merge?

From the point of view of not destabilizing the build, yes.

Correctly document the default maxBuffer size for execSync, execFileSync, and spawnSync. It is 200 * 1024, not Infinity. Add tests to verify behaviour is as documented.

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

@koh110 I pushed my suggested changes. I'll merge this when CI is green. Thanks for your patience.

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

Landed in ceb80f4

Correctly document the default maxBuffer size for execSync, execFileSync, and spawnSync. It is 200 * 1024, not Infinity. Add tests to verify behaviour is as documented. PR-URL: #22894 Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Set the default maxBuffer size to 204,800 bytes for execSync, execFileSync, and spawnSync. APIs that return the child output as a string should have non-infinite defaults for maxBuffer sizes to avoid out-of-memory error conditions. A non-infinite default used to be the documented behaviour for all relevant APIs, but the implemented behaviour for execSync, execFileSync and spawnSync was to have no maxBuffer limits. PR-URL: #23027 Refs: #22894 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Correctly document the default maxBuffer size for execSync, execFileSync, and spawnSync. It is 200 * 1024, not Infinity. Add tests to verify behaviour is as documented. PR-URL: #22894 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

koh110 force-pushed the max_buffer_default branch 2 times, most recently from 760f652 to d238499 Compare September 17, 2018 12:28

BridgeAR reviewed Sep 17, 2018

View reviewed changes

Comment thread doc/api/child_process.md Outdated

koh110 force-pushed the max_buffer_default branch from d238499 to d49e5aa Compare September 17, 2018 13:33

koh110 mentioned this pull request Sep 22, 2018

child_process: use non-infinite maxBuffer defaults #23027

Closed

4 tasks

sam-github approved these changes Nov 26, 2018

View reviewed changes

sam-github suggested changes Nov 26, 2018

View reviewed changes

koh110 force-pushed the max_buffer_default branch from d49e5aa to 8867efd Compare January 7, 2019 14:36

sam-github approved these changes Jan 7, 2019

View reviewed changes

koh110 force-pushed the max_buffer_default branch from 1ba4c1d to b297c71 Compare February 11, 2019 14:22

thefourtheye reviewed Feb 13, 2019

View reviewed changes

koh110 force-pushed the max_buffer_default branch from b297c71 to 37bf7c8 Compare February 18, 2019 09:52

koh110 force-pushed the max_buffer_default branch 2 times, most recently from 08934cf to 8ab3cb1 Compare March 28, 2019 14:04

nodejs deleted a comment from nodejs-github-bot Mar 28, 2019

BridgeAR changed the title doc: fix default maxBuffer size doc,test: fix default maxBuffer size Apr 3, 2019

sam-github reviewed Apr 3, 2019

View reviewed changes

Comment thread test/parallel/test-child-process-exec-maxBuffer.js Outdated

Comment thread test/parallel/test-child-process-execfile-maxBuffer.js Outdated

koh110 and others added 2 commits April 3, 2019 11:15

doc: fix default maxBuffer size

34d937d

Correctly document the default maxBuffer size for execSync, execFileSync, and spawnSync. It is 200 * 1024, not Infinity. Add tests to verify behaviour is as documented.

fixup! doc: fix default maxBuffer size

f605ba9

sam-github force-pushed the max_buffer_default branch from 2cdf3fa to f605ba9 Compare April 3, 2019 18:47

fixup! doc: fix default maxBuffer size

e2a0521

sam-github approved these changes Apr 4, 2019

View reviewed changes

sam-github closed this Apr 4, 2019

BethGriggs mentioned this pull request Apr 9, 2019

v11.14.0 proposal #27163

Merged

koh110 deleted the max_buffer_default branch May 9, 2019 10:41

doc,test: fix default maxBuffer size by koh110 · Pull Request #22894 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc,test: fix default maxBuffer size - #22894

doc,test: fix default maxBuffer size#22894koh110 wants to merge 3 commits intonodejs:masternodejs/node:masterfrom koh110:max_buffer_defaultkoh110/node:max_buffer_defaultCopy head branch name to clipboard

Conversation

koh110 commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

lovinglyy commented Sep 17, 2018

Uh oh!

koh110 commented Sep 17, 2018

Uh oh!

lovinglyy commented Sep 18, 2018

Uh oh!

koh110 commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Nov 26, 2018

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

koh110 commented Jan 7, 2019

Uh oh!

thefourtheye commented Jan 7, 2019

Uh oh!

koh110 commented Jan 20, 2019

Uh oh!

koh110 commented Feb 11, 2019

Uh oh!

ChALkeR commented Feb 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

koh110 commented Mar 7, 2019

Uh oh!

koh110 commented Mar 8, 2019

Uh oh!

koh110 commented Mar 12, 2019

Uh oh!

nodejs-github-bot commented Mar 28, 2019

Uh oh!

nodejs-github-bot commented Mar 28, 2019

Uh oh!

koh110 commented Mar 29, 2019

Uh oh!

koh110 commented Apr 3, 2019

Uh oh!

nodejs-github-bot commented Apr 3, 2019

Uh oh!

sam-github commented Apr 3, 2019

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sam-github commented Apr 3, 2019

Uh oh!

richardlau commented Apr 3, 2019

Uh oh!

sam-github commented Apr 3, 2019

Uh oh!

sam-github commented Apr 3, 2019

Uh oh!

richardlau commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Apr 3, 2019

Uh oh!

richardlau commented Apr 3, 2019

Uh oh!

doc,test: fix default maxBuffer size#22894
koh110 wants to merge 3 commits into
nodejs:masterfrom
koh110:max_buffer_default

koh110 commented Sep 17, 2018 •
edited
Loading

koh110 commented Sep 25, 2018 •
edited
Loading

richardlau commented Apr 3, 2019 •
edited
Loading