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.

Sign up for GitHub

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

Jump to bottom

worker: add args constructor option #30559

Closed
wants to merge 1 commit into from
Closed

worker: add args constructor option #30559

wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

A convenience option to populate process.argv in worker threads.

Fixes: #30531

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

@nodejs-github-bot nodejs-github-bot added the worker Issues and PRs related to Worker support. label Nov 20, 2019
addaleax
addaleax approved these changes Nov 20, 2019
View reviewed changes
doc/api/worker_threads.md Outdated Show resolved Hide resolved
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 20, 2019
lundibundi
lundibundi reviewed Nov 20, 2019
View reviewed changes
lib/internal/worker.js Outdated Show resolved Hide resolved
gireeshpunathil
gireeshpunathil approved these changes Nov 21, 2019
View reviewed changes
doc/api/worker_threads.md Outdated Show resolved Hide resolved
lundibundi
lundibundi reviewed Nov 24, 2019
View reviewed changes
doc/api/worker_threads.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26926/

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2019
@legendecas
Copy link
Member Author

@addaleax @gireeshpunathil @lundibundi just found a new issue on eval mode of workers. PTAL again...😅

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26930/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26936/

addaleax
addaleax reviewed Nov 25, 2019
View reviewed changes
test/parallel/test-worker-process-argv.js Outdated
const assert = require('assert');
assert.deepStrictEqual(
process.argv,
[process.execPath, 'null', 'foo', '123', String(Symbol('bar'))]
Copy link
Member

Choose a reason for hiding this comment

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

I know this matches node --eval, but … this feels a bit wrong to me.

Shouldn’t the second entry always be the filename for the current script, even if it’s a fake one because of eval: true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it make sense that the filename is [worker eval] for node -p '__filename' printing [eval]?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I’m good with that personally 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more common to get filename from the __filename right? Since __filename in eval worker already presents and has a value of [worker eval], is it required to add the filename to argv while we don't generally have it in our node -e 'process.argv'?

Copy link
Member

Choose a reason for hiding this comment

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

It's more common to get filename from the __filename right?

__filename and process.argv[1] are different things; the former is the path of the current CJS module, the latter is the path of the main module.

Since __filename in eval worker already presents and has a value of [worker eval], is it required to add the filename to argv while we don't generally have it in our node -e 'process.argv'?

You can feel free to omit this, but I’d consider it inconsistent that process.argv doesn’t include a fake filename for node -e. It seems to break expectations for me, because in node -e the structure of process.argv should be the same as elsewhere.

Changing that for node -e is a breaking change – and I’d be willing to open a PR for that – but here we have the chance to be consistent from the beginning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested against bash, in which argv never includes filename, in both eval mode or script mode. So it might make sense to add a fake filename in eval mode since we have it in script mode.

I'll update the PR to include the filename in eval worker's argv to keep consistency with script mode.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2019
@legendecas legendecas force-pushed the issue-30531 branch 3 times, most recently from eae77c1 to d2ce74f Compare November 30, 2019 16:04
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27216/

@legendecas
worker: add args constructor option
9dde270 
A convenience option to populate `process.argv` in worker threads.
@legendecas
Copy link
Member Author

Tests failed for the case test/parallel/test-worker-process-argv.js was initially ran in workers. Trying to fix it by adding an additional check on if the worker was started by the expected options.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27245/

jasnell
jasnell reviewed Dec 1, 2019
View reviewed changes
lib/internal/main/worker_thread.js
const name = '[worker eval]';
// This is necessary for CJS module compilation.
// TODO: pass this with something really internal.
ObjectDefineProperty(process, '_eval', {
Copy link
Member

Choose a reason for hiding this comment

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

Would a symbol be better/possible here?

Copy link
Member

Choose a reason for hiding this comment

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

This is to match with what the CJS loader expects for the --eval CLI option, unfortunately

@addaleax
Copy link
Member

addaleax commented Dec 3, 2019

Landed in 3ebae6c 🎉

@addaleax addaleax closed this Dec 3, 2019
addaleax pushed a commit that referenced this pull request Dec 3, 2019
@legendecas @addaleax
worker: add argv constructor option
3ebae6c 
A convenience option to populate `process.argv` in worker threads.

PR-URL: #30559
Fixes: #30531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@legendecas legendecas deleted the issue-30531 branch December 4, 2019 03:39
targos pushed a commit that referenced this pull request Dec 9, 2019
@legendecas @targos
worker: add argv constructor option
71d43a5 
A convenience option to populate `process.argv` in worker threads.

PR-URL: #30559
Fixes: #30531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
Merged
MylesBorins added a commit that referenced this pull request Dec 16, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
57bf5b7 
This is a security release.

This release includes a single commit, an update to npm to 6.13.4.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  - update npm to 6.13.4
    #30904
  - update uvwasi (Anna Henningsen)
    #30745
  - upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  - docs deprecate http finished (Robert Nagy)
    #28679
* events:
  - add captureRejection option (Matteo Collina)
    #27867
* http:
  - add captureRejection support (Matteo Collina)
    #27867
  - llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  - implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  - implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  - support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  - add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  - implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  - expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  - add argv constructor option (legendecas)
    #30559

PR-URL: #30937
MylesBorins added a commit that referenced this pull request Dec 16, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
906a1fd 
This is a security release.

This release includes a single commit, an update to npm to 6.13.4.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  * update npm to 6.13.4
    #30904
  * update uvwasi (Anna Henningsen)
    #30745
  * upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  * docs deprecate http finished (Robert Nagy)
    #28679
* events:
  * add captureRejection option (Matteo Collina)
    #27867
* http:
  * add captureRejection support (Matteo Collina)
    #27867
  * llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  * implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  * implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  * support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  * add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  * implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  * expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  * add argv constructor option (legendecas)
    #30559

PR-URL: #30937
MylesBorins added a commit that referenced this pull request Dec 16, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
f15fe94 
This is a security release.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  * update npm to 6.13.4
    #30904
  * update uvwasi (Anna Henningsen)
    #30745
  * upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  * docs deprecate http finished (Robert Nagy)
    #28679
* events:
  * add captureRejection option (Matteo Collina)
    #27867
* http:
  * add captureRejection support (Matteo Collina)
    #27867
  * llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  * implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  * implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  * support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  * add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  * implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  * expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  * add argv constructor option (legendecas)
    #30559

PR-URL: #30937
MylesBorins added a commit that referenced this pull request Dec 17, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
bef1e87 
This is a security release.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  * update npm to 6.13.4
    #30904
  * update uvwasi (Anna Henningsen)
    #30745
  * upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  * docs deprecate http finished (Robert Nagy)
    #28679
* events:
  * add captureRejection option (Matteo Collina)
    #27867
* http:
  * add captureRejection support (Matteo Collina)
    #27867
  * llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  * implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  * implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  * support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  * add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  * implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  * expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  * add argv constructor option (legendecas)
    #30559

PR-URL: #30937
targos pushed a commit that referenced this pull request Jan 14, 2020
@legendecas @targos
worker: add argv constructor option
81e363a 
A convenience option to populate `process.argv` in worker threads.

PR-URL: #30559
Fixes: #30531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@legendecas @BethGriggs
worker: add argv constructor option
4ea1b81 
A convenience option to populate `process.argv` in worker threads.

PR-URL: #30559
Fixes: #30531
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@addaleax addaleax addaleax approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@lundibundi lundibundi lundibundi approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

worker: process.argv for workers
6 participants
@legendecas @nodejs-github-bot @addaleax @jasnell @gireeshpunathil @lundibundi