Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Child process options should not override env #1789

Closed
wants to merge 1 commit into from

3 participants

@polotek

This is a simple patch for an issue I've run into. The short of it is, if you pass an empty object as the options to child_process#spawn, That object will become the env instead of defaulting to process.env. The result is running the command with an empty environment. The included test just spawns a child with an empty options object and confirms that some env was present by checking for 'PATH='. Feel free to add something more sophisticated.

The reason I ran into this is because I'm building a lib around child processes and I'm passing through an options object. Something like the following.

function myLib(cmd, args, options) {
  options = options || {}
  // do some other stuff
  spawn(cmd, args, options);
}

You can work around this problem entirely by ensuring that the env property is set.

function myLib(cmd, args, options) {
  options = options || {}
  // do some other stuff

  if(!options.env) { options.env = process.env }
  spawn(cmd, args, options);
}

Since child_process is being overhauled as we speak, if this didn't go into an 0.4.x release, it wouldn't bother me. Consider it a note to double check 0.6.x.

@bnoordhuis

I appreciate the effort but this is a 'works as expected' and there's probably somewhere someone relying on it.

Unless you can present a compelling argument as to why this is always the wrong behaviour I won't merge it.

@bnoordhuis bnoordhuis closed this
@bnoordhuis

I should add that we're probably going to remove that bit of functionality some time in the future anyway, it's already deprecated.

@isaacs
Owner

I'm very much in agreement with @bnoordhuis that this patch is crazy hazardous, since it removes the ability to set the environment at all using the deprecated API.

I'd rather just remove the deprecated API altogether, which I think would also solve your problem.

@polotek

I admit I'm not that familiar with the old api. I was looking here.

// Deprecated API: (path, args, options, env, customFds)
// https://github.com/joyent/node/blob/v0.4/lib/child_process.js#L220

Could it be that this is a typo and the legacy api actually looks like this?

// Deprecated API: (path, args, env, customFds)

If that's true, then I can see your point. If options is an empty object then you have to assume it's the legacy api with an empty environment. This makes sense on it's own.

But I would argue that this is favoring the old api over the new one. Because an empty options object is a perfectly valid input for the new api. And as such you should revert all options to the default, including env => process.env. That's what I was expecting and that's what anybody using the new api should expect. To achieve an empty env, you could simply do:

spawn(cmd, [], {env:{}});

That's more explicit anyway.

The problem is that for the case of an empty object and no customFds passed in for the 4th argument, it's impossible to distinguish whether the user intends the old api or the new. My argument is that in these situations, after making all attempts to preserve backwards compat, you should favor the new api.

With that being said. If the old api is going away in 0.6.x this is a moot point. Feel free to punt on this. But keep in mind that you're then going to go through a period of having different semantics here.

spawn(cmd, [], {});

This statement will behave completely differently depending on which version of node you're using.

@polotek

At the very least I would put a note about this in the 0.4.x docs. Do you anticipate more point releases for 0.4? If so, I'll send a new pull request.

@bnoordhuis

Could it be that this is a typo and the legacy api actually looks like this?

// Deprecated API: (path, args, env, customFds)

Yes, that's a typo.

The old API will go away in 0.6.x - which should be Real Soon Now - but if you want to submit a pull request to update the 0.4.x docs, by all means do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 28, 2011
  1. @polotek
This page is out of date. Refresh to see the latest.
Showing with 13 additions and 1 deletion.
  1. +1 −1  lib/child_process.js
  2. +12 −0 test/simple/test-child-process-env.js
View
2  lib/child_process.js
@@ -219,7 +219,7 @@ ChildProcess.prototype.spawn = function(path, args, options, customFds) {
options.uid === undefined) {
// Deprecated API: (path, args, options, env, customFds)
cwd = '';
- env = options || process.env;
+ env = process.env;
customFds = customFds || [-1, -1, -1];
setsid = false;
uid = -1;
View
12 test/simple/test-child-process-env.js
@@ -42,7 +42,19 @@ child.stdout.addListener('data', function(chunk) {
response += chunk;
});
+var child2 = spawn('/usr/bin/env', [], {});
+var response2 = '';
+
+child2.stdout.setEncoding('utf8');
+
+child2.stdout.addListener('data', function(chunk) {
+ console.log('stdout2: ' + chunk);
+ response2 += chunk;
+});
+
process.addListener('exit', function() {
assert.ok(response.indexOf('HELLO=WORLD') >= 0);
assert.ok(response.indexOf('FOO=BAR') >= 0);
+
+ assert.ok(response2.indexOf('PATH=') >= 0);
});
Something went wrong with that request. Please try again.