FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: handling fork( path, undefined / null, obj ) by shobhitchittora · Pull Request #22416 · nodejs/node · GitHub

/ node Public

child_process: handling fork( path, undefined / null, obj ) - #22416

Closed
shobhitchittora wants to merge 8 commits into
nodejs:masterfrom
shobhitchittora:fork-args-fix
Closed

child_process: handling fork( path, undefined / null, obj )#22416
shobhitchittora wants to merge 8 commits into
nodejs:masterfrom
shobhitchittora:fork-args-fix

Conversation

shobhitchittora commented Aug 20, 2018
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the child_process Issues and PRs related to the child_process subsystem. label Aug 20, 2018
Comment thread lib/child_process.js Outdated

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add a test for this.

shobhitchittora commented Aug 22, 2018
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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'}});

Copy link
Copy Markdown
Contributor Author

@jasnell A CITGM run is required.

Copy link
Copy Markdown
Contributor Author

Ping @jasnell.

lundibundi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Comment thread lib/child_process.js Outdated

Copy link
Copy Markdown
Member

lundibundi commented Aug 28, 2018
edited
Loading

Copy link
Copy Markdown
Member

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)

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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 commented Sep 5, 2018

Copy link
Copy Markdown
Member

Ping @shobhitchittora

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member

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

shobhitchittora commented Oct 3, 2018
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member

Copy link
Copy Markdown
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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>

Trott commented Nov 2, 2018

Copy link
Copy Markdown
Member

Landed in 0d9d32a

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 <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
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 <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #22416
Fixes: #20749
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
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 <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
BethGriggs mentioned this pull request Dec 4, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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 outstanding review comments, 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


Back | FazBrowse Home | New Git URL