child_process.fork: add string-shortcut for stdio #10866

trendsetter37 · 2017-01-18T07:37:25Z

Add string-shortcut option for stdio parameter.

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

Affected core subsystem(s)

doc, child_process

mscdex · 2017-01-18T07:39:37Z

The subsystem name should just be child_process. This should fit the first line length limit: child_process: add string shortcut for fork stdio.

mscdex · 2017-01-18T07:41:16Z

lib/child_process.js

@@ -50,7 +50,24 @@ exports.fork = function(modulePath /*, args, options*/) {

  args = execArgv.concat([modulePath], args);

-  if (!Array.isArray(options.stdio)) {
+  if (typeof options.stdio == 'string') {
+    const stdioStringToArray = (option) => {


I think this should be moved outside of fork() since it does not rely on any variables in the current scope, where it should use the function stdioStringToArray(option) { ... } format.

While it doesn't depend on any variables in the fork() scope I did think it logically belonged inside of fork because it wasn't generic enough to be used by any of the other exported functions needing the stdioStringToArray operation.

Case in point, fork is the only function that requires 'ipc' be append to the returned array. This is just my opinion though so comments/feedback are welcomed.

Well, the main idea is to avoid the possibility of V8 having to recompile/reoptimize the function. By pulling it out of the function, that can be achieved for sure.

mscdex · 2017-01-18T07:41:27Z

lib/child_process.js

@@ -50,7 +50,24 @@ exports.fork = function(modulePath /*, args, options*/) {

  args = execArgv.concat([modulePath], args);

-  if (!Array.isArray(options.stdio)) {
+  if (typeof options.stdio == 'string') {


Triple equals needed here.

addaleax · 2017-01-18T07:43:45Z

lib/child_process.js

+        break;
+      case 'inherit':
+        options.stdio = stdioStringToArray(options.stdio);
+        break;


fwiw it looks like you can shorten these to

case 'ignore': case 'pipe': case 'inherit': options.stdio = stdioStringToArray(options.stdio); break;

Wow, yep. Definitely missed that; thanks

trendsetter37 · 2017-01-18T14:17:52Z

I guess I shouldn't squash until the entire review process is done right?

Trott · 2017-01-18T14:57:58Z

lib/child_process.js

+        options.stdio = stdioStringToArray(options.stdio);
+        break;
+      default:
+        throw new TypeError(`Incorrect type of stdio option: ${options.stdio}`);


Nit: It would probably be more consistent with our other TypeError messages if this were something like Unknown stdio option: ${options.stdio}.

Should the unknown option be embedded in the message? What happens if a complex JSON object is in there? One that is circular? Won't the attempt to stringify it fail? I don't think we generaly print the bad input in the message, we just say what is bad about the input.

@sam-github grep -R 'new [A-Za-z]*Error' lib|grep '`' does give about 10 positive results or so.

Fwiw I find it really useful when I can just take part of an error message and grep my sources for that bit with a reasonable expectation of instantly finding the offending lines. If you’re worried about failed stringifying, we can always wrap the input in util.inspect; does that sound better?

The fixed string part should be greppable and unique, as you say. Just tested, the stringification seems to just do [object Object] for recursive and complex objects. Not useful, but not damaging.

git grep TypeError lib is pretty clear: https://gist.github.com/f81b6d3e19ab6595244905575e55e082

There are many, many lines, and all the errors say WHAT is wrong with the input, but do not re-include the input itself in the message.

git grep TypeError.*\` lib lib/dns.js: throw new TypeError(`"port" should be >= 0 and < 65536, got "${port}"`); lib/internal/url.js: throw new TypeError('Value of `this` is not a URLSearchParams'); ... (all examples of `this`, not of format strings)

So at this point, we have one example of a format string inputting an invalid argument into the type error message, and this would be the second, so now I'm convinced. The error should not echo the input back, it should include information that is not known to the caller - what is wrong with the input, not the input itself.

git grep TypeError lib

Funny, why does git (re-?)implement grep?

So at this point, we have one example of a format string inputting an invalid argument into the type error message, and this would be the second, so now I'm convinced.

These are the positive results when not restricting to TypeErrors (or is there any particular reason you only looked for those?): https://gist.github.com/addaleax/cd3d3e5a6c7d752eaa1d15396e80608e (“only” 12 but still.)

But why does it really matter whether there are precedents for this? I can’t really see any situation in which this is harmful, and I can see cases in which echoing the input back might be helpful (very simple example: typos).

I find randomness in APIs not helpful. When one API does it one way, and another does it another way, its confusing. If we believe that errors on invalid arguments to API calls should include the input, we should then start work on adding such information to every error, would make good code-and-learn contributions. If we don't believe that, and individual errors that say "X is wrong with argument Y" (maybe followed with) ": got/was/saw/whatever ${y}" are randomly formatted according to what looked good in a single PR or appeals to the taste of whoever happend to write and review that PR, we get a chaotic API. Btw, in your gist, lines 3 and 4 should be type errors to be consistent with line 2... the type of error chosen for those is another example of the kind of random API choice I would like us to make less of.

@sam-github I opted to go with the TypeError: Unknown stdio option although I do find it useful to have the corrupted input logged back to the developer. That being said, I don't think it's useful enough to go against consistency unless we do decide that it is beneficial to change all errors accordingly.

@addaleax I did have to use util.inspect to prevent printing [Object Object]. Not sure why I forgot to use that in the original PR

Trott · 2017-01-18T14:59:30Z

test/parallel/test-child-process-fork-stdio-string-variant.js

+const malFormedOpts = {stdio: '33'};
+const payload = {hello: 'world'};
+
+assert.throws(() => fork(childScript, malFormedOpts), /Incorrect type/);


If possible, let's match the entire error message in the regular expression, so something like ^TypeError: Unknown stdio option: 33$/ or whatever the message ends up being.

Trott · 2017-01-18T15:01:11Z

I guess I shouldn't squash until the entire review process is done right?

Up to you, I think. In most cases, I prefer that we don't squash until the review process is done but others feel differently.

sam-github · 2017-01-18T15:03:23Z

lib/child_process.js

+        options.stdio = stdioStringToArray(options.stdio);
+        break;
+      default:
+        throw new TypeError(`Incorrect type of stdio option: ${options.stdio}`);


Should the unknown option be embedded in the message? What happens if a complex JSON object is in there? One that is circular? Won't the attempt to stringify it fail? I don't think we generaly print the bad input in the message, we just say what is bad about the input.

cjihrig · 2017-01-18T15:22:22Z

lib/child_process.js

@@ -50,7 +50,20 @@ exports.fork = function(modulePath /*, args, options*/) {

  args = execArgv.concat([modulePath], args);

-  if (!Array.isArray(options.stdio)) {
+  if (typeof options.stdio === 'string') {
+    const stdioStringToArray = (option) => {


You can make this a normal function, and pull it out of fork() to the top level.

cjihrig · 2017-01-18T15:22:47Z

lib/child_process.js

+      default:
+        throw new TypeError(`Incorrect type of stdio option: ${options.stdio}`);
+    }
+  } else if (!Array.isArray(options.stdio)) {
    // Use a separate fd=3 for the IPC channel. Inherit stdin, stdout,
    // and stderr from the parent if silent isn't set.
    options.stdio = options.silent ? ['pipe', 'pipe', 'pipe', 'ipc'] :


You can use stdioStringToArray() here too.

trendsetter37 · 2017-01-20T17:22:44Z

@cjihrig @sam-github changes commited.

sam-github

Re-add the link to https://nodejs.org/api/child_process.html#child_process_options_stdio, which describes the meaning of this option. We need that info.

Otherwise, LGTM

trendsetter37 · 2017-01-22T06:32:29Z

@sam-github Done.

trendsetter37 · 2017-01-23T19:27:48Z

ping @cjihrig @sam-github

cjihrig · 2017-01-23T19:34:42Z

doc/api/child_process.md

-    [`stdio`][] option. When this option is provided, it overrides `silent`.
-    The array must contain exactly one item with value `'ipc'` or an error will
-    be thrown. For instance `[0, 1, 2, 'ipc']`.
+  * `stdio` {Array|String} Supports the Array and String versions of


I don't think we need to explicitly say "Supports the Array and String versions of..." since they are now the same. Maybe just a note to "see child_process.spawn()'s stdio"

cjihrig · 2017-01-23T19:35:44Z

lib/child_process.js

@@ -455,6 +469,7 @@ function lookupSignal(signal) {
  if (typeof signal === 'number')
    return signal;

+


Can you remove this blank line.

cjihrig · 2017-01-23T19:37:00Z

test/parallel/test-child-process-fork-stdio-string-variant.js

+const errorRegexp = /^TypeError: Unknown stdio option$/;
+const malFormedOpts = {stdio: '33'};
+const payload = {hello: 'world'};
+const StringOpts = {stdio: 'pipe'};


StringOpts -> stringOpts please

cjihrig · 2017-01-23T19:38:15Z

test/parallel/test-child-process-fork-stdio-string-variant.js

+const child = fork(childScript, StringOpts);
+
+child.on('message', (message) => {
+  assert.ok(message.foo);


Is there a strictEqual() comparison that you can make here?

cjihrig

Left a few comments. Looking good though.

Add string shortcut option for stdio parameter. PR-URL: nodejs#10866 Fixes: nodejs#10793

mscdex · 2017-01-26T04:14:02Z

@trendsetter37 Other than the error message text, it LGTM.

sam-github · 2017-01-27T20:07:30Z

Landed in 3268863, thanks.

Add string shortcut option for stdio parameter. Fixes: #10793 PR-URL: #10866 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Brian White <mscdex@mscdex.net>

mscdex · 2017-01-27T22:21:53Z

@sam-github My LGTM was conditional, I was wanting the error text to be changed first ...

jasnell · 2017-01-27T22:27:25Z

I am also wondering if this shouldn't be semver-major due to the change in throw conditions it introduces.

sam-github · 2017-01-27T22:32:42Z

Sorry, Brian, I misunderstood. And I can't figure out exactly what you want changed, I thought it was the fixes line. @mscdex Can you point me to your comment? I can fix and PR a follow up.

@jasnell probably. That the conditions under which errors are thrown and the error message strings are part of the API should be mentioned in #7964

sam-github · 2017-01-27T22:37:23Z

Its probably possible to create a backportable version of this that does not throw. Or even a version that doesn't throw, and then a tiny update PR following on, that makes it throw and will only go in 8.x?

Should this be reverted and given more time?

mscdex · 2017-01-27T22:50:50Z

@sam-github #10866 (comment) and #10866 (comment)

sam-github · 2017-01-27T22:51:38Z

What about the error message text would you like changed?

mscdex · 2017-01-27T22:52:14Z

@sam-github In my first linked comment I suggested the same error message used for spawn() for the same situation.

sam-github · 2017-01-27T22:54:04Z

@jasnell @mscdex #11044

trendsetter37 · 2017-01-28T20:04:14Z

@sam-github saw this just now. Should I change this here or just let the change happen on #11044?

sam-github · 2017-01-30T21:56:21Z

@trendsetter37 I am fixing in #11044

sam-github · 2017-01-30T22:10:22Z

@nodejs/lts This is a marginal case of semver-majorness. Strings were previously undefined as options.stdio values for fork(), and were ignored. Now, they are processed.

If someone is passing a string to .stdio, hoping it does something like spawn, it now will, of if they are pasing invalid values to .stdio, it will now throw.

I guess this could be major... but its also a useful feature that would be nice to have on LTS.

Is LTS just out of luck?

nodejs-github-bot added the lts-watch-v6.x label Jan 18, 2017

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Jan 18, 2017

mscdex reviewed Jan 18, 2017

View reviewed changes

addaleax reviewed Jan 18, 2017

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 18, 2017

trendsetter37 force-pushed the feature/10793-add-string-variant-stdio-option branch from 90b64ff to aed4586 Compare January 18, 2017 14:13

nodejs-github-bot added the lts-watch-v6.x label Jan 18, 2017

Trott reviewed Jan 18, 2017

View reviewed changes

sam-github suggested changes Jan 18, 2017

View reviewed changes

cjihrig requested changes Jan 18, 2017

View reviewed changes

trendsetter37 force-pushed the feature/10793-add-string-variant-stdio-option branch from aed4586 to 79562f6 Compare January 20, 2017 17:18

nodejs-github-bot added the lts-watch-v6.x label Jan 20, 2017

sam-github suggested changes Jan 20, 2017

View reviewed changes

trendsetter37 force-pushed the feature/10793-add-string-variant-stdio-option branch from 79562f6 to e00a6c6 Compare January 22, 2017 06:29

nodejs-github-bot added the lts-watch-v6.x label Jan 22, 2017

sam-github approved these changes Jan 23, 2017

View reviewed changes

cjihrig reviewed Jan 23, 2017

View reviewed changes

cjihrig requested changes Jan 23, 2017

View reviewed changes

trendsetter37 force-pushed the feature/10793-add-string-variant-stdio-option branch from e00a6c6 to 06bf0dd Compare January 24, 2017 15:56

child_process: add string shortcut for fork stdio

710b468

Add string shortcut option for stdio parameter. PR-URL: nodejs#10866 Fixes: nodejs#10793

trendsetter37 force-pushed the feature/10793-add-string-variant-stdio-option branch from 2513b29 to 710b468 Compare January 26, 2017 02:03

nodejs-github-bot added the lts-watch-v6.x label Jan 26, 2017

sam-github closed this Jan 27, 2017

sam-github mentioned this pull request Jan 27, 2017

meta: deprecation policy #7964

Closed

2 tasks

sam-github added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Jan 27, 2017

sam-github mentioned this pull request Jan 27, 2017

child_process: align fork/spawn stdio error msg #11044

Closed

sam-github reopened this Jan 27, 2017

sam-github closed this Jan 30, 2017

sam-github added dont-land-on-v7.x and removed lts-watch-v6.x labels Jan 30, 2017

trendsetter37 deleted the feature/10793-add-string-variant-stdio-option branch January 31, 2017 00:31

Trott mentioned this pull request Mar 14, 2017

test: include all stdio strings for fork() #11783

Closed

2 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

gireeshpunathil mentioned this pull request May 14, 2018

child_process: fork does not accept null or undefined args array #10819

Closed

		@@ -455,6 +469,7 @@ function lookupSignal(signal) {
		if (typeof signal === 'number')
		return signal;

child_process.fork: add string-shortcut for stdio #10866

child_process.fork: add string-shortcut for stdio #10866

Conversation

trendsetter37 commented Jan 18, 2017

Checklist

Affected core subsystem(s)

mscdex commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trendsetter37 commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trendsetter37 commented Jan 20, 2017

sam-github left a comment

Choose a reason for hiding this comment

trendsetter37 commented Jan 22, 2017

trendsetter37 commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

mscdex commented Jan 26, 2017

sam-github commented Jan 27, 2017

mscdex commented Jan 27, 2017

jasnell commented Jan 27, 2017

sam-github commented Jan 27, 2017

sam-github commented Jan 27, 2017

mscdex commented Jan 27, 2017 • edited Loading

sam-github commented Jan 27, 2017

mscdex commented Jan 27, 2017 • edited Loading

sam-github commented Jan 27, 2017

trendsetter37 commented Jan 28, 2017

sam-github commented Jan 30, 2017

sam-github commented Jan 30, 2017

mscdex Jan 18, 2017 •

edited

Loading

mscdex commented Jan 27, 2017 •

edited

Loading

mscdex commented Jan 27, 2017 •

edited

Loading