worker: reset Isolate stack limit after entering Locker

[addaleax]

It turns out that using v8::Locker undoes the effects of
passing an explicit stack limit as part of the Isolate’s
resource constraints.

Therefore, reset the stack limit manually after entering a Locker.

Refs: #26049 (comment)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

The test added here failed on Win2012 R2 + VS2019 in CI. Not sure if it's fail-every-time or flaky, but either way, probably warrants investigation before landing.....

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/1509/RUN_SUBSET=0,nodes=win2012r2-COMPILED_BY-vs2019-x86/console

02:46:31 not ok 618 parallel/test-worker-stack-overflow-stack-size
02:46:31 ---
02:46:31 duration_ms: 0.734
02:46:31 severity: fail
02:46:31 exitcode: 1
02:46:31 stack: |-
02:46:31 C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:616
02:46:31 const crashOnUnhandledRejection = (err) => { throw err; };
02:46:31 ^
02:46:31 02:46:31 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
02:46:31 02:46:31 110925 !== 110924
02:46:31 02:46:31 at C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-worker-stack-overflow-stack-size.js:39:10 {
02:46:31 generatedMessage: true,
02:46:31 code: 'ERR_ASSERTION',
02:46:31 actual: 110925,
02:46:31 expected: 110924,
02:46:31 operator: 'strictEqual'
02:46:31 }
02:46:31 ...

[addaleax]

@Trott thanks for checking – I’ve updated the PR, leaving some wiggle room for inaccuracies.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[addaleax]

Landed in 9111e02

[ulrichb]

@addaleax Could you explain the effect of this change?

Dos this mean --stack-size doesn't get applied for worker threads anymore? If yes, how can I change the limit for a WT?

[addaleax]

@ulrichb Yes, that is correct. The stack size in Workers that V8 knows about is now the actual stack size that was used when creating the Workers. There is currently no way so set --stack-size manually for Workers (but --stack-size usually doesn’t do what people think it does, and this PR is definitely a bugfix).

We’ve considered making the stack size configurable through the resourceLimits option of the Worker constructor, however, we are not currently aware of an use case for that, and it seems hard for users to figure out what a “good” stack size might be. However, that does not mean that that isn’t still possible.

[ulrichb]

Thanks for your reply!

Okay, I understand. Atm. this works for me because the hard-coded WT limit (4 MB?) is way larger than the default --stack-size => So my use case works with Node 14 (just tested it).

But to be future-proof, I'd really hope for a resourceLimits option. Should I create an issue?

My use case: I'm hosting the TypeScript compiler in a WT (which is necessary because it is heavily CPU bound + non-async) and the TS compiler does a lot of recursion, which can possibly hit the stack limit dependent on user input :(

Here is a synthetic example , which hits the recursion limit with N > ~2500 on my machine (TS v3.8.3):

importfsfrom"fs";importworkerThreadsfrom"worker_threads";importtsfrom"typescript";if(workerThreads.isMainThread){constworker=newworkerThreads.Worker(__filename);worker.on("exit",code=>{console.log(`Worker stopped with exit code ${code}`);});}else{console.log("Hello from worker, TS version: "+ts.version);fs.writeFileSync("TempLargeTSFile.ts",`class MyBuilder { method(): this { return this; } }export default new MyBuilder()${[...Array(4000)].map(()=>` .method()`).join("\n")};`);constprogram=ts.createProgram(["TempLargeTSFile.ts"],{types: []});constemitResult=program.emit();constdiagnostics=[...ts.getPreEmitDiagnostics(program), ...emitResult.diagnostics];console.log('diagnostics',diagnostics);}

[addaleax]

@ulrichb So to be clear, you are using --stack-size to increase the default recursion limit? Just keep in mind that it does not actually set the stack size, and only tells V8 how much of the stack it can assume is usable. (i.e. setting it to a value higher than the actual stack size is possible but will result in a hard crash in case of stack overflow.)

But to be future-proof, I'd really hope for a resourceLimits option. Should I create an issue?

I can just open a PR and we can see how that goes.

[addaleax]

@ulrichb That would be #33085 :)

[ulrichb]

@ulrichb So to be clear, you are using --stack-size to increase the default recursion limit?

Yes.

Just keep in mind that it does not actually set the stack size, and only tells V8 how much of the stack it can assume is usable. (i.e. setting it to a value higher than the actual stack size is possible but will result in a hard crash in case of stack overflow.)

Okay, so the actual stack limit is dependent on the OS process, and --stack-size (and the new WT option) are only there for a) fail-fast, and b) better "Maximum call stack size exceeded" errors, just like Python's sys.setrecursionlimit(). Right?

I can just open a PR and we can see how that goes.

Nice. Thank you!

[addaleax]

Just keep in mind that it does not actually set the stack size, and only tells V8 how much of the stack it can assume is usable. (i.e. setting it to a value higher than the actual stack size is possible but will result in a hard crash in case of stack overflow.)

Okay, so the actual stack limit is dependent on the OS process, and --stack-size (and the new WT option) are only there for a) fail-fast, and b) better "Maximum call stack size exceeded" errors, just like Python's sys.setrecursionlimit(). Right?

Yes, but only if you set it to a lower value than the actual stack size. 👍