child_process: change the defaults maxBuffer size

[koh110]

I changed the defaults for all the maxBuffer sizes to a much larger value.

Refs: #23027

Requested in #23027 (review) by @mcollina

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]

cc @sam-github @mcollina

[sam-github]

Strikes me as an enormously large value, but I don't have a good feel for what value is reasonable. This could lead to sharply increased memory usage for some use cases... but then, if people care, the maxBuffer option should be used to set limits appropriate to the use.

[bnoordhuis]

See #9829 and #11196, notably #9829 (comment) - it's been discussed before.

Changing the default isn't completely out of the question but since it's intended as a containment mechanism for runaway processes, increasing it 320x from 200k to 64M seems like a tremendously bad idea.

[mcollina]

Changing the default isn't completely out of the question but since it's intended as a containment mechanism for runaway processes, increasing it 320x from 200k to 64M seems like a tremendously bad idea.

We had Infinity before. This is smaller. From a user point of view, if we are executing a binary and it suddenly got output truncated because of a Node.js update, it would be a bad user experience.

[koh110]

The option was set to Infinity only for spawnSync. This PR changed option of exec too. It is bad point in this PR. But I think that exec and spawnSync should be set same value.

I found this test. How about 1000 * 1024?
https://github.com/nodejs/node/blob/master/test/pummel/test-child-process-spawn-loop.js

I think changing Infinity to 200 * 1024 make a major impact.
Would I check value that CITGM passed?

[sam-github]

I doubt that citgm would find these things, its likely going to only show up at load (not that more testing is bad).

But I think that exec and spawnSync should be set same value.

Definitely.

I like the 1 meg suggestion (#9829 (comment)).

Its really hard to get these numbers right, I'd also be OK with no limit, or a child_process.DEFAULT_MAX_BUFFER that was writable.

[sam-github]

To follow on, 1meg is only 5 times larger, which seems to deal with output that is larger than a normal hand-created text file, more log-file size. Bigger enough it may cover some more use cases, but not so big its useless as protection against a run-away process, and not so controversial as removing all limits, and letting the only limit be node's ability to allocate memory (aka "infininite").

[koh110]

I like this suggestion too. I think It is suitable.

I like the 1 meg suggestion

@bnoordhuis How about this idea?

maxBuffer should not be setted Infinity for run-away process. But I think it is necessary to set maxBuffer to a higher value, because changing value of spawnSync give big impact.

[bnoordhuis]

1M seems like a good middle ground.

[koh110]

@bnoordhuis @sam-github @mcollina I changed 64 * 1024 * 1024 to 1024 * 1024. Would you check it?

[BridgeAR]

Changing a default should likely be semver-major.

[sam-github]

Changing a default should likely be semver-major.

I'm on the fence about that. Its increasing the maxBuffer, is it a breaking change that some apps will stop failing? But then, it will use more memory. I guess there is no hurry to land in 12.x, it'll be in 13.x in a month.

[koh110]

I think it should be landed in 12.x. Because, it will have an impact on developers who are using over 200 * 1024.
I think that we should decrease impact with this version up.

[bnoordhuis]

LGTM. Semver-major seems appropriate.

[sam-github]

@BethGriggs @nodejs/tsc Should this land in 12.x? Its not a complex change, so I think its low risk of instability, even though it is late in 12.x rc cycle.

[nodejs-github-bot]

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

[BethGriggs]

@sam-github, @mhdawson suggested that this should land with #23027 (which is not in v12.x yet). I'm +1 on them both going into 12.x.

[sam-github]

@mcollina @nodejs/tsc: this requires 2 TSC approvals, it currently has 0

[mhdawson]

LGTM

[mhdawson]

@sam-github there are 2 now.

[sam-github]

Landed in 652877e