inspector: support for worker inspection in chrome devtools #56759
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
| })); | ||
| } | ||
|
|
||
| test().then(common.mustCall()); |
There was a problem hiding this comment.
I think the test() function wrapper is no longer needed: by default, an unhandled promise rejection makes the process exit with a non-zero exit code.
There was a problem hiding this comment.
Thank you.
I removed the wrapper.
There was a problem hiding this comment.
Sorry for the previous comment. The promise.then(common.mustCall()) pattern is still needed to handle the case where the process exits with the promise unsettled.
There was a problem hiding this comment.
Ah, got it — I’ll bring it back.
|
Tracing the history discussion, it would be great to have @eugeneo, @pavelfeldman and @dgozman weighing in this work |
|
Confirmed with Chrome DevTools team (dsv@, caseq@) that they are good with Node.js implementing the Target domain for worker discovery, or implementing NodeWorker domain support in Chrome DevTools frontend. As the Target domain in this PR is behind the flag |
legendecas
added
the
inspector
Thank you! |
islandryu
force-pushed
the
feat/cdpTarget
branch
from
141ec5a to
984f640
Compare
islandryu
force-pushed
the
feat/cdpTarget
branch
from
984f640 to
fafadd0
Compare
islandryu
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56759 +/- ##
==========================================
+ Coverage 89.21% 90.26% +1.05%
==========================================
Files 663 632 -31
Lines 192001 186021 -5980
Branches 36923 36442 -481
==========================================
- Hits 171290 167914 -3376
+ Misses 13582 10984 -2598
+ Partials 7129 7123 -6
🚀 New features to boost your workflow:
|
|
Issue to devtools-frontend |
| const sessionId = '1'; | ||
| await session.waitForNotification('Target.targetCreated'); | ||
| await session.waitForNotification((notification) => { | ||
| return notification.method === 'Target.attachedToTarget' && |
There was a problem hiding this comment.
Event Target.attachedToTarget should not be emitted without a Target.setAutoAttach command or Target.attachToTarget command.
| // TODO(islandryu): Currently, setAutoAttach applies the main thread's value to | ||
| // all threads. Modify it to be managed per worker thread. | ||
| crdtp::DispatchResponse TargetAgent::setAutoAttach( | ||
| bool auto_attach, bool wait_for_debugger_on_start) { | ||
| auto_attach_ = auto_attach; | ||
| wait_for_debugger_on_start_ = wait_for_debugger_on_start; | ||
|
|
||
| if (auto_attach) { | ||
| for (auto& target : targets_) { | ||
| if (!target.attached) { | ||
| target.attached = true; | ||
| attachedToTarget(target.worker, | ||
| target.target_id, | ||
| target.type, | ||
| target.title, | ||
| target.url); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
setAutoAttach is implemented.
However, I am not able to determine autoAttach for each related Target and waitForDebuggerOnStart.
I have tried several way, but it was difficult to make the changes without affecting the existing nodeWorker, so I would like to make a TODO.
| ); | ||
|
|
||
|
|
||
| const session = await child.connectInspectorSession(); |
There was a problem hiding this comment.
Can you add another suite to guarantee this command will fail when --permission is used?
There was a problem hiding this comment.
I added it below.
Co-authored-by: James M Snell <[email protected]>
There was a problem hiding this comment.
Thank you for working on this! This will need change in the Chrome DevTools Frontend code as well: https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/sdk/ChildTargetManager.ts;l=143-188?q=ChildTargetManager.ts
Would you like to draft the change as well? Otherwise I can help with the work to land the change in devtools-frontend.
src/inspector/target_agent.cc
Outdated
| const std::string& title, | ||
| const std::string& url) { | ||
| std::string target_id = std::to_string(getNextTargetId()); | ||
| std::string type = "worker"; |
There was a problem hiding this comment.
This type conflicts with the web worker type in https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/sdk/ChildTargetManager.ts;l=176?q=ChildTargetManager.ts
If devtools-frontend recognize Node.js worker as a web worker, it will fail to attach to this target as Node.js does not support all domains supported by a web worker.
There was a problem hiding this comment.
Would you like to draft the change as well? Otherwise I can help with the work to land the change in devtools-frontend.
Thx, I have prepared a draft on my end.
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6218804
For now, I named the new type node_worker.
Co-authored-by: Chengzhong Wu <[email protected]>
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
legendecas
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/56759 ✔ Done loading data for nodejs/node/pull/56759 ----------------------------------- PR info ------------------------------------ Title inspector: support for worker inspection in chrome devtools (#56759) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch islandryu:feat/cdpTarget -> nodejs:main Labels c++, lib / src, inspector, needs-ci Commits 16 - inspector: support for worker inspection in chrome devtools - fix/test - format - format - fix test - support setAutoAttach - fix stoi error - add permission option test - Update doc/api/cli.md - fix doc - Update src/inspector/target_agent.h - fix string_view - fix lint - format - Update src/inspector/node_inspector.gypi - change worker name Committers 2 - islandryu <[email protected]> - GitHub <[email protected]> PR-URL: https://github.com/nodejs/node/pull/56759 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56759 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 25 Jan 2025 13:31:05 GMT ✔ Approvals: 2 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/56759#pullrequestreview-2786639812 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56759#pullrequestreview-2765056254 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-05-06T23:44:04Z: https://ci.nodejs.org/job/node-test-pull-request/66661/ - Querying data for job/node-test-pull-request/66661/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 56759 From https://github.com/nodejs/node * branch refs/pull/56759/merge -> FETCH_HEAD ✔ Fetched commits as cfd2021c35ea..5c317564e5b1 -------------------------------------------------------------------------------- Auto-merging doc/api/cli.md Auto-merging src/inspector/node_inspector.gypi Auto-merging src/inspector/node_protocol.pdl Auto-merging src/inspector_agent.cc Auto-merging src/inspector_io.cc Auto-merging src/node_options.cc Auto-merging src/node_options.h [main 5c70e2928e] inspector: support for worker inspection in chrome devtools Author: islandryu <[email protected]> Date: Sat Jan 25 17:26:31 2025 +0900 11 files changed, 322 insertions(+), 23 deletions(-) create mode 100644 src/inspector/target_agent.cc create mode 100644 src/inspector/target_agent.h create mode 100644 test/parallel/test-inspector-worker-target.js [main 07931625ed] fix/test Author: islandryu <[email protected]> Date: Mon Jan 27 20:35:02 2025 +0900 2 files changed, 14 insertions(+), 19 deletions(-) Auto-merging src/inspector/node_protocol.pdl Auto-merging src/inspector_agent.cc Auto-merging src/inspector_io.cc [main 348f5a820a] format Author: islandryu <[email protected]> Date: Wed Jan 29 23:23:19 2025 +0900 6 files changed, 81 insertions(+), 77 deletions(-) Auto-merging doc/api/cli.md Auto-merging src/inspector_agent.cc [main 8eadb0fa5f] format Author: islandryu <[email protected]> Date: Thu Jan 30 21:13:48 2025 +0900 3 files changed, 9 insertions(+), 14 deletions(-) [main 670a92df75] fix test Author: islandryu <[email protected]> Date: Mon Feb 3 22:09:53 2025 +0900 3 files changed, 46 insertions(+), 16 deletions(-) create mode 100644 test/fixtures/inspect-worker/index.js create mode 100644 test/fixtures/inspect-worker/worker.js Auto-merging src/inspector/node_protocol.pdl [main 4a3313b65d] support setAutoAttach Author: islandryu <[email protected]> Date: Sat Feb 8 20:13:16 2025 +0900 6 files changed, 111 insertions(+), 40 deletions(-) Auto-merging src/inspector_io.cc [main b9a806e927] fix stoi error Author: islandryu <[email protected]> Date: Sun Feb 9 18:32:09 2025 +0900 1 file changed, 11 insertions(+), 6 deletions(-) [main 818a0668b8] add permission option test Author: islandryu <[email protected]> Date: Mon Mar 31 11:19:56 2025 +0900 1 file changed, 18 insertions(+) Auto-merging doc/api/cli.md [main e99b71c09d] Update doc/api/cli.md Author: Shima Ryuhei <[email protected]> Date: Mon Mar 31 14:20:22 2025 +0900 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging doc/api/cli.md [main e052d4b31e] fix doc Author: islandryu <[email protected]> Date: Mon Mar 31 14:21:32 2025 +0900 1 file changed, 1 insertion(+), 1 deletion(-) [main ebb0bbf228] Update src/inspector/target_agent.h Author: Shima Ryuhei <[email protected]> Date: Mon Apr 14 23:45:19 2025 +0900 1 file changed, 1 insertion(+) Auto-merging src/inspector_agent.cc [main a9c383405c] fix string_view Author: islandryu <[email protected]> Date: Mon Apr 14 23:32:47 2025 +0900 4 files changed, 21 insertions(+), 19 deletions(-) [main d7e7f71330] fix lint Author: islandryu <[email protected]> Date: Tue Apr 15 00:05:49 2025 +0900 1 file changed, 5 insertions(+), 4 deletions(-) [main c468f87317] format Author: islandryu <[email protected]> Date: Tue Apr 15 10:11:27 2025 +0900 1 file changed, 4 insertions(+), 4 deletions(-) Auto-merging src/inspector/node_inspector.gypi [main 30c907ff29] Update src/inspector/node_inspector.gypi Author: Shima Ryuhei <[email protected]> Date: Thu Apr 17 17:21:03 2025 +0900 1 file changed, 1 deletion(-) [main 40d8f016e7] change worker name Author: islandryu <[email protected]> Date: Thu Apr 17 22:08:28 2025 +0900 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 16 commits in the PR. Attempting autorebase. Rebasing (2/32) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- inspector: support for worker inspection in chrome devtoolshttps://github.com/nodejs/node/actions/runs/14973042167 |
legendecas
added
commit-queue-squash
nodejs-github-bot
removed
the
commit-queue
|
Landed in 2281a04 |
Fixes: #56343 PR-URL: #56759 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
This PR introduces support for inspecting workers in Chrome DevTools. I would like to gather some feedback to confirm whether there seem to be no issues with this approach. If the approach in this PR deviates significantly from the desired direction, I am happy to close it. However, if the general approach seems acceptable, I will proceed with this PR and submit a change request to Chrome DevTools.
Summary of Changes
This implementation uses the
attachedToTargetevent to notify the creation of workers. More details on the protocol can be found [here](https://chromedevtools.github.io/devtools-protocol/tot/Target/#event-attachedToTarget).This sequence is how to worker inspection works.
When a worker is created, attachedToTarget is notified to the server, and the server thread allocates methods from the client to the appropriate thread based on the sessionId.
sequenceDiagram participant DevtoolClient as DevTool Client participant ServerThread as Server Thread participant MainThread as Main Thread participant WorkerThread as Worker Thread DevtoolClient ->> ServerThread: attach ServerThread ->> MainThread: attach MainThread ->> MainThread: execute program MainThread ->> WorkerThread: create worker MainThread ->> ServerThread: attachedToTarget ServerThread ->> DevtoolClient: attachedToTarget DevtoolClient ->> ServerThread: debugger method ServerThread ->> ServerThread: Check which thread to send to by sessionId ServerThread ->> WorkerThread: Send to main thread or worker threadHow to Verify
Currently, dedicated DevTools for Node.js do not support worker targets. For verification, use
inspector.html:Steps:
index.jsandworker.jsfiles for testing:index.js:worker.js:Currently, the results of console.log within the worker are being sent twice, but I'm working on fixing this issue.Additional Information
The approach used in this PR follows the CDP (Chrome DevTools Protocol) and is different from the Node.js worker debugging protocol used in VSCode. It adheres to the existing CDP protocol to ensure compatibility.
fix #56343