node process: make process.config read only

process: make process.config read only

Open gribnoysup opened this issue 1 year ago • 6 comments

Small clean up for the process.config. This part of the code already went through one major release cycle so it should be okay to remove. There were two suggested approaches to this, I decided to go with making the whole config read only as this feel a bit more performant than parsing it every time on access.

/cc @jasnell

Fixes: https://github.com/nodejs/node/issues/43581

Jun 30 '22 10:06 gribnoysup

Review requested:

[ ] @nodejs/startup

Jun 30 '22 10:06 nodejs-github-bot

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

Jul 04 '22 13:07 nodejs-github-bot

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

Jul 07 '22 20:07 nodejs-github-bot

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

Jul 08 '22 10:07 nodejs-github-bot

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

Aug 09 '22 13:08 nodejs-github-bot

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

Aug 09 '22 20:08 nodejs-github-bot

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

Aug 10 '22 10:08 nodejs-github-bot

test-abort-fatal-error is failing on several platforms for this PR -- I haven't seen it fail elsewhere. e.g. https://ci.nodejs.org/job/node-test-commit-linux/47044/nodes=rhel8-x64/console

12:22:53 not ok 3737 abort/test-abort-fatal-error
12:22:54   ---
12:22:54   duration_ms: 0.341
12:22:54   severity: fail
12:22:54   exitcode: 1
12:22:54   stack: |-
12:22:54     
12:22:54     
12:22:54     <--- Last few GCs --->
12:22:54     
12:22:54     [2342926:0x6ffa580]       28 ms: Mark-sweep (reduce) 3.5 (6.0) -> 3.5 (5.0) MB, 3.0 / 0.0 ms  (average mu = 0.254, current mu = 0.157) last resort; GC in old space requested
12:22:54     [2342926:0x6ffa580]       32 ms: Mark-sweep (reduce) 3.5 (5.0) -> 3.5 (5.0) MB, 3.7 / 0.0 ms  (average mu = 0.132, current mu = 0.004) last resort; GC in old space requested
12:22:54     
12:22:54     
12:22:54     <--- JS stacktrace --->
12:22:54     
12:22:54     
12:22:54     #
12:22:54     # Fatal javascript OOM in CALL_AND_RETRY_LAST
12:22:54     #
12:22:54     
12:22:54     /bin/sh: line 1: 2342926 Trace/breakpoint trap   (core dumped) /home/iojs/build/workspace/node-test-commit-linux/out/Release/node --max-old-space-size=4 --max-semi-space-size=1 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"
12:22:54     
12:22:54     Error: Command failed: ulimit -c 0; /home/iojs/build/workspace/node-test-commit-linux/out/Release/node --max-old-space-size=4 --max-semi-space-size=1 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"
12:22:54     
12:22:54     <--- Last few GCs --->
12:22:54     
12:22:54     [2342926:0x6ffa580]       28 ms: Mark-sweep (reduce) 3.5 (6.0) -> 3.5 (5.0) MB, 3.0 / 0.0 ms  (average mu = 0.254, current mu = 0.157) last resort; GC in old space requested
12:22:54     [2342926:0x6ffa580]       32 ms: Mark-sweep (reduce) 3.5 (5.0) -> 3.5 (5.0) MB, 3.7 / 0.0 ms  (average mu = 0.132, current mu = 0.004) last resort; GC in old space requested
12:22:54     
12:22:54     
12:22:54     <--- JS stacktrace --->
12:22:54     
12:22:54     
12:22:54     #
12:22:54     # Fatal javascript OOM in CALL_AND_RETRY_LAST
12:22:54     #
12:22:54     
12:22:54     /bin/sh: line 1: 2342926 Trace/breakpoint trap   (core dumped) /home/iojs/build/workspace/node-test-commit-linux/out/Release/node --max-old-space-size=4 --max-semi-space-size=1 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"
12:22:54     
12:22:54         at ChildProcess.exithandler (node:child_process:412:12)
12:22:54         at ChildProcess.emit (node:events:513:28)
12:22:54         at maybeClose (node:internal/child_process:1091:16)
12:22:54         at Socket.<anonymous> (node:internal/child_process:449:11)
12:22:54         at Socket.emit (node:events:513:28)
12:22:54         at Pipe.<anonymous> (node:net:309:12) {
12:22:54       code: 133,
12:22:54       killed: false,
12:22:54       signal: null,
12:22:54       cmd: 'ulimit -c 0; /home/iojs/build/workspace/node-test-commit-linux/out/Release/node --max-old-space-size=4 --max-semi-space-size=1 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"'
12:22:54     }
12:22:54     node:assert:389
12:22:54           throw message;
12:22:54           ^
12:22:54     
12:22:54     Error: Command failed: ulimit -c 0; /home/iojs/build/workspace/node-test-commit-linux/out/Release/node --max-old-space-size=4 --max-semi-space-size=1 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"
12:22:54     
12:22:54     <--- Last few GCs --->
12:22:54     
12:22:54     [2342926:0x6ffa580]       28 ms: Mark-sweep (reduce) 3.5 (6.0) -> 3.5 (5.0) MB, 3.0 / 0.0 ms  (average mu = 0.254, current mu = 0.157) last resort; GC in old space requested
12:22:54     [2342926:0x6ffa580]       32 ms: Mark-sweep (reduce) 3.5 (5.0) -> 3.5 (5.0) MB, 3.7 / 0.0 ms  (average mu = 0.132, current mu = 0.004) last resort; GC in old space requested
12:22:54     
12:22:54     
12:22:54     <--- JS stacktrace --->
12:22:54     
12:22:54     
12:22:54     #
12:22:54     # Fatal javascript OOM in CALL_AND_RETRY_LAST
12:22:54     #
12:22:54     
12:22:54     /bin/sh: line 1: 2342926 Trace/breakpoint trap   (core dumped) /home/iojs/build/workspace/node-test-commit-linux/out/Release/node --max-old-space-size=4 --max-semi-space-size=1 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"
12:22:54     
12:22:54         at ChildProcess.exithandler (node:child_process:412:12)
12:22:54         at ChildProcess.emit (node:events:513:28)
12:22:54         at maybeClose (node:internal/child_process:1091:16)
12:22:54         at Socket.<anonymous> (node:internal/child_process:449:11)
12:22:54         at Socket.emit (node:events:513:28)
12:22:54         at Pipe.<anonymous> (node:net:309:12) {
12:22:54       code: 133,
12:22:54       killed: false,
12:22:54       signal: null,
12:22:54       cmd: 'ulimit -c 0; /home/iojs/build/workspace/node-test-commit-linux/out/Release/node --max-old-space-size=4 --max-semi-space-size=1 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"'
12:22:54     }
12:22:54     
12:22:54     Node.js v19.0.0-pre
12:22:54   ...

Aug 10 '22 14:08 richardlau

Hmm, this wasn't failing for me locally, but I'm on macos and it seems to be passing there in CI too. I'll spin up a vm and will try to reproduce, thanks for highlighting

Aug 10 '22 14:08 gribnoysup

My guess is that freezing results in a memory overhead so the heap is now slightly larger than what --max-old-space-size=4 --max-semi-space-size=1 allows on certain platforms. You could try to bump those size limits to see if the crash goes away

Aug 10 '22 14:08 joyeecheung

@gribnoysup Any chance you can address the test failures in time for Node.js v19.0.0? Semver cutoff is in one month from now.

Sep 08 '22 16:09 aduh95

@aduh95 very sorry for the delay on this, struggling to find free time to finish it up. I think I'll be able to make it in time, and thanks for the heads up

Sep 08 '22 19:09 gribnoysup

@richardlau @aduh95 @joyeecheung I couldn't reproduce even after setting up a similar rhel machine, so just followed @joyeecheung suggestion (ty!) and bumped the memory limits. Can you please trigger the CI to see if that helps resolve it?

Sep 09 '22 08:09 gribnoysup

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

Sep 09 '22 09:09 nodejs-github-bot

Commit Queue failed

- Loading data for nodejs/node/pull/43627
✔  Done loading data for nodejs/node/pull/43627
----------------------------------- PR info ------------------------------------
Title      process: make process.config read only (#43627)
Author     Sergey Petushkov  (@gribnoysup)
Branch     gribnoysup:make-process-config-read-only -> nodejs:main
Labels     semver-major, process, author ready, needs-ci
Commits    1
 - process: make process.config read only
Committers 1
 - Sergey Petushkov 
PR-URL: https://github.com/nodejs/node/pull/43627
Fixes: https://github.com/nodejs/node/issues/43581
Reviewed-By: Anna Henningsen 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Colin Ihrig 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Ben Noordhuis 
Reviewed-By: Tobias Nießen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43627
Fixes: https://github.com/nodejs/node/issues/43581
Reviewed-By: Anna Henningsen 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Colin Ihrig 
Reviewed-By: Chengzhong Wu 
Reviewed-By: Ben Noordhuis 
Reviewed-By: Tobias Nießen 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - process: make process.config read only
   ℹ  This PR was created on Thu, 30 Jun 2022 10:21:37 GMT
   ✔  Approvals: 6
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/43627#pullrequestreview-1024630346
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43627#pullrequestreview-1024838350
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/43627#pullrequestreview-1025015733
   ✔  - Chengzhong Wu (@legendecas): https://github.com/nodejs/node/pull/43627#pullrequestreview-1025302826
   ✔  - Ben Noordhuis (@bnoordhuis): https://github.com/nodejs/node/pull/43627#pullrequestreview-1025447201
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/43627#pullrequestreview-1026017684
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-09-09T09:14:33Z: https://ci.nodejs.org/job/node-test-pull-request/46499/
- Querying data for job/node-test-pull-request/46499/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3029554252

Sep 10 '22 21:09 nodejs-github-bot

Landed in e0ab8dd63731d8eb92053776c5160fb190d5aea5

Sep 10 '22 22:09 nodejs-github-bot

node node copied to clipboard

process: make process.config read only

node
node copied to clipboard