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

Add option to filter invalid arguments from execArgv when launching worker threads #3207

Closed
shellscape opened this issue Jun 5, 2023 · 9 comments · Fixed by #3336
Closed

Comments

@shellscape
Copy link

Please provide details about:

  • What you're trying to do

I'm using a task runner that's constructing Node commands as such:

node --title svc-test:test ../../node_modules/ava/entrypoints/cli.mjs
  • What happened

--title is not valid for creating a worker.

✘ Internal error
  Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --title
  Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --title
      at new NodeError (node:internal/errors:399:5)
      at new Worker (node:internal/worker:192:13)
      at createWorker (file:///Users/user/code/test/node_modules/.pnpm/ava@5.3.0/node_modules/ava/lib/fork.js:24:12)
      at loadFork (file:///Users/user/code/test/node_modules/.pnpm/ava@5.3.0/node_modules/ava/lib/fork.js:84:39)
      at pMap.concurrency.concurrency (file:///Users/user/code/test/node_modules/.pnpm/ava@5.3.0/node_modules/ava/lib/api.js:289:20)
      at file:///Users/user/code/test/node_modules/.pnpm/p-map@5.5.0/node_modules/p-map/index.js:141:26
  • What you expected to happen

I'd expect that Ava would filter out options that would throw. Probably related nodejs/node#41103 (comment)

@ghost
Copy link

ghost commented Jun 12, 2023

I also want to do this! Would like to access GC control in my tests, with one of these options:

  • --expose-gc
  • --allow-natives-syntax

Temporarily I disabled workers to get it running.

export default {
  verbose: true,
  workerThreads: false
}

@novemberborn
Copy link
Member

@shellscape I'm not sure what the best way out of this is.

We do explicitly forward the execArgv, maybe that is not necessary? You can use the nodeArguments option to specify additional arguments.

I think not forwarding execArgv would be a breaking change so now's the time.

@novemberborn
Copy link
Member

Looks like the explicit process.execArgv reference was added for a debugging feature back in 2016: #874

The problem though is we need to pass a flag to enable source maps, which means we can never inherit the default execArgv:

const additionalExecArgv = ['--enable-source-maps'];

Filtering the list seems problematic since it may change between Node.js releases, unless there's a third party dependency we can use for that.

@shellscape
Copy link
Author

Yeah it's a tricky one. This is a documented issue across the Node ecosystem as well - it's a gap between process and workers in Node and so far the Node core team won't address it. The tool I was using has since removed use of --title from their code, so my immediate cause is fixed. And that's probably the process for most users - asking their tooling authors (or their own team) to remove that parameter until Node itself acts on some compatibility. Filtering will probably work in the near term, so probably not so problematic until it breaks (if it ever does) again.

If that doesn't feel right, adding to the docs about this edge case would probably suffice.

@novemberborn
Copy link
Member

Having thought about this some more, we could add a filterExecArgv() through the ava.config.* files. It could receive the individual values allowing end-users to discard incompatible ones.

As for documentation, how about this? #3215

@novemberborn
Copy link
Member

See also #3025.

@shellscape
Copy link
Author

Those are both great. I think we're in good shape to close this.

@novemberborn novemberborn changed the title execArgv should not be passed invalid arguments Add option to filter invalid arguments from execArgv when launching worker threads Jul 30, 2023
@novemberborn
Copy link
Member

I think a filter function could be a useful addition, so I'll leave this open for now.

@kirklimbu

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants