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 <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>