child_process: exec's promisify impl now includes stdout/err in error #13388

giltayar · 2017-06-02T09:26:12Z

This converts the initial implementation of a promised exec that used
the customPromisifyArgs support in util.promisify with a custom
implementation. This is because exec and execFile, when there is an
error, still supply the stdout and stderr of the process, and yet
the promisified version with customPromisifyArgs does
not supply this ability.

I created a custom implementation and attached it to exec and execFile
using the util.promisify.custom key.

Note that I added stdout and stderr as regular enumerable
properties. I'm not sure that's the right decision.

Fixes: #13364

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)

child_process

addaleax · 2017-06-02T10:24:41Z

lib/child_process.js

-                      { value: ['stdout', 'stderr'], enumerable: false });
-
+Object.defineProperty(exports.exec, util.promisify.custom, {
+  enumerable: false, value: function(...args) {


I think we prefer breaking after the ,, and if you want, you can use the value(...args) { shorthand notation

addaleax · 2017-06-02T10:25:44Z

lib/child_process.js

+    });
+    return promise;
+  }
+});


These two functions are nearly identical. It should be okay, but it shouldn’t be too hard to merge these, right?

addaleax · 2017-06-02T10:26:14Z

lib/child_process.js

+Object.defineProperty(exports.exec, util.promisify.custom, {
+  enumerable: false, value: function(...args) {
+    const promise = createPromise();
+    exports.exec.call(this, ...args, (err, stdout, stderr) => {


I don’t think you need to use .call; exec should work no matter what this refers to

Definitely, that was my first all. And to use an arrow function instead of a regular function (as the property value). But then I looked at util.promisify and that's what it does, so I got all afraid that in some cases the this in exec may have some meaning.

OK - I'll refactor the value of the property into an arrow function + call exec regulary without .call.

addaleax · 2017-06-02T10:26:59Z

test/parallel/test-child-process-promisified.js

@@ -32,3 +32,22 @@ const execFile = promisify(child_process.execFile);
    assert(err.message.includes('doesntexist'));
  }));
 }
+const failingCodeWithStdoutErr =
+  'console.log("out");console.error("err");process.exit(1)';


Some people have figured out that you can use template literals for getting good multiline JS code. :)

Very much not a fan of multiline termplate literals :-(

I prefer not to - this string goes into the command line, and I have no idea what this will do there, especially not in windows.

addaleax · 2017-06-02T10:28:03Z

test/parallel/test-child-process-promisified.js

+const failingCodeWithStdoutErr =
+  'console.log("out");console.error("err");process.exit(1)';
+{
+  exec(`${process.execPath} -e '${failingCodeWithStdoutErr}'`)


Does this work on Windows? I think there was some weirdness with single/double quotes. (If you don’t know, that’s okay; CI will tell us.)

"A wise man does not fall into a hole that a smart man can get out of" :-). I'll change it to output numbers and then use double quotes like the other tests.

addaleax · 2017-06-02T10:29:51Z

Note that I added stdout and stderr as regular enumerable properties. I'm not sure that's the right decision.

It sounds like a good call to me; I think console.log() should print this kind of thing, for example.

jasnell

LGTM with the reported nits fixed

giltayar · 2017-06-02T18:23:48Z

Refactored according to all review comments, except for the multiline string comment, due to concern about Windows compatibility.

BTW, note that the kCustomPromisifyArgsSymbol introduced in util.promisify was used only for child_process.exec[File] and is now not used anywhere in the code.

addaleax · 2017-06-02T18:29:36Z

CI: https://ci.nodejs.org/job/node-test-commit/10320/

This converts the initial implementation of a promised exec that used the customPromisifyArgs support in util.promisify with a custom implementation. This is because exec and execFile, when there is an error, still supply the stdout and stderr of the process, and yet the promisified version with customPromisifyArgs does not supply this ability. I created a custom implementation and attached it to exec and execFile using the util.promisify.custom key. Fixes: #13364

giltayar · 2017-06-02T19:07:40Z

Sorry about that. Fixing - I had some weird git problems, that should now be resolved. Double-checking and then I'll push.

giltayar · 2017-06-02T19:16:40Z

Hmm... nope, no weird git stuff. I did break the build (sorry...), but fixed it immediately after with a commit that fixed the bug. I double-checked and it's OK.

For some reason, the CI is marking that commit as failing, but when I click on the job, then it's building the previous commit, which should fail.

Maybe you should just run the CI again on this pull request?

addaleax · 2017-06-02T19:21:11Z

For some reason, the CI is marking that commit as failing, but when I click on the job, then it's building the previous commit, which should fail.

Yes, the bot reports the results for the last commit in a PR, not the one which the build was actually started for. It’s a bit unfortunate.

Maybe you should just run the CI again on this pull request?

Yup. :)
CI: https://ci.nodejs.org/job/node-test-commit/10322/

giltayar · 2017-06-02T20:15:08Z

It seems there's a problem in Windows. I'll dig up a Windows VM tomorrow and try and figure it out.

addaleax · 2017-06-03T09:20:31Z

New CI: https://ci.nodejs.org/job/node-test-commit/10330/

giltayar · 2017-06-08T19:16:28Z

Anything additional I need to do here?

addaleax · 2017-06-09T19:48:55Z

@giltayar Yes, ping us and ask whether there’s anything left to do. 😄

Landed in 8d7f07f, thanks for the PR! I tweaked the subject line of the commit a bit to get it under 50 characters.

This converts the initial implementation of a promised exec that used the customPromisifyArgs support in util.promisify with a custom implementation. This is because exec and execFile, when there is an error, still supply the stdout and stderr of the process, and yet the promisified version with customPromisifyArgs does not supply this ability. I created a custom implementation and attached it to exec and execFile using the util.promisify.custom key. Fixes: #13364 PR-URL: #13388 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

* **Child processes** * `stdout` and `stderr` are now available on the error output of a failed call to the `util.promisify()`ed version of `child_process.exec`. [[`d66d4fc94c`](d66d4fc94c)] [#13388](#13388) * **HTTPS** * The `rejectUnauthorized` option now works properly for unix sockets. [[`c4cbd99d37`](c4cbd99d37)] [#13505](#13505) * **Readline** * A change that broke `npm init` and other code which uses `readline` multiple times on the same input stream is reverted. [[`0df6c0b5f0`](0df6c0b5f0)] [#13560](#13560)

* **Child processes** * `stdout` and `stderr` are now available on the error output of a failed call to the `util.promisify()`ed version of `child_process.exec`. [[`d66d4fc94c`](d66d4fc94c)] [#13388](#13388) * **HTTP** * A regression that broke certain scenarios in which HTTP is used together with the `cluster` module has been fixed. [[`fff8a56d6f`](fff8a56d6f)] [#13578](#13578) * **HTTPS** * The `rejectUnauthorized` option now works properly for unix sockets. [[`c4cbd99d37`](c4cbd99d37)] [#13505](#13505) * **Readline** * A change that broke `npm init` and other code which uses `readline` multiple times on the same input stream is reverted. [[`0df6c0b5f0`](0df6c0b5f0)] [#13560](#13560) PR-URL: #13598

MylesBorins · 2017-07-17T18:18:36Z

Is this something we should backport to v6.x?

giltayar · 2017-07-17T18:27:44Z

Given that util.promisify doesn't exist in v6.x, there's no need for this to be backported.

…

On Mon, Jul 17, 2017 at 9:19 PM Myles Borins ***@***.***> wrote: Is this something we should backport to v6.x? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13388 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYnRAGRrV3-ORrdzKa_QuUDolfcOuSgks5sO6W8gaJpZM4NuCjK> .

nicojs · 2018-10-09T11:05:22Z

Given that util.promisify doesn't exist in v6.x, there's no need for this to be backported.

I'm trying to create a node6 compatible shim for util.promisify, so a backport would have been very useful to me.

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jun 2, 2017

addaleax approved these changes Jun 2, 2017

View reviewed changes

mscdex added the promises Issues and PRs related to ECMAScript promises. label Jun 2, 2017

jasnell approved these changes Jun 2, 2017

View reviewed changes

addaleax approved these changes Jun 2, 2017

View reviewed changes

giltayar added 3 commits June 2, 2017 21:59

child_process: refactorings after review comments

e2182e5

child_process: fixed bug in promisified exec test

6adbd98

child_process: fixed win compat bug in test

e37cb7e

addaleax closed this Jun 9, 2017

addaleax mentioned this pull request Jun 10, 2017

v8.1.1 proposal #13598

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: exec's promisify impl now includes stdout/err in error #13388

child_process: exec's promisify impl now includes stdout/err in error #13388

giltayar commented Jun 2, 2017

addaleax Jun 2, 2017

addaleax Jun 2, 2017

giltayar Jun 2, 2017

addaleax Jun 2, 2017

giltayar Jun 2, 2017

addaleax Jun 2, 2017

jasnell Jun 2, 2017

giltayar Jun 2, 2017

addaleax Jun 2, 2017

giltayar Jun 2, 2017

addaleax commented Jun 2, 2017

jasnell left a comment

giltayar commented Jun 2, 2017

addaleax commented Jun 2, 2017

giltayar commented Jun 2, 2017

giltayar commented Jun 2, 2017

addaleax commented Jun 2, 2017

giltayar commented Jun 2, 2017

addaleax commented Jun 3, 2017

giltayar commented Jun 8, 2017

addaleax commented Jun 9, 2017

MylesBorins commented Jul 17, 2017

giltayar commented Jul 17, 2017 via email

nicojs commented Oct 9, 2018

child_process: exec's promisify impl now includes stdout/err in error #13388

child_process: exec's promisify impl now includes stdout/err in error #13388

Conversation

giltayar commented Jun 2, 2017

Checklist

Affected core subsystem(s)

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 2, 2017

jasnell left a comment

Choose a reason for hiding this comment

giltayar commented Jun 2, 2017

addaleax commented Jun 2, 2017

giltayar commented Jun 2, 2017

giltayar commented Jun 2, 2017

addaleax commented Jun 2, 2017

giltayar commented Jun 2, 2017

addaleax commented Jun 3, 2017

giltayar commented Jun 8, 2017

addaleax commented Jun 9, 2017

MylesBorins commented Jul 17, 2017

giltayar commented Jul 17, 2017 via email

nicojs commented Oct 9, 2018