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

worker: reset Isolate stack limit after entering Locker #31593

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

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

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: nodejs#26049 (comment)
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels Jan 31, 2020
src/node_worker.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

src/node_worker.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Feb 2, 2020

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
Copy link
Member Author

addaleax commented Feb 4, 2020

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit that referenced this pull request Feb 8, 2020
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)

PR-URL: #31593
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Feb 8, 2020

Landed in 9111e02

@addaleax addaleax closed this Feb 8, 2020
@addaleax addaleax deleted the worker-stack-size-locker branch February 8, 2020 14:17
codebytere pushed a commit that referenced this pull request Feb 17, 2020
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)

PR-URL: #31593
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
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)

PR-URL: #31593
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
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)

PR-URL: #31593
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
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)

PR-URL: #31593
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ulrichb
Copy link

ulrichb commented Apr 26, 2020

@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
Copy link
Member Author

@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
Copy link

ulrichb commented Apr 26, 2020

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):

import fs from "fs";
import workerThreads from "worker_threads";
import ts from "typescript";

if (workerThreads.isMainThread) {

    const worker = new workerThreads.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")}
;
`);

    const program = ts.createProgram(["TempLargeTSFile.ts"], { types: [] });

    const emitResult = program.emit();

    const diagnostics = [...ts.getPreEmitDiagnostics(program), ...emitResult.diagnostics];
    console.log('diagnostics', diagnostics);
}

@addaleax
Copy link
Member Author

@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 added a commit to addaleax/node that referenced this pull request Apr 27, 2020
Add `stackSizeMb` to the `resourceLimit` option group.

Refs: nodejs#31593 (comment)
@addaleax
Copy link
Member Author

@ulrichb That would be #33085 :)

@ulrichb
Copy link

ulrichb commented Apr 27, 2020

@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
Copy link
Member Author

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. 👍

addaleax added a commit that referenced this pull request Apr 29, 2020
Add `stackSizeMb` to the `resourceLimit` option group.

Refs: #31593 (comment)

PR-URL: #33085
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request May 4, 2020
Add `stackSizeMb` to the `resourceLimit` option group.

Refs: #31593 (comment)

PR-URL: #33085
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Add `stackSizeMb` to the `resourceLimit` option group.

Refs: #31593 (comment)

PR-URL: #33085
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants