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.

Sign up for GitHub

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

Jump to bottom

Add NODE_DEBUG info to child_process #721

Closed
wants to merge 1 commit into from
Closed

Add NODE_DEBUG info to child_process #721

wants to merge 1 commit into from

Conversation

remixz
Copy link
Contributor

@remixz remixz commented Feb 5, 2015

Fixes GH-720. Also adds the arguments passed to the Error object.

Example:

var spawn = require('child_process').spawn;
var path = require('path');

var child = spawn('fdjslkdf', ['--foo', 'bar'], {
  cwd: path.join(process.cwd(), './bat')
});

child.on('error', function (err) {
  throw err;
});
$ NODE_DEBUG=child_process ./iojs tmp.js
CHILD_PROCESS 72525: command: fdjslkdf
CHILD_PROCESS 72525: arguments: --foo,bar
CHILD_PROCESS 72525: options: {"cwd":"/Users/zach/Documents/github/remixz/io.js/bat"}
/Users/zach/Documents/github/remixz/io.js/tmp.js:9
  throw err;
        ^
Error: spawn fdjslkdf ENOENT
    at exports._errnoException (util.js:738:11)
    at Process.ChildProcess._handle.onexit (child_process.js:1029:32)
    at child_process.js:1120:20
    at process._tickCallback (node.js:337:11)
    at Function.Module.runMain (module.js:489:11)
    at startup (node.js:111:16)
    at node.js:799:3

cjihrig
cjihrig reviewed Feb 5, 2015
View reviewed changes
lib/child_process.js
@@ -6,6 +6,7 @@ const net = require('net');
const dgram = require('dgram');
const assert = require('assert');
const util = require('util');
const debug = util.debuglog('child_process')
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a semicolon

@remixz remixz force-pushed the gh-720-node-debug-child-process branch from dfe56bd to a967948 Compare February 5, 2015 03:38
@remixz
Copy link
Contributor Author

remixz commented Feb 5, 2015

@cjihrig All done! Rebased into a967948.

cjihrig
cjihrig reviewed Feb 5, 2015
View reviewed changes
lib/child_process.js
@@ -1035,6 +1038,7 @@ function ChildProcess() {
if (self.spawnfile)
err.path = self.spawnfile;

err.spawnargs = self.spawnargs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the slice(1) from below to this line so that it's only called when an error occurs.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 5, 2015

@trevnorris what do you think?

@remixz
Add NODE_DEBUG info to child_process
f7711fe 
Fixes nodejsGH-720. Also adds the arguments passed to the Error object.
@remixz remixz force-pushed the gh-720-node-debug-child-process branch from a967948 to f7711fe Compare February 5, 2015 03:54
@trevnorris
Copy link
Contributor

I'm cool with this. Except for the debug() call, it only affects error cases. And this isn't a hot code path anyways. LGTM.

@cjihrig want to clean up the commit and land it, or shall I?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 5, 2015

I'll land it.

cjihrig pushed a commit that referenced this pull request Feb 5, 2015
@remixz @cjihrig
child_process: add debug and error details
9a8f186 
This commit adds debug() calls to spawn() and spawnSync(), and
attaches additional information to Error objects.

Fixes: #720
PR-URL: #721
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Feb 5, 2015

Thanks! Landed in 9a8f186. I tweaked the commit message and moved the debug('spawnSync' up a few lines to avoid printing the environment data too.

@cjihrig cjihrig closed this Feb 5, 2015
@rvagg
Copy link
Member

rvagg commented Feb 9, 2015

@cjihrig @trevnorris should this have landed with a test too? It doesn't look trivial enough to me to not include a new test to help prevent regressions.

Reopening but please feel free to close if you have another perspective.

@rvagg rvagg reopened this Feb 9, 2015
@trevnorris
Copy link
Contributor

@rvagg The change isn't going to break existing functionality, but we could add a test to make sure the correct values are being attached to the error object.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2015

I'll add the test for the error objects.

@cjihrig cjihrig mentioned this pull request Feb 13, 2015
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Feb 14, 2015

Test landed in d53b636

@cjihrig cjihrig closed this Feb 14, 2015
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@remixz @cjihrig @trevnorris @rvagg