child_process: use non-infinite maxBuffer defaults by koh110 · Pull Request #23027 · nodejs/node (original) (raw)

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.