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.

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

child_process.fork not passing along options.env #20749

Closed
borisovg opened this issue May 15, 2018 · 7 comments
Closed

child_process.fork not passing along options.env #20749

borisovg opened this issue May 15, 2018 · 7 comments
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@borisovg
Copy link

  • Version: v8.11.1
  • Platform: Linux
  • Subsystem: Child Process

parent.js:

require('child_process').fork('./child.js', undefined, { env: { FOO: 'bar' } });

child.js

console.log(process.env.FOO);

output:

$ FOO=foo node parent.js 
foo
@vsemozhetbyt
Copy link
Contributor

Try:

require('child_process').fork('./child.js', [], { env: { FOO: 'bar' } });

@nodejs/child_process Is this expected behavior that the second argument set in undefined or null prevents the third one from being transferred?

@vsemozhetbyt vsemozhetbyt added the child_process Issues and PRs related to the child_process subsystem. label May 15, 2018
@borisovg
Copy link
Author

@vsemozhetbyt thanks - this also works:

require('child_process').fork('./child.js', { env: { FOO: 'bar' } });

I can guess why this is happening, having seen enough dodgy JS code that does parameter overloading ;)

@shobhitchittora
Copy link
Contributor

@vsemozhetbyt I think the check for arguments can be improved.

if (pos < arguments.length && arguments[pos] != null) {

if (typeof arguments[pos] !== 'object') {

We can check for emptiness (something like lodash.isEmpty) for both of the arguments. Then we can conditionally check whether both are present or not.

Or we can just document the behavior altogether. 😛

@ryzokuken
Copy link
Contributor

This looks like an ambiguity problem, and improving docs seems to be the logical way forward to me as well.

@borisovg
Copy link
Author

borisovg commented May 21, 2018

As a developer, even if documented, this looks like broken behaviour. Throwing an error on an invalid argument type is perfectly acceptable, but ignoring the subsequent argument is just shoddy - to be tolerated in a third party lib but not a core Node module. Just my opinion.

@ryzokuken
Copy link
Contributor

@borisovg if you insist, I could look a tad deeper into this over the course of the next few days and hack up a small PR.

@ryzokuken
Copy link
Contributor

@shobhitchittora is this fixed yet?

shobhitchittora added a commit to shobhitchittora/node that referenced this issue Aug 20, 2018
shobhitchittora added a commit to shobhitchittora/node that referenced this issue Aug 20, 2018
shobhitchittora added a commit to shobhitchittora/node that referenced this issue Aug 20, 2018
shobhitchittora added a commit to shobhitchittora/node that referenced this issue Aug 20, 2018
shobhitchittora added a commit to shobhitchittora/node that referenced this issue Aug 20, 2018
@Trott Trott closed this as completed in 0d9d32a Nov 2, 2018
targos pushed a commit that referenced this issue Nov 2, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 27, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 27, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
rvagg pushed a commit that referenced this issue Nov 28, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
rvagg pushed a commit that referenced this issue Nov 28, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 3, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants