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

doc,test: fix default maxBuffer size #22894

Closed
wants to merge 3 commits into from

Conversation

koh110
Copy link
Contributor

@koh110 koh110 commented Sep 17, 2018

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

@koh110 koh110 force-pushed the max_buffer_default branch 2 times, most recently from 760f652 to d238499 Compare September 17, 2018 12:28
doc/api/child_process.md Outdated Show resolved Hide resolved
@lovinglyy
Copy link
Contributor

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

@koh110
Copy link
Contributor Author

koh110 commented Sep 17, 2018

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

@lovinglyy
Copy link
Contributor

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

@koh110
Copy link
Contributor Author

koh110 commented Sep 25, 2018

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

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

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.

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.

@sam-github
Copy link
Contributor

@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?

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.

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

@koh110
Copy link
Contributor Author

koh110 commented Jan 7, 2019

@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

@thefourtheye
Copy link
Contributor

cc @nodejs/child_process

@koh110
Copy link
Contributor Author

koh110 commented Jan 20, 2019

@nodejs/documentation Anyone else approve?

@koh110
Copy link
Contributor Author

koh110 commented Feb 11, 2019

@nodejs/documentation Anyone else approve?

@ChALkeR
Copy link
Member

ChALkeR commented Feb 11, 2019

/cc @nodejs/documentation

@koh110
Copy link
Contributor Author

koh110 commented Mar 7, 2019

Anyone else approve?
/cc @nodejs/documentation

@koh110
Copy link
Contributor Author

koh110 commented Mar 8, 2019

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

@koh110
Copy link
Contributor Author

koh110 commented Mar 12, 2019

@thefourtheye ping

@nodejs-github-bot
Copy link
Collaborator

@koh110 koh110 force-pushed the max_buffer_default branch 2 times, most recently from 08934cf to 8ab3cb1 Compare March 28, 2019 14:04
@nodejs-github-bot
Copy link
Collaborator

@nodejs nodejs deleted a comment from nodejs-github-bot Mar 28, 2019
@koh110
Copy link
Contributor Author

koh110 commented Mar 29, 2019

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

@koh110
Copy link
Contributor Author

koh110 commented Apr 3, 2019

@sam-github ping

@BridgeAR BridgeAR changed the title doc: fix default maxBuffer size doc,test: fix default maxBuffer size Apr 3, 2019
@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

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

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 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 Outdated Show resolved Hide resolved
test/parallel/test-child-process-execfile-maxBuffer.js Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor

 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.

@richardlau
Copy link
Member

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.

@sam-github
Copy link
Contributor

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

@sam-github
Copy link
Contributor

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

@richardlau
Copy link
Member

richardlau commented Apr 3, 2019

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.

@sam-github
Copy link
Contributor

@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
Copy link
Member

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

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

koh110 and others added 2 commits April 3, 2019 11:15
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.
@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

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

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

Landed in ceb80f4

@sam-github sam-github closed this Apr 4, 2019
sam-github pushed a commit that referenced this pull request Apr 4, 2019
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 <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 4, 2019
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 <[email protected]>
sam-github pushed a commit that referenced this pull request Apr 9, 2019
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
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 <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
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 <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@koh110 koh110 deleted the max_buffer_default branch May 9, 2019 10:41
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.

8 participants