Skip to content

async_hooks: fix resource stack for deep stacks - #34573

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

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

Conversation

@addaleax

@addaleaxaddaleax commented Jul 31, 2020•
edited
Loading

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator

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

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

@addaleaxaddaleaxJul 31, 2020

Copy link
Copy Markdown
MemberAuthor

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 threadtest/parallel/test-async-local-storage-deep-stack.js Outdated
Comment threadtest/parallel/test-async-local-storage-deep-stack.js Outdated
Comment threadtest/parallel/test-async-local-storage-deep-stack.js Outdated
@puzpuzpuz

puzpuzpuz commented Jul 31, 2020

Copy link
Copy Markdown
Member

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#34319Fixes: nodejs#34556
@addaleax

addaleax commented Jul 31, 2020

Copy link
Copy Markdown
MemberAuthor

@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

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

Copy link
Copy Markdown
Collaborator

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

@puzpuzpuz

puzpuzpuz commented Aug 1, 2020

Copy link
Copy Markdown
Member

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

@addaleaxaddaleax 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

Flarna commented Aug 2, 2020

Copy link
Copy Markdown
Member

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

@addaleax

addaleax commented Aug 2, 2020

Copy link
Copy Markdown
MemberAuthor

@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: #34319Fixes: #34556
PR-URL: #34573
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Gus Caplan <me@gus.host>
@puzpuzpuz

puzpuzpuz commented Aug 3, 2020

Copy link
Copy Markdown
Member

Landed in 2ba93e1

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: #34319Fixes: #34556
PR-URL: #34573
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Gus Caplan <me@gus.host>
@codebyterecodebytere 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: #34319Fixes: #34556
PR-URL: #34573
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Gus Caplan <me@gus.host>
@vmarchaudvmarchaud mentioned this pull request Sep 13, 2020
Merged
@johanneswuerbachjohanneswuerbach 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: #34319Fixes: #34556
PR-URL: #34573
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Gus Caplan <me@gus.host>
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: #34319Fixes: #34556
PR-URL: #34573
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Gus Caplan <me@gus.host>
@codebyterecodebytere mentioned this pull request Sep 28, 2020
Merged
@FlarnaFlarna mentioned this pull request Nov 9, 2020
Closed
3 tasks
@puzpuzpuzpuzpuzpuz 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#34319Fixes: nodejs/node#34556
PR-URL: nodejs/node#34573
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Gus Caplan <me@gus.host>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes
@FlarnaFlarnaFlarna approved these changes
+2 more reviewers
@devsnekdevsnekdevsnek approved these changes
@puzpuzpuzpuzpuzpuzpuzpuzpuz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_hooksIssues and PRs related to the async hooks subsystem.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.review wantedPRs 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