FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: preserve argument type by Trott · Pull Request #7391 · nodejs/node · GitHub

/ node Public

child_process: preserve argument type - #7391

Closed
Trott wants to merge 6 commits into
nodejs:masterfrom
Trott:benchomatic
ConversationCommits (6)ChecksFiles changed
Closed

child_process: preserve argument type#7391
Trott wants to merge 6 commits into
nodejs:masterfrom
Trott:benchomatic

Conversation

Trott commented Jun 23, 2016

Copy link
Copy Markdown
Member
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).

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
Comment thread lib/child_process.js Outdated

mscdex Jun 23, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott Jun 23, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Aligned.

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

Trott commented Jun 25, 2016

Copy link
Copy Markdown
Member Author

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

mscdex commented Jun 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Jun 25, 2016

Copy link
Copy Markdown
Member Author

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

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

Trott commented Jun 25, 2016

Copy link
Copy Markdown
Member Author

CI looks good. /cc @bnoordhuis @cjihrig

bnoordhuis reviewed Jun 26, 2016
View reviewed changes
Comment thread lib/child_process.js Outdated

bnoordhuis Jun 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you drop the negation and flip the clauses?

Trott added 3 commits June 27, 2016 14:15
Revert "child_process: measure buffer length in bytes"
174f7c5 
This reverts commit c9a5990.
child_process: preserve argument type
7059a85 
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
squash: flip !encoding -> encoding
c9ccd19

Trott commented Jun 27, 2016

Copy link
Copy Markdown
Member Author

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

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

Trott mentioned this pull request Jun 27, 2016
Closed
2 tasks

Trott commented Jun 27, 2016

Copy link
Copy Markdown
Member Author

Modified tests for Windows. Let's try again:

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

Trott commented Jun 27, 2016

Copy link
Copy Markdown
Member Author

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 commented Jun 28, 2016

Copy link
Copy Markdown
Member

LGTM

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

I agree.

Trott commented Jun 28, 2016

Copy link
Copy Markdown
Member Author

/cc @cjihrig @JacksonTian

Trott commented Jun 29, 2016

Copy link
Copy Markdown
Member Author

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

Trott closed this Jun 30, 2016
This was referenced Jun 30, 2016
Closed
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
child_process: preserve argument type
2aa06b9 
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Fixes: #7342
Refs: #1901
Fishrock123 mentioned this pull request Jul 5, 2016
Merged
MylesBorins mentioned this pull request Jul 11, 2016
Closed

MylesBorins commented Jul 12, 2016

Copy link
Copy Markdown
Contributor

@Trott lts?

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

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

MylesBorins commented Jul 12, 2016

Copy link
Copy Markdown
Contributor

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?

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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
child_process: preserve argument type
4e6cbde 
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Fixes: #7342
Refs: #1901

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

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

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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
child_process: preserve argument type
97b50d3 
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Fixes: #7342
Refs: #1901
MylesBorins mentioned this pull request Jul 12, 2016
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
child_process: preserve argument type
0923c00 
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Fixes: #7342
Refs: #1901
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
child_process: preserve argument type
6a08535 
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 <info@bnoordhuis.nl>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Fixes: #7342
Refs: #1901
Trott deleted the benchomatic branch January 13, 2022 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

Back | FazBrowse Home | New Git URL