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.

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

inspector: supported NodeRuntime domain in worker #27706

Closed
wants to merge 1 commit into from
Closed

inspector: supported NodeRuntime domain in worker #27706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/inspector/node_protocol.pdl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ experimental domain NodeWorker
# Detaches from all running workers and disables attaching to new workers as they are started.
command disable

# Detached from the worker with given sessionId.
command detach
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need attach for symmetry? The problem I see is that we do not track worker separately from a session so introducing attach would definitely make it more confusing...

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case should we have a little bit crazy NodeRuntime.disconnect method that works for main and worker thread?
I don't know any use case for NodeWorker.attach. Since workers are not waiting for attach by default, this method does not add any value without waitForDebuggerOnStart flag. Could we add it later when we know use case better?

Copy link
Contributor

Choose a reason for hiding this comment

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

One potential problem I see with the detach is that the worker is kind of "lost" once session is detached. A frontend may detach from the worker early and then there will be no way of attaching again. IMHO, attach can be implemented later if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case should we have a little bit crazy NodeRuntime.disconnect method that works for main and worker thread?

Don't think this is necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to ping me when someone would ask about this feature!

parameters
SessionID sessionId

# Issued when attached to a worker.
event attachedToWorker
parameters
Expand Down
4 changes: 0 additions & 4 deletions src/inspector/runtime_agent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ void RuntimeAgent::Wire(UberDispatcher* dispatcher) {
}

DispatchResponse RuntimeAgent::notifyWhenWaitingForDisconnect(bool enabled) {
if (!env_->owns_process_state()) {
return DispatchResponse::Error(
"NodeRuntime domain can only be used through main thread sessions");
}
notify_when_waiting_for_disconnect_ = enabled;
return DispatchResponse::OK();
}
Expand Down
5 changes: 5 additions & 0 deletions src/inspector/worker_agent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,11 @@ DispatchResponse WorkerAgent::disable() {
return DispatchResponse::OK();
}

DispatchResponse WorkerAgent::detach(const String& sessionId) {
workers_->Detached(sessionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need a detachedFromWorker notification in response to a frontend request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! It is how most domains (e.g. Target domain) in Chrome works. Notification means that worker session was detached for some reasons, one of the reasons might be explicit protocol request. It is easy to ignore this notification on frontend if needed.

return DispatchResponse::OK();
}

void NodeWorkers::WorkerCreated(const std::string& title,
const std::string& url,
bool waiting,
Expand Down
1 change: 1 addition & 0 deletions src/inspector/worker_agent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class WorkerAgent : public NodeWorker::Backend {

DispatchResponse enable(bool waitForDebuggerOnStart) override;
DispatchResponse disable() override;
DispatchResponse detach(const String& sessionId) override;

private:
std::shared_ptr<NodeWorker::Frontend> frontend_;
Expand Down
18 changes: 12 additions & 6 deletions src/inspector_agent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,8 +464,8 @@ class NodeInspectorClient : public V8InspectorClient {
runMessageLoop();
}

void waitForIoShutdown() {
waiting_for_io_shutdown_ = true;
void waitForSessionsDisconnect() {
waiting_for_sessions_disconnect_ = true;
runMessageLoop();
}

Expand Down Expand Up @@ -540,6 +540,8 @@ class NodeInspectorClient : public V8InspectorClient {
}
contextDestroyed(env_->context());
}
if (waiting_for_sessions_disconnect_ && !is_main_)
waiting_for_sessions_disconnect_ = false;
}

void dispatchMessageFromFrontend(int session_id, const StringView& message) {
Expand Down Expand Up @@ -670,8 +672,9 @@ class NodeInspectorClient : public V8InspectorClient {
bool shouldRunMessageLoop() {
if (waiting_for_frontend_)
return true;
if (waiting_for_io_shutdown_ || waiting_for_resume_)
if (waiting_for_sessions_disconnect_ || waiting_for_resume_) {
return hasConnectedSessions();
}
return false;
}

Expand Down Expand Up @@ -715,7 +718,7 @@ class NodeInspectorClient : public V8InspectorClient {
int next_session_id_ = 1;
bool waiting_for_resume_ = false;
bool waiting_for_frontend_ = false;
bool waiting_for_io_shutdown_ = false;
bool waiting_for_sessions_disconnect_ = false;
// Allows accessing Inspector from non-main threads
std::unique_ptr<MainThreadInterface> interface_;
std::shared_ptr<WorkerManager> worker_manager_;
Expand Down Expand Up @@ -811,11 +814,14 @@ void Agent::WaitForDisconnect() {
fprintf(stderr, "Waiting for the debugger to disconnect...\n");
fflush(stderr);
}
if (!client_->notifyWaitingForDisconnect())
if (!client_->notifyWaitingForDisconnect()) {
client_->contextDestroyed(parent_env_->context());
} else if (is_worker) {
client_->waitForSessionsDisconnect();
}
if (io_ != nullptr) {
io_->StopAcceptingNewConnections();
client_->waitForIoShutdown();
client_->waitForSessionsDisconnect();
}
}

Expand Down
47 changes: 47 additions & 0 deletions test/parallel/test-worker-debug.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,52 @@ async function testTwoWorkers(session, post) {
await Promise.all([worker1Exited, worker2Exited]);
}

async function testWaitForDisconnectInWorker(session, post) {
console.log('Test NodeRuntime.waitForDisconnect in worker');

const sessionWithoutWaiting = new Session();
sessionWithoutWaiting.connect();
const sessionWithoutWaitingPost = doPost.bind(null, sessionWithoutWaiting);

await sessionWithoutWaitingPost('NodeWorker.enable', {
waitForDebuggerOnStart: true
});
await post('NodeWorker.enable', { waitForDebuggerOnStart: true });

const attached = [
waitForWorkerAttach(session),
waitForWorkerAttach(sessionWithoutWaiting)
];

let worker = null;
const exitPromise = runWorker(2, (w) => worker = w);

const [{ sessionId: sessionId1 }, { sessionId: sessionId2 }] =
await Promise.all(attached);

const workerSession1 = new WorkerSession(session, sessionId1);
const workerSession2 = new WorkerSession(sessionWithoutWaiting, sessionId2);

await workerSession2.post('Runtime.enable');
await workerSession1.post('Runtime.enable');
await workerSession1.post('NodeRuntime.notifyWhenWaitingForDisconnect', {
enabled: true
});
await workerSession1.post('Runtime.runIfWaitingForDebugger');

worker.postMessage('resume');

await waitForEvent(workerSession1, 'NodeRuntime.waitingForDisconnect');
post('NodeWorker.detach', { sessionId: sessionId1 });
await waitForEvent(workerSession2, 'Runtime.executionContextDestroyed');

await exitPromise;

await post('NodeWorker.disable');
await sessionWithoutWaitingPost('NodeWorker.disable');
sessionWithoutWaiting.disconnect();
}

async function test() {
const session = new Session();
session.connect();
Expand All @@ -219,6 +265,7 @@ async function test() {

await testNoWaitOnStart(session, post);
await testTwoWorkers(session, post);
await testWaitForDisconnectInWorker(session, post);

session.disconnect();
console.log('Test done');
Expand Down