Skip to content

src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt #26935

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt #26935

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 26, 2019 •
edited
Loading

If Debugger.setAsyncCallStackDepth is sent during bootstrap,
we cannot immediately call into JS to enable the hooks, which could
interrupt the JS execution of bootstrap. So instead we save the
notification in the inspector agent if it's sent in the middle of
bootstrap, and process the notification later during pre-execution.

Example error caused by the interrupt calling into JS directly:

00:28:08     [err] async_hooks.js:32
00:28:08     [err]   async_id_symbol, trigger_async_id_symbol,
00:28:08     [err]   ^
00:28:08     [err] 
00:28:08     [err] TypeError: Cannot destructure property `async_id_symbol` of 'undefined' or 'null'.
00:28:08     [err]     at async_hooks.js:31:7
00:28:08     [err]     at NativeModule.compile (internal/bootstrap/loaders.js:300:5)
00:28:08     [err]     at nativeModuleRequire (internal/bootstrap/loaders.js:188:14)
00:28:08     [err]     at lazyHookCreation (internal/inspector_async_hook.js:8:26)
00:28:08     [err]     at enable (internal/inspector_async_hook.js:47:27)
00:28:08     [err]     at internal/async_hooks.js:1:1
00:28:08     [err]     at NativeModule.compile (internal/bootstrap/loaders.js:300:5)
00:28:08     [err]     at nativeModuleRequire (internal/bootstrap/loaders.js:188:14)
00:28:08     [err]     at internal/process/task_queues.js:31:5
00:28:08     [err]     at NativeModule.compile (internal/bootstrap/loaders.js:300:5)
00:28:08     [err]     at nativeModuleRequire (internal/bootstrap/loaders.js:188:14)

Refs: #26798

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@joyeecheung
src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt
d5f9395 
If Debugger.setAsyncCallStackDepth is sent during bootstrap,
we cannot immediately call into JS to enable the hooks, which could
interrupt the JS execution of bootstrap. So instead we save the
notification in the inspector agent if it's sent in the middle of
bootstrap, and process the notification later here.

Refs: nodejs#26798
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3063/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 26, 2019
@joyeecheung joyeecheung requested review from eugeneo and ofrobots March 26, 2019 20:07
@nodejs-github-bot
Copy link
Collaborator

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

@joyeecheung joyeecheung mentioned this pull request Mar 26, 2019
Closed
@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 26, 2019 •
edited
Loading

Stress test (see if the CHECK fails on win2008r2-vs2017) : https://ci.nodejs.org/job/node-stress-single-test/2194/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 27, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@joyeecheung
Copy link
Member Author

7aad63b

joyeecheung added a commit that referenced this pull request Mar 30, 2019
@joyeecheung
src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt
7aad63b 
If Debugger.setAsyncCallStackDepth is sent during bootstrap,
we cannot immediately call into JS to enable the hooks, which could
interrupt the JS execution of bootstrap. So instead we save the
notification in the inspector agent if it's sent in the middle of
bootstrap, and process the notification later here.

Refs: #26798

PR-URL: #26935
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
@joyeecheung @BethGriggs
src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt
4e1fe29 
If Debugger.setAsyncCallStackDepth is sent during bootstrap,
we cannot immediately call into JS to enable the hooks, which could
interrupt the JS execution of bootstrap. So instead we save the
notification in the inspector agent if it's sent in the middle of
bootstrap, and process the notification later here.

Refs: #26798

PR-URL: #26935
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
@joyeecheung @BethGriggs
src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt
e776b01 
If Debugger.setAsyncCallStackDepth is sent during bootstrap,
we cannot immediately call into JS to enable the hooks, which could
interrupt the JS execution of bootstrap. So instead we save the
notification in the inspector agent if it's sent in the middle of
bootstrap, and process the notification later here.

Refs: #26798

PR-URL: #26935
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
@joyeecheung joyeecheung mentioned this pull request Sep 29, 2019
Closed
4 tasks
@joyeecheung joyeecheung mentioned this pull request Feb 25, 2020
Closed
@baby636 baby636 mentioned this pull request Jul 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@JungMinu JungMinu JungMinu approved these changes

@ofrobots ofrobots Awaiting requested review from ofrobots

+1 more reviewer

@eugeneo eugeneo eugeneo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung @nodejs-github-bot @eugeneo @addaleax @JungMinu @BridgeAR