child_process: preserve argument type #7391

Trott · 2016-06-23T18:43:08Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

A previous fix for a maxBuffer bug resulted in a change to the
argument type for the data event on child.stdin and child.stdout
when using child_process.exec().

This fixes the maxBuffer bug in a way that does not have that side
effect.

Fixes: #7342
Refs: #1901

Alternative to #7381 (which changes the argument type and is arguably semver-major for that reason--being affected by it requires relying on undocumented behavior and using exec() in ways that are better suited to fork() or spawn() which are unaffected by either fix).

mscdex · 2016-06-23T19:08:26Z

lib/child_process.js

-      stdoutLen += chunk.byteLength;
+      stdoutLen += chunk instanceof Buffer ?
+        chunk.length :
+        Buffer.byteLength(chunk, encoding);


Can you at least line these two lines up with chunk? Same for the other instance of this below.

Trott · 2016-06-25T00:20:23Z

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

mscdex · 2016-06-25T00:51:40Z

Looks like the common.mustCall(..., 2) will have to be changed to ~~just common.mustCall()~~ an "at least twice" check or even better, ensure that each separate callback is called at least once, for the stdout-stderr-data test. Windows seems to be less reliable in its chunking?

Trott · 2016-06-25T03:52:15Z

@mscdex Whoops, yeah, fixed, I hope, let's try again...

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

Trott · 2016-06-25T05:22:39Z

CI looks good. /cc @bnoordhuis @cjihrig

bnoordhuis · 2016-06-26T10:16:35Z

lib/child_process.js

        kill();
      } else {
-        _stdout.push(chunk);
+        if (!encoding)


Can you drop the negation and flip the clauses?

This reverts commit c9a5990.

A previous fix for a `maxBuffer` bug resulted in a change to the argument type for the `data` event on `child.stdin` and `child.stdout` when using `child_process.exec()`. This fixes the `maxBuffer` bug in a way that does not have that side effect. Fixes: nodejs#7342 Refs: nodejs#1901

Trott · 2016-06-27T21:52:26Z

CI again, hoping my echo for unicode characters tests work across platforms...

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

Trott · 2016-06-27T22:58:17Z

Modified tests for Windows. Let's try again:

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

Trott · 2016-06-27T23:30:17Z

That CI is green. Woot.

@bnoordhuis I put quotation marks around process.execPath but I'm pretty sure other characters that could conceivably be in the path may adversely affect at least some platforms (probably at least some of $, backticks, different types of quotation marks and slashes, question marks, asterisk...). Maybe that's OK because those are sufficiently infrequent while spaces are pretty common?

bnoordhuis · 2016-06-28T07:45:01Z

LGTM

Maybe that's OK because those are sufficiently infrequent while spaces are pretty common?

I agree.

Trott · 2016-06-28T18:14:54Z

/cc @cjihrig @JacksonTian

Trott · 2016-06-29T00:19:10Z

I'll land this in 24 hours if no one objects. An additional LGTM or two would be nice, though. /cc @nodejs/collaborators

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

A previous fix for a `maxBuffer` bug resulted in a change to the argument type for the `data` event on `child.stdin` and `child.stdout` when using `child_process.exec()`. This fixes the `maxBuffer` bug in a way that does not have that side effect. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Fixes: #7342 Refs: #1901

MylesBorins · 2016-07-12T00:01:44Z

@Trott lts?

Trott · 2016-07-12T03:30:02Z

@thealphanerd Yes if #6764 lands on LTS, otherwise no.

MylesBorins · 2016-07-12T16:44:44Z

I've gone ahead and landed this as #6764 landed. Do you think we would be better without either of these changes? Is there any chance of a regression?

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

A previous fix for a `maxBuffer` bug resulted in a change to the argument type for the `data` event on `child.stdin` and `child.stdout` when using `child_process.exec()`. This fixes the `maxBuffer` bug in a way that does not have that side effect. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Fixes: #7342 Refs: #1901

Trott · 2016-07-12T18:12:44Z

@thealphanerd I think we're better off with these changes than without them.

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

A previous fix for a `maxBuffer` bug resulted in a change to the argument type for the `data` event on `child.stdin` and `child.stdout` when using `child_process.exec()`. This fixes the `maxBuffer` bug in a way that does not have that side effect. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Fixes: #7342 Refs: #1901

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

A previous fix for a `maxBuffer` bug resulted in a change to the argument type for the `data` event on `child.stdin` and `child.stdout` when using `child_process.exec()`. This fixes the `maxBuffer` bug in a way that does not have that side effect. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Fixes: #7342 Refs: #1901

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

A previous fix for a `maxBuffer` bug resulted in a change to the argument type for the `data` event on `child.stdin` and `child.stdout` when using `child_process.exec()`. This fixes the `maxBuffer` bug in a way that does not have that side effect. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Fixes: #7342 Refs: #1901

Trott added the child_process Issues and PRs related to the child_process subsystem. label Jun 23, 2016

mscdex reviewed Jun 23, 2016
View reviewed changes

Trott force-pushed the benchomatic branch 2 times, most recently from 35485cd to 4c7266b Compare June 25, 2016 00:19

Trott force-pushed the benchomatic branch from 8e14148 to af90b50 Compare June 25, 2016 03:50

bnoordhuis reviewed Jun 26, 2016
View reviewed changes

lib/child_process.js

kill();

} else {

_stdout.push(chunk);

if (!encoding)

Copy link

Member

bnoordhuis Jun 26, 2016

Choose a reason for hiding this comment

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

Can you drop the negation and flip the clauses?

Trott force-pushed the benchomatic branch from af90b50 to 7ea64ad Compare June 26, 2016 20:53

Trott added 3 commits June 27, 2016 14:15

Revert "child_process: measure buffer length in bytes"

174f7c5

This reverts commit c9a5990.

squash: flip !encoding -> encoding

c9ccd19

Trott force-pushed the benchomatic branch from 7ea64ad to c9ccd19 Compare June 27, 2016 21:15

Trott added 2 commits June 27, 2016 14:46

squash: revise tests

381b0e6

squash: test nits

c5a3294

Trott mentioned this pull request Jun 27, 2016

Revert "child_process: measure buffer length in bytes" #7377

Closed

2 tasks

squash: fix test for Windows

a3a9223

Trott closed this Jun 30, 2016

This was referenced Jun 30, 2016

Encoding of child_process.exec.stdout changed from String to Buffer in v6.2.1 #7342

Closed

child_process: measure buffer length in bytes #7381

Closed

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016

Revert "child_process: measure buffer length in bytes"

4a0fb6f

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the lts-watch-v4.x label Jul 12, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jul 12, 2016

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016

Revert "child_process: measure buffer length in bytes"

8029ddd

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016

Revert "child_process: measure buffer length in bytes"

6b37cd5

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

MylesBorins pushed a commit that referenced this pull request Jul 14, 2016

Revert "child_process: measure buffer length in bytes"

1622bac

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jul 14, 2016

Revert "child_process: measure buffer length in bytes"

fd05b0b

This reverts commit c9a5990. PR-URL: #7391 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jackson Tian <[email protected]>

Trott deleted the benchomatic branch January 13, 2022 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: preserve argument type #7391

child_process: preserve argument type #7391

Trott commented Jun 23, 2016

mscdex Jun 23, 2016

Trott Jun 23, 2016

Trott commented Jun 25, 2016

mscdex commented Jun 25, 2016 •

edited

Loading

Trott commented Jun 25, 2016

Trott commented Jun 25, 2016

bnoordhuis Jun 26, 2016

Trott commented Jun 27, 2016

Trott commented Jun 27, 2016

Trott commented Jun 27, 2016

bnoordhuis commented Jun 28, 2016

Trott commented Jun 28, 2016

Trott commented Jun 29, 2016

MylesBorins commented Jul 12, 2016

Trott commented Jul 12, 2016

MylesBorins commented Jul 12, 2016

Trott commented Jul 12, 2016

child_process: preserve argument type #7391

child_process: preserve argument type #7391

Conversation

Trott commented Jun 23, 2016

Checklist

Affected core subsystem(s)

Description of change

mscdex Jun 23, 2016

Choose a reason for hiding this comment

Trott Jun 23, 2016

Choose a reason for hiding this comment

Trott commented Jun 25, 2016

mscdex commented Jun 25, 2016 • edited Loading

Trott commented Jun 25, 2016

Trott commented Jun 25, 2016

bnoordhuis Jun 26, 2016

Choose a reason for hiding this comment

Trott commented Jun 27, 2016

Trott commented Jun 27, 2016

Trott commented Jun 27, 2016

bnoordhuis commented Jun 28, 2016

Trott commented Jun 28, 2016

Trott commented Jun 29, 2016

MylesBorins commented Jul 12, 2016

Trott commented Jul 12, 2016

MylesBorins commented Jul 12, 2016

Trott commented Jul 12, 2016

mscdex commented Jun 25, 2016 •

edited

Loading