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

child_process: measure buffer length in bytes #7381

Closed
wants to merge 8 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 23, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

child_process

Description of change

This re-implements a previous fix that had to be reverted. The fix is to measure the maxBuffer option of child_process.exec() in bytes (per the documentation and user expectations) and not characters.

Unfortunately, the fix had a side effect such the child.stdout and child.stderr data events sent their callbacks buffers by default rather than strings. This is undocumented but fully exposed and caused bugs in userland (e.g., https://github.com/tes/bosco/issues/152).

This PR depends on #7377. It re-implements the change, but this time it is marked semver-major and tests are added to confirm the behavior change.

@Trott Trott added child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 23, 2016
@@ -20,7 +20,7 @@ function main(conf) {
const len = +conf.len;

const msg = '"' + Array(len).join('.') + '"';
const options = { 'stdio': ['ignore', 'pipe', 'ignore'] };
const options = {'stdio': ['ignore', 'ipc', 'ignore']};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge error on my part that undoes a change that came in after the commit I reverted. Will fix it momentarily...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Jun 23, 2016
Trott added 8 commits June 23, 2016 10:55
This change fixes a known issue where `maxBuffer` limits by characters
rather than bytes. Benchmark added to confirm no performance regression
occurs with this change.

This necessarily changes default behavior of `stdout` and `stderr`
callbacks such that they receive buffers rather than strings. The
alternative would be a performance hit on the defaults.

Refs: nodejs#6764
Refs: nodejs#1901
@Trott
Copy link
Member Author

Trott commented Jun 30, 2016

Closing in favor of non-semver-major #7391

@Trott Trott closed this Jun 30, 2016
@Trott Trott deleted the major-general branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants