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

[v22.2.0 regression] Some Worker use cases are broken #53011

Closed
liuxingbaoyu opened this issue May 15, 2024 · 5 comments · Fixed by #53029
Closed

[v22.2.0 regression] Some Worker use cases are broken #53011

liuxingbaoyu opened this issue May 15, 2024 · 5 comments · Fixed by #53029
Labels
v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. worker Issues and PRs related to Worker support.

Comments

@liuxingbaoyu
Copy link
Contributor

liuxingbaoyu commented May 15, 2024

Version

v22.2.0

Platform

all

Subsystem

worker_threads

What steps will reproduce the bug?

import { Worker, isMainThread } from "worker_threads";

if (isMainThread) {
  new Worker(import.meta.filename, { env: process.env });
} else {
  console.log("Hello, world!");
}

node --expose_gc .\test.mjs

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

Hello, world!

What do you see instead?

node:internal/worker:221
      throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv);
            ^

Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --expose_gc
    at new Worker (node:internal/worker:221:13)
    at file:///F:/babel/test.mjs:4:3
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5) {
  code: 'ERR_WORKER_INVALID_EXEC_ARGV'
}

Additional information

Ref: #52827
It seems that passing execArgv will cause some problems until #41103 is resolved.

I'm not sure this is a regression, but it could be an unintentional breakage and would be a bit difficult to resolve on the user side since there are no known exec argvs supported by Worker.

cc @theanarkh You may be interested!

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented May 15, 2024

Would it be possible to revert #52827 until a better fix is found? (i.e. the worker should just ignore unsupported argv that are passed implicitly rather than explicitly, or argv should be filtered before being passed to the worker)

@VoltrexKeyva VoltrexKeyva added worker Issues and PRs related to Worker support. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels May 16, 2024
@bricss
Copy link

bricss commented May 16, 2024

It seems like this behavior is quite intentional, as per docs:

execArgv <string[]> List of node CLI options passed to the worker. V8 options (such as --max-old-space-size) and options that affect the process (such as --title) are not supported. If set, this is provided as process.execArgv inside the worker. By default, options are inherited from the parent thread.

And a simple workaround is to add execArgv: [] to the worker's options 🕵️ or filter forbidden cli options from parent thread and pass it into worker options 🤷

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented May 16, 2024

If "execArgv must always be passed otherwise your worker might break" is intentional, then:

  • it shouldn't be affected by whether the env: {} option is passed or not
  • Node.js should probably warn when not passing execArgv, as a library might pass tests but it cannot control what CLI options the app that it's used into uses

The docs say that "By default, options are inherited from the parent thread". The behavior before #52827 was (when defining env) that process-wide options are inherited and thread-local options are not. The new behavior is that thread-local options are inherited, and process-wide options just crash.

Neither of the two behaviors seem ideal and neither matches the docs (when env is defined), but broken for broken it's better to keep the old broken to avoid breaking existing code.

When not passing env, both thread-local and process-wide options are inherited.

or filter forbidden cli options from parent thread and pass it into worker options 🤷

Node.js doesn't expose a list of options that are forbidden, so it's impossible to do so (see #52827)

@bricss
Copy link

bricss commented May 16, 2024

How about adding some code to fix this by filtering out unwanted cli options by default? 🤔

@theanarkh
Copy link
Contributor

Maybe we can ignore the invalid args when the execArgv is inherited from parent ? Only throw an error when the user pass execArgv explicitly, like here.

nodejs-github-bot pushed a commit that referenced this issue May 23, 2024
PR-URL: #53029
Fixes: #53011
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Jun 1, 2024
PR-URL: #53029
Fixes: #53011
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this issue Jun 1, 2024
PR-URL: nodejs#53029
Fixes: nodejs#53011
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. worker Issues and PRs related to Worker support.
Projects
None yet
5 participants