Child process options should not override env #1789

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@polotek
polotek commented Sep 28, 2011

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
Member

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 Sep 28, 2011
@bnoordhuis
Member

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
Collaborator
isaacs commented Sep 28, 2011

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
polotek commented Sep 29, 2011

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
polotek commented Sep 29, 2011

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
Member

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