Skip to content

async_hooks: fix resource stack for deep stacks#34573

Closed
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:fix-34556
Closed

async_hooks: fix resource stack for deep stacks#34573
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:fix-34556

Conversation

@addaleax
Copy link
Copy Markdown
Member

@addaleax addaleax commented Jul 31, 2020 •
edited
Loading

460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: #34319
Fixes: #34556

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Jul 31, 2020

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4182

@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 31, 2020
@addaleax addaleax mentioned this pull request Jul 31, 2020
Closed
addaleax
addaleax commented Jul 31, 2020
View reviewed changes
Comment thread test/parallel/test-async-local-storage-deep-stack.js Outdated
Copy link
Copy Markdown
Member Author

@addaleax addaleax Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, this still crashes for me for larger values (e.g. 2000), but with a different error and unrelated to the bug that this is fixing, and depending on --stack-size, i.e. there’s probably something wrong with our error handling here.

puzpuzpuz
puzpuzpuz reviewed Jul 31, 2020
View reviewed changes
Comment thread test/parallel/test-async-local-storage-deep-stack.js Outdated
Comment thread test/parallel/test-async-local-storage-deep-stack.js Outdated
Comment thread test/parallel/test-async-local-storage-deep-stack.js Outdated
@puzpuzpuz
Copy link
Copy Markdown
Member

puzpuzpuz commented Jul 31, 2020

Does it make sense to mention that this PR fixes #34556?

@addaleax
async_hooks: fix resource stack for deep stacks
da69cd3 
460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: nodejs#34319
Fixes: nodejs#34556
@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Jul 31, 2020

@puzpuzpuz Yup, added a Fixes: tag here. As mentioned above, it doesn’t fix the problem for when the stack gets very large, so there’s still a bug around, but I think it’s fair to say that it’s distinct from #34556.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 1, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32591/ (:green_heart:)

@puzpuzpuz
Copy link
Copy Markdown
Member

puzpuzpuz commented Aug 1, 2020

@nodejs/async_hooks looks like this fix requires another reviewer.

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Aug 1, 2020
@Flarna
Copy link
Copy Markdown
Member

Flarna commented Aug 2, 2020

Of curiosity: Why do we have a managed and a native stack? Couldn't we use one stack shared between managed and native?

@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Aug 2, 2020

@Flarna See #34319 – it’s for performance reasons, because we don’t have a data structure that is both fast to access from C++ and fast to access from JS.

puzpuzpuz pushed a commit that referenced this pull request Aug 3, 2020
@addaleax @puzpuzpuz
async_hooks: fix resource stack for deep stacks
2ba93e1 
460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: #34319
Fixes: #34556

PR-URL: #34573
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@puzpuzpuz
Copy link
Copy Markdown
Member

puzpuzpuz commented Aug 3, 2020

Landed in 2ba93e1

@puzpuzpuz puzpuzpuz closed this Aug 3, 2020
codebytere pushed a commit that referenced this pull request Aug 5, 2020
@addaleax @codebytere
async_hooks: fix resource stack for deep stacks
5aed83c 
460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: #34319
Fixes: #34556

PR-URL: #34573
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@codebytere codebytere mentioned this pull request Aug 10, 2020
Merged
codebytere pushed a commit that referenced this pull request Aug 11, 2020
@addaleax @codebytere
async_hooks: fix resource stack for deep stacks
7c4e1db 
460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: #34319
Fixes: #34556

PR-URL: #34573
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@vmarchaud vmarchaud mentioned this pull request Sep 13, 2020
Merged
@johanneswuerbach johanneswuerbach mentioned this pull request Sep 21, 2020
Closed
4 tasks
@addaleax addaleax deleted the fix-34556 branch September 21, 2020 23:17
addaleax added a commit that referenced this pull request Sep 22, 2020
@addaleax
async_hooks: fix resource stack for deep stacks
afdbb01 
460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: #34319
Fixes: #34556

PR-URL: #34573
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
addaleax added a commit that referenced this pull request Sep 22, 2020
@addaleax
async_hooks: fix resource stack for deep stacks
cfc769b 
460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: #34319
Fixes: #34556

PR-URL: #34573
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
@Flarna Flarna mentioned this pull request Nov 9, 2020
Closed
3 tasks
@puzpuzpuz puzpuzpuz mentioned this pull request Nov 12, 2020
Closed
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
@addaleax @johanneswuerbach
async_hooks: fix resource stack for deep stacks
5362bef 
460c81d introduced a bug where the execution resource was not
stored properly if we needed to call into C++ to extend the stack size.
Fix that bug by always storing the resource.

Refs: nodejs/node#34319
Fixes: nodejs/node#34556

PR-URL: nodejs/node#34573
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@Flarna Flarna Flarna approved these changes

+2 more reviewers

@devsnek devsnek devsnek approved these changes

@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AsyncLocalStorage: Cannot read property 'Symbol(kResourceStore)'

6 participants

@addaleax @nodejs-github-bot @puzpuzpuz @Flarna @benjamingr @devsnek