child_process: use non-infinite maxBuffer defaults

[koh110]

I fixed the default behavior of maxBuffer, because the document is written 200 * 1024.
Currently the default maxBuffer value is Infinity in execSync, execFileSync and spawnSync.

Refs: #22894

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.

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]

I changed default value. How about this one ?
I think that is breaking change. Should it be reviewed by tsc?

/cc @BridgeAR @thefourtheye @lovinglyy

[thefourtheye]

cc @nodejs/child_process

[koh110]

@nodejs/tsc This is semver-major. Please review it.

[sam-github]

I recall the default was very deliberately changed to infinity. Some history checks might be needed, perhaps its the docs that need changing, not the code?

[koh110]

@sam-github
At first, I have same idea. I change doc in this PR.
#22894

We discussed this PR and my idea was changed.
I think that allow Infinity and default: Infinity are different.
What do you think about?

I recall the default was very deliberately changed to infinity

Is this what you talk about this?
#10769

I checked some history.

At first, maxBuffer of spawnSync was not written. It might allow Infinity.
e8df267

I think that this PR was unfit for spawnSync. Is it correct?
#11791

[koh110]

I fixed conflict. Which do you think should be "breaking change" or "change docs"? @nodejs/tsc
I think it should be "breaking change".

[koh110]

@sam-github bump

[sam-github]

I agree that this is semver-major, even though its a bug fix, and I reviewed the links you posted, thanks.

Personally, I think it would make more sense to land #22894 first, because it corrects the docs, and adds tests for the current behaviour, tests that look to be substantially similar to the tests you are adding in this PR. #22894 change can be backported to 11.x (and further, as required - it would be helpful if you could make clear how far back the change should be backported).

Once its landed, since it added lots of tests (thanks!), I would then rebase this PR, the one that makes the code behave as it should (and as it used to be documented) onto master after #22894. It looks to me like it would keep much of the tests from #22894, but just change them to check different defaults. As a semver-major, it would not get released on any current release lines, but that's OK, it would become how node works in 12.x.

Btw, could you (or whoever lands this) please update the commit description and message to be more informative and grammatical? I would suggest something like:

child_process: use non-infinite maxBuffer defaults

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.

[sam-github]

I kicked ci off on #22894, could you please resolve the conflicts, and I'll kick CI on this.

[koh110]

@sam-github Thanks. I resolved the conflicts. But Travis throw timeout. Would you kick CI on this?

[nodejs-github-bot]

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

[koh110]

@sam-github CI passed! After kicked CI and passed on #22894, would you merge it?