Skip to content

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

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:interrupt
Closed

src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt#26935
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:interrupt

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Mar 26, 2019

Copy link
Copy Markdown
Member

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

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
Copy Markdown
Collaborator

@nodejs-github-botnodejs-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
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@joyeecheung

joyeecheung commented Mar 26, 2019

Copy link
Copy Markdown
MemberAuthor

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

@BridgeARBridgeAR 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
Copy Markdown
Collaborator

@joyeecheung

Copy link
Copy Markdown
MemberAuthor

7aad63b

joyeecheung added a commit that referenced this pull request Mar 30, 2019
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 <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
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 <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
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 <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggsBethGriggs mentioned this pull request Apr 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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