FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt by joyeecheung · Pull Request #26935 · nodejs/node · GitHub

/ node Public

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

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:interrupt
ConversationCommits (1)ChecksFiles changed
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 commented Mar 26, 2019 •
edited
Loading

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

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 commented Mar 26, 2019

Copy link
Copy Markdown
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 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 requested review from eugeneo and ofrobots March 26, 2019 20:07

nodejs-github-bot commented Mar 26, 2019

Copy link
Copy Markdown
Collaborator

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

joyeecheung mentioned this pull request Mar 26, 2019
Closed

joyeecheung commented Mar 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 27, 2019

nodejs-github-bot commented Mar 28, 2019

Copy link
Copy Markdown
Collaborator

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

joyeecheung commented Mar 30, 2019

Copy link
Copy Markdown
Member Author

7aad63b

joyeecheung added a commit that referenced this pull request Mar 30, 2019
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 <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
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 <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
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 <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>
BethGriggs mentioned this pull request Apr 9, 2019
Merged
joyeecheung mentioned this pull request Sep 29, 2019
Closed
4 tasks
joyeecheung mentioned this pull request Feb 25, 2020
Closed
baby636 mentioned this pull request Jul 12, 2023
Open
joyeecheung mentioned this pull request Aug 4, 2026
Merged
nodejs-github-bot pushed a commit that referenced this pull request Aug 14, 2026
inspector: avoid calling into JS from V8 interrupts
3a35e48 
Our inspector implementation dispatches inspector messages from
a V8 interrupt handler, so they could be handled during an arbitrary
point of JS execution where re-calling into another irrelevant JS
code is not safe.

This patch tracks V8 interrupt state in this case and rewrite the
async hook toggling as state reconciliation, so requests only record
the desired state, which is applied once calling into JS is possible
and safe, and the actual invocation is deferred to an immediate when
inside an interrupt. This simplifies the previous mechanism and
makes re-entracy and early termination safer.

Drive-by: skip installing command line API extensions during
teardown when calling into JS is no longer safe.

Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #65028
Refs: https://issues.chromium.org/u/1/issues/42212250
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
Refs: #26935
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
inspector: avoid calling into JS from V8 interrupts
c19889d 
Our inspector implementation dispatches inspector messages from
a V8 interrupt handler, so they could be handled during an arbitrary
point of JS execution where re-calling into another irrelevant JS
code is not safe.

This patch tracks V8 interrupt state in this case and rewrite the
async hook toggling as state reconciliation, so requests only record
the desired state, which is applied once calling into JS is possible
and safe, and the actual invocation is deferred to an immediate when
inside an interrupt. This simplifies the previous mechanism and
makes re-entracy and early termination safer.

Drive-by: skip installing command line API extensions during
teardown when calling into JS is no longer safe.

Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #65028
Refs: https://issues.chromium.org/u/1/issues/42212250
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
Refs: #26935
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
inspector: avoid calling into JS from V8 interrupts
0998016 
Our inspector implementation dispatches inspector messages from
a V8 interrupt handler, so they could be handled during an arbitrary
point of JS execution where re-calling into another irrelevant JS
code is not safe.

This patch tracks V8 interrupt state in this case and rewrite the
async hook toggling as state reconciliation, so requests only record
the desired state, which is applied once calling into JS is possible
and safe, and the actual invocation is deferred to an immediate when
inside an interrupt. This simplifies the previous mechanism and
makes re-entracy and early termination safer.

Drive-by: skip installing command line API extensions during
teardown when calling into JS is no longer safe.

Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #65028
Refs: https://issues.chromium.org/u/1/issues/42212250
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
Refs: #26935
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
JungMinu JungMinu approved these changes
ofrobots Awaiting requested review from ofrobots
+1 more reviewer
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 outstanding review comments, 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

Back | FazBrowse Home | New Git URL