Skip to content
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.

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

[WIP] inspector: support for worker inspection in chrome devtools #56759

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

islandryu
Copy link
Contributor

@islandryu islandryu commented Jan 25, 2025

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 attachedToTarget event 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 thread
Loading

How to Verify

Currently, dedicated DevTools for Node.js do not support worker targets. For verification, use inspector.html:

devtools://devtools/bundled/inspector.html?ws=127.0.0.1:9229/(sessionId)

Steps:

  1. Start the Node.js process with the following command:
    node --inspect-brk --experimental-worker-inspection index.js
  2. Use the following index.js and worker.js files for testing:

index.js:

const { Worker } = require('worker_threads');
const workerPath =  __dirname + '/worker.js';
const worker = new Worker(workerPath);

worker.js:

console.log("worker thread");
process.on('exit', () => {
  console.log('Worker1: Exiting...');
});

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@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. needs-ci PRs that need a full CI run. labels Jan 25, 2025
}));
}

test().then(common.mustCall());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@legendecas
Copy link
Member

Tracing the history discussion, it would be great to have @eugeneo, @pavelfeldman and @dgozman weighing in this work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--inspect-brk causes code with workers to hang
4 participants