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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[jest-worker] Don't crash when running node with flags disallowed in workers #12128

Merged
merged 5 commits into from Dec 10, 2021

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Dec 6, 2021

Summary

I noticed that Babel's CI started failing with this error a few days ago:

Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid NODE_OPTIONS env variable: --openssl-legacy-provider is not allowed in NODE_OPTIONS
    at new NodeError (node:internal/errors:371:5)
    at new Worker (node:internal/worker:194:13)
    at ExperimentalWorker.initialize (/home/circleci/babel/scripts/integration-tests/tmp/create-react-app/node_modules/jest-worker/build/workers/NodeThreadsWorker.js:149:20)

Then, I noticed #12103 which fixes the problem for a specific option, and I found the root cause in #12069. I don't understand why #12069 specifies execArgv: process.execArgv, but removing it should fix the bug for all the different options that currently make jest-worker crash (I don't even know if this list is documented, but I can try finding it again in the Node.js source if needed).

<rant>
I am really annoyed by this Node.js behavior. Reading the docs it looks like the default value of execArgv is process.execArgv, but it's not. Yesterday I spent some time trying to fix this same identical bug in a non-jest-related PR I was working on for Babel (babel/babel#14025 (comment)) and just after reading the Node.js source code I realized that it's an important difference: if you pass execArgv: process.execArgv Node.js validates it; if you don't pass anything it uses process.execArgv without performing any validation.
If you read the docs knowing this difference you can see that it never actually says that process.execArgv, but it's really annoying that this difference is not explicitly stated and they make you think that it's the default value.
</rant>

Test plan

CI 馃
I did this change from the GH UI; lets see if it works.

@nicolo-ribaudo
Copy link
Contributor Author

@nicolo-ribaudo nicolo-ribaudo commented Dec 6, 2021

Ok I need to clone the repo locally to understand what's going on; I thought execArgv in the now failing test was coming from ...this._options.forkOptions.

@nicolo-ribaudo
Copy link
Contributor Author

@nicolo-ribaudo nicolo-ribaudo commented Dec 6, 2021

Passing execArgv: process.execArgv in #12069 was a breaking change, for the reason highlighted above. However, after this PR:

  • Users can still pass custom execArgv in forkOptions.
  • The default value of execArgv in a worker are the execArgv of the parent thread (not the value of process.execArgv, but the original arguments used to start the thread).

If someone needs the behavior introduced in #12069 (i.e. enforce validation of the exec argv of the parent thread), they can explicitly pass forkOptions: { execArgv: process.execArgv }.

@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #12128 (37db5f0) into main (3093c18) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12128      +/-   ##
==========================================
- Coverage   67.71%   67.71%   -0.01%     
==========================================
  Files         328      328              
  Lines       16990    16989       -1     
  Branches     4817     4817              
==========================================
- Hits        11505    11504       -1     
  Misses       5452     5452              
  Partials       33       33              
Impacted Files Coverage 螖
...kages/jest-worker/src/workers/NodeThreadsWorker.ts 93.54% <酶> (-0.07%) 猬囷笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3093c18...37db5f0. Read the comment docs.

@nicolo-ribaudo
Copy link
Contributor Author

@nicolo-ribaudo nicolo-ribaudo commented Dec 7, 2021

I also opened nodejs/node#41103 to lift this restriction from the Node.js side.

@DzmVasileusky
Copy link

@DzmVasileusky DzmVasileusky commented Dec 9, 2021

+1 for fixing it
in my case it is falling on max_http_header_size

filtering v => !/^--(max_old_space_size|max-old-space-size)/.test(v), is a very selective way to fix much bigger issue

[13:32:37][Step 1/7] Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --max_http_header_size=20000
[13:32:37][Step 1/7]     at new Worker (internal/worker.js:153:13)
[13:32:37][Step 1/7]     at ExperimentalWorker.initialize (/opt/tc-agent/work/e98f5429040b0b37/node_modules/jest-worker/build/workers/NodeThreadsWorker.js:149:20)
[13:32:37][Step 1/7]     at new ExperimentalWorker (/opt/tc-agent/work/e98f5429040b0b37/node_modules/jest-worker/build/workers/NodeThreadsWorker.js:145:10)
[13:32:37][Step 1/7]     at WorkerPool.createWorker (/opt/tc-agent/work/e98f5429040b0b37/node_modules/jest-worker/build/WorkerPool.js:44:12)
[13:32:37][Step 1/7]     at new BaseWorkerPool (/opt/tc-agent/work/e98f5429040b0b37/node_modules/jest-worker/build/base/BaseWorkerPool.js:127:27)
[13:32:37][Step 1/7]     at new WorkerPool (/opt/tc-agent/work/e98f5429040b0b37/node_modules/jest-worker/build/WorkerPool.js:30:1)
[13:32:37][Step 1/7]     at new Worker (/opt/tc-agent/work/e98f5429040b0b37/node_modules/jest-worker/build/index.js:167:26)
[13:32:37][Step 1/7]     at getWorker (/opt/tc-agent/work/e98f5429040b0b37/node_modules/@angular-devkit/build-angular/node_modules/terser-webpack-plugin/dist/index.js:288:9)
[13:32:37][Step 1/7]     at /opt/tc-agent/work/e98f5429040b0b37/node_modules/@angular-devkit/build-angular/node_modules/terser-webpack-plugin/dist/index.js:389:41
[13:32:37][Step 1/7]     at /opt/tc-agent/work/e98f5429040b0b37/node_modules/p-limit/index.js:23:31
[13:32:37][Step 1/7]     at run (/opt/tc-agent/work/e98f5429040b0b37/node_modules/p-limit/index.js:23:43)
[13:32:37][Step 1/7]     at /opt/tc-agent/work/e98f5429040b0b37/node_modules/p-limit/index.js:45:20

SimenB
SimenB approved these changes Dec 10, 2021
Copy link
Collaborator

@SimenB SimenB left a comment

thanks for digging into this! I was similarly surprised by the lacking docs in this area, thanks for opening an issue there!

@SimenB SimenB merged commit 5ccb0a0 into facebook:main Dec 10, 2021
2 of 10 checks passed
@SimenB
Copy link
Collaborator

@SimenB SimenB commented Dec 10, 2021

/cc @kherock FYI

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Dec 10, 2021

https://github.com/facebook/jest/releases/tag/v27.4.4

@github-actions
Copy link

@github-actions github-actions bot commented Jan 10, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants