worker_threads: proper env and NODE_OPTIONS handling #31711

lundibundi · 2020-02-09T22:29:37Z

~~Draft for now, as I still want to review the documentation and take another look at the code.~~

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

Closes: #30627

/cc @nodejs/workers @addaleax

src/node_options.cc

src/node_worker.cc

nodejs-github-bot · 2020-02-09T22:36:34Z

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

addaleax · 2020-02-09T22:35:38Z

lib/internal/errors.js

@@ -1361,6 +1361,9 @@ E('ERR_WASI_ALREADY_STARTED', 'WASI instance has already started', Error);
 E('ERR_WORKER_INVALID_EXEC_ARGV', (errors) =>
  `Initiated Worker with invalid execArgv flags: ${errors.join(', ')}`,
  Error);
+E('ERR_WORKER_INVALID_NODE_OPTIONS', (errors) =>
+  `Initiated Worker with invalid NODE_OPTIONS: ${errors.join(', ')}`,
+  Error);


Tbh, I think this is a situation here we should just use ERR_WORKER_INVALID_EXEC_ARGV, as it is the same type of error that’s being reported

Gave it another thought, imo ERR_WORKER_INVALID_EXEC_ARGV is way too specific and may mislead people into thinking that the error is elsewhere. I think it would be better to add ERR_WORKER_INVALID_OPTIONS and group both ERR_WORKER_INVALID_EXEC_ARGV and ERR_WORKER_INVALID_NODE_OPTIONS under it, though that may be breaking. Or just add and use ERR_WORKER_INVALID_OPTIONS here and then open a semver-major to change that in for ERR_WORKER_INVALID_EXEC_ARGV. wdyt?

That’s a general problem with our error codes – they tend to be way too specific.

I’d be good with changing the code, but yes, that would be breaking and I’d like to avoid it in this PR.

I’d still be good with using ERR_WORKER_INVALID_EXEC_ARGV and then pointing out in the error message where the error came from (and, ideally, also in a property on the error object). We can always switch that to a different error code later.

(Fwiw, ERR_WORKER_INVALID_OPTIONS sounds like it refers to the options object passed to the Worker constructor…)

(Fwiw, ERR_WORKER_INVALID_OPTIONS sounds like it refers to the options object passed to the Worker constructor…)

That was the idea - part of the options passed is incorrect. tbh we can probably use ERR_INVALID_OPT_VALUE from #31251 😄.

Then I'll use the ERR_WORKER_INVALID_EXEC_ARGV in this PR and then open another for the error change.

src/node.cc

src/node_options.cc

src/node_worker.cc

nodejs-github-bot · 2020-02-10T17:21:28Z

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

lundibundi · 2020-02-10T18:18:50Z

Not sure if flaky or a regression:
node-test-binary-arm-12+/test.sequential/test-inspector-port-zero

Error Message
fail (6)
Stacktrace
internal/console/global.js:44
globalConsole[kBindStreamsLazy](process);
                               ^

TypeError: globalConsole[kBindStreamsLazy] is not a function
    at internal/console/global.js:44:32
    at NativeModule.compileForInternalLoader (internal/bootstrap/loaders.js:276:7)
    at nativeModuleRequire (internal/bootstrap/loaders.js:305:14)
    at createGlobalConsole (internal/bootstrap/node.js:317:5)
    at internal/bootstrap/node.js:120:38

Trying a resume for now.

nodejs-github-bot · 2020-02-10T18:19:36Z

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

addaleax · 2020-02-10T18:29:12Z

@lundibundi I think I’ve seen that before in another test, but really couldn’t figure out why on earth that would fail … maybe it’s worth opening an issue about that

addaleax · 2020-02-13T15:05:47Z

Landed in 3e9302b...d63bcdd

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 9, 2020

lundibundi commented Feb 9, 2020

View reviewed changes

src/node_options.cc Show resolved Hide resolved

lundibundi commented Feb 9, 2020

View reviewed changes

src/node_worker.cc Show resolved Hide resolved

addaleax approved these changes Feb 9, 2020

View reviewed changes

benjamingr approved these changes Feb 10, 2020

View reviewed changes

lundibundi added 2 commits February 10, 2020 19:10

src: allow to reuse env options handling

0c87f8c

worker_threads: properly handle env and NODE_OPTIONS in workers

503003e

lundibundi force-pushed the worker-proper-argv-handling branch from 8f49779 to 503003e Compare February 10, 2020 17:17

lundibundi marked this pull request as ready for review February 10, 2020 17:19

lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2020

addaleax added the worker Issues and PRs related to Worker support. label Feb 13, 2020

addaleax closed this Feb 13, 2020

addaleax pushed a commit that referenced this pull request Feb 13, 2020

src: allow to reuse env options handling

84b8857

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

addaleax pushed a commit that referenced this pull request Feb 13, 2020

worker: properly handle env and NODE_OPTIONS in workers

d63bcdd

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

lundibundi deleted the worker-proper-argv-handling branch February 13, 2020 18:31

lundibundi mentioned this pull request Feb 13, 2020

worker_threads and NODE_OPTIONS #29117

Closed

codebytere pushed a commit that referenced this pull request Feb 17, 2020

src: allow to reuse env options handling

54caf76

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere pushed a commit that referenced this pull request Feb 17, 2020

worker: properly handle env and NODE_OPTIONS in workers

cd99dc7

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere mentioned this pull request Feb 17, 2020

v13.9.0 proposal #31837

Merged

codebytere pushed a commit that referenced this pull request Mar 15, 2020

src: allow to reuse env options handling

c125d46

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere pushed a commit that referenced this pull request Mar 15, 2020

worker: properly handle env and NODE_OPTIONS in workers

ad4d60c

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere pushed a commit that referenced this pull request Mar 17, 2020

src: allow to reuse env options handling

f08e0dd

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere pushed a commit that referenced this pull request Mar 17, 2020

worker: properly handle env and NODE_OPTIONS in workers

d8373a1

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere mentioned this pull request Mar 17, 2020

v12.16.2 proposal #32313

Merged

codebytere pushed a commit that referenced this pull request Mar 30, 2020

src: allow to reuse env options handling

28289ea

PR-URL: #31711 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere pushed a commit that referenced this pull request Mar 30, 2020

worker: properly handle env and NODE_OPTIONS in workers

30dbc84

PR-URL: #31711 Fixes: #30627 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Uzlopak mentioned this pull request Nov 4, 2023

lib: set default ICU locale per cli option --icu-locale #50538

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker_threads: proper env and NODE_OPTIONS handling #31711

worker_threads: proper env and NODE_OPTIONS handling #31711

lundibundi commented Feb 9, 2020 •

edited

Loading

nodejs-github-bot commented Feb 9, 2020

addaleax Feb 9, 2020

lundibundi Feb 10, 2020

addaleax Feb 10, 2020

lundibundi Feb 10, 2020

nodejs-github-bot commented Feb 10, 2020

lundibundi commented Feb 10, 2020 •

edited

Loading

nodejs-github-bot commented Feb 10, 2020

addaleax commented Feb 10, 2020

addaleax commented Feb 13, 2020

worker_threads: proper env and NODE_OPTIONS handling #31711

worker_threads: proper env and NODE_OPTIONS handling #31711

Conversation

lundibundi commented Feb 9, 2020 • edited Loading

Checklist

nodejs-github-bot commented Feb 9, 2020

addaleax Feb 9, 2020

Choose a reason for hiding this comment

lundibundi Feb 10, 2020

Choose a reason for hiding this comment

addaleax Feb 10, 2020

Choose a reason for hiding this comment

lundibundi Feb 10, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 10, 2020

lundibundi commented Feb 10, 2020 • edited Loading

nodejs-github-bot commented Feb 10, 2020

addaleax commented Feb 10, 2020

addaleax commented Feb 13, 2020

lundibundi commented Feb 9, 2020 •

edited

Loading

lundibundi commented Feb 10, 2020 •

edited

Loading