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: handling fork( path, undefined / null, obj ) #22416

Closed
wants to merge 8 commits into from

Conversation

shobhitchittora
Copy link
Contributor

@shobhitchittora shobhitchittora commented Aug 20, 2018

Closes: #20749

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

NOTE: Run test using - python ./tools/test.py parallel/test-child-process-fork-options.js

Affected subsystem(s)

child_process / fork

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Aug 20, 2018
lib/child_process.js Outdated Show resolved Hide resolved
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Please add a test for this.

@shobhitchittora
Copy link
Contributor Author

shobhitchittora commented Aug 22, 2018

@jasnell @BridgeAR with this fix we can make the options work when args is undefined or null, but it still fails when args is an empty object {}. Should we also incorporate this by throwing if an object is passed as the second argument? -

fork('test.js', {} , {env: {foo: 'bar'}});

@shobhitchittora
Copy link
Contributor Author

@jasnell A CITGM run is required.

@shobhitchittora
Copy link
Contributor Author

Ping @jasnell.

Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM

lib/child_process.js Outdated Show resolved Hide resolved
test/parallel/test-child-process-fork-options.js Outdated Show resolved Hide resolved
test/parallel/test-child-process-fork-options.js Outdated Show resolved Hide resolved
@lundibundi
Copy link
Member

@lundibundi
Copy link
Member

lundibundi commented Aug 28, 2018

Related failure:
https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel72-s390x/4549/testReport/junit/(root)/test/parallel_test_child_process_fork_options/
(I suspect that the process exited faster than you've got the message)

@shobhitchittora
Copy link
Contributor Author

@lundibundi Need some help fixing the failure that you've mentioned. Cannot reproduce in my local.

@BridgeAR
Copy link
Member

@shobhitchittora fork is an async operation that does not keep the event loop alive. Therefore the test exits before the spawned file is done and in that case the test will not receive the message and the test fails.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

Ping @shobhitchittora

@shobhitchittora
Copy link
Contributor Author

@BridgeAR I've removed exit(0) from the forked process fixture. I haven't found any existing fixture to use here.

Also I've no clue about how to make the test wait for the fork's response.

@lundibundi
Copy link
Member

@shobhitchittora I think using .on('exit', common.mustCall()) might help.

@shobhitchittora
Copy link
Contributor Author

shobhitchittora commented Oct 3, 2018

@lundibundi Are you sure? I don't see a point of using onexit. Can you please explain? Also any other ideas? This seems weird to me.

@lundibundi
Copy link
Member

Theoretically, it should make current node process wait until fork finishes as we are listening on its close event. Also, this seems to be the way it's done with other fork tests. Anyway I think it's worth a try, this is a simple change anyway.

@lundibundi
Copy link
Member

@shobhitchittora
Copy link
Contributor Author

Any idea why the status code returned by child is 127 or 1 in the CI? This seems that there was some error while executing the forked process.

Also I tried running the child js file in local and got the below error -

TypeError: process.send is not a function
    at Object.<anonymous> (/Users/schittora/Desktop/node/test/fixtures/child-process-spawn-node.js:10:9)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)
    at Function.Module.runMain (module.js:676:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3

@lundibundi
Copy link
Member

Sorry, forgot about this one.
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18238/.

@shobhitchittora
Copy link
Contributor Author

@lundibundi Everything looks green 💚 . Thanks!! 👍

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 2, 2018
PR-URL: nodejs#22416
Fixes: nodejs#20749
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 2, 2018

Landed in 0d9d32a

@Trott Trott closed this Nov 2, 2018
targos pushed a commit that referenced this pull request 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 pull request 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 pull request 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]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request 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 pull request 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 pull request 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]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
MylesBorins pushed a commit that referenced this pull request 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]>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

child_process.fork not passing along options.env
10 participants