Skip to content

inspector: report all workers #28872

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

Closed
eugeneo wants to merge 5 commits into from
Closed

inspector: report all workers #28872

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8cbf72e
inspector: report all workers
eugeneo Jul 26, 2019
337bc4a
squash: address code review comments
Jul 28, 2019
16eee7a
squash: do not use NodeWorker domain in a worker.
eugeneo Jul 29, 2019
fdd7a24
fixup
Trott Jul 29, 2019
d804348
fixup
Trott Jul 29, 2019
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
Next Next commit
inspector: report all workers 
Main thread (the one that WS endpoint connects to) should be able
to report all workers.
  • Loading branch information
@eugeneo
eugeneo committed Jul 27, 2019
commit 8cbf72e8317e5e90cd67028b7b6a94734100b6c6
7 changes: 7 additions & 0 deletions src/inspector/worker_inspector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ class ParentInspectorHandle {
std::shared_ptr<MainThreadHandle> parent_thread,
bool wait_for_connect);
~ParentInspectorHandle();
std::unique_ptr<ParentInspectorHandle> NewParentInspectorHandle(
int thread_id, const std::string& url) {
return std::make_unique<ParentInspectorHandle>(thread_id,
url,
parent_thread_,
wait_);
}
void WorkerStarted(std::shared_ptr<MainThreadHandle> worker_thread,
bool waiting);
bool WaitForConnect() {
Expand Down
21 changes: 16 additions & 5 deletions src/inspector_agent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,17 +237,21 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
tracing_agent_ =
std::make_unique<protocol::TracingAgent>(env, main_thread_);
tracing_agent_->Wire(node_dispatcher_.get());
worker_agent_ = std::make_unique<protocol::WorkerAgent>(worker_manager);
worker_agent_->Wire(node_dispatcher_.get());
if (worker_manager) {
worker_agent_ = std::make_unique<protocol::WorkerAgent>(worker_manager);
worker_agent_->Wire(node_dispatcher_.get());
}
runtime_agent_ = std::make_unique<protocol::RuntimeAgent>();
runtime_agent_->Wire(node_dispatcher_.get());
}

~ChannelImpl() override {
tracing_agent_->disable();
tracing_agent_.reset(); // Dispose before the dispatchers
worker_agent_->disable();
worker_agent_.reset(); // Dispose before the dispatchers
if (worker_agent_) {
worker_agent_->disable();
worker_agent_.reset(); // Dispose before the dispatchers
Copy link
Copy Markdown
Member

@addaleax addaleax Jul 27, 2019

Choose a reason for hiding this comment

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

Btw, if you need member fields to be destroyed in a specific order, it might make the sense to re-order the fields so that that happens automatically?

Copy link
Copy Markdown
Contributor Author

@eugeneo eugeneo Jul 28, 2019

Choose a reason for hiding this comment

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

I would like to have this specified explicitly so I don't cause crashes by moving fields in the header file. Had that happen before :)

}
runtime_agent_->disable();
runtime_agent_.reset(); // Dispose before the dispatchers
}
Expand Down Expand Up @@ -670,6 +674,9 @@ class NodeInspectorClient : public V8InspectorClient {
}

std::shared_ptr<WorkerManager> getWorkerManager() {
if (!is_main_) {
return nullptr;
}
if (worker_manager_ == nullptr) {
worker_manager_ =
std::make_shared<WorkerManager>(getThreadHandle());
Expand Down Expand Up @@ -968,7 +975,11 @@ void Agent::SetParentHandle(

std::unique_ptr<ParentInspectorHandle> Agent::GetParentHandle(
int thread_id, const std::string& url) {
return client_->getWorkerManager()->NewParentHandle(thread_id, url);
if (!parent_handle_) {
return client_->getWorkerManager()->NewParentHandle(thread_id, url);
} else {
return parent_handle_->NewParentInspectorHandle(thread_id, url);
}
}

void Agent::WaitForConnect() {
Expand Down
70 changes: 70 additions & 0 deletions test/parallel/test-inspector-workers-flat-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
'use strict';
const common = require('../common');
common.skipIfInspectorDisabled();

const { Session } = require('inspector');
const { Worker, isMainThread, parentPort } = require('worker_threads');

const MAX_DEPTH = 3;

let rootWorker = null;

function runTest() {
Copy link
Copy Markdown
Member

@addaleax addaleax Jul 27, 2019

Choose a reason for hiding this comment

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

const runTest = common.mustCall(() => { ..., to make sure that this is actually run?

Copy link
Copy Markdown
Contributor Author

@eugeneo eugeneo Jul 28, 2019

Choose a reason for hiding this comment

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

Done

let reportedWorkersCount = 0;
const session = new Session();
session.connect();
session.on('NodeWorker.attachedToWorker', ({ params: { workerInfo } }) => {
Copy link
Copy Markdown
Member

@addaleax addaleax Jul 27, 2019

Choose a reason for hiding this comment

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

ditto here, common.mustCall(..., MAX_DEPTH) for the event handler to make sure this actually runs as often as expected?

Copy link
Copy Markdown
Contributor Author

@eugeneo eugeneo Jul 28, 2019

Choose a reason for hiding this comment

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

Done

console.log(`Worker ${workerInfo.title} was reported`);
if (++reportedWorkersCount === MAX_DEPTH) {
rootWorker.postMessage({ done: true });
}
});
session.post('NodeWorker.enable', { waitForDebuggerOnStart: false });
}

function processMessage({ child }) {
console.log(`Worker ${child} is running`);
if (child === MAX_DEPTH) {
runTest();
}
}

function workerCallback(message) {
parentPort.postMessage(message);
}

function startWorker(depth, messageCallback) {
const worker = new Worker(__filename);
worker.on('message', messageCallback);
worker.postMessage({ depth });
return worker;
}

function runMainThread() {
rootWorker = startWorker(1, processMessage);
}

function runWorkerThread() {
let worker = null;
Copy link
Copy Markdown
Member

@addaleax addaleax Jul 27, 2019

Choose a reason for hiding this comment

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

Can you rename this to child or childWorker to be more explicit?

Copy link
Copy Markdown
Contributor Author

@eugeneo eugeneo Jul 28, 2019

Choose a reason for hiding this comment

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

Done

parentPort.on('message', ({ child, depth, done }) => {
if (done) {
if (worker) {
worker.postMessage({ done: true });
}
parentPort.close();
} else if (depth) {
parentPort.postMessage({ child: depth });
if (depth < MAX_DEPTH) {
worker = startWorker(depth + 1, workerCallback);
}
} else if (child) {
parentPort.postMessage({ child });
}
});
}

if (isMainThread) {
runMainThread();
} else {
runWorkerThread();
}