inspector: report all workers by eugeneo · Pull Request #28872 · nodejs/node

eugeneo · 2019-07-27T00:14:09Z

Main thread (the one that WS endpoint connects to) should be able
to report all workers.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Main thread (the one that WS endpoint connects to) should be able to report all workers.

addaleax · 2019-07-27T00:28:35Z

src/inspector_agent.cc

-    worker_agent_.reset();  // Dispose before the dispatchers
+    if (worker_agent_) {
+      worker_agent_->disable();
+      worker_agent_.reset();  // Dispose before the dispatchers


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?

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 :)

addaleax · 2019-07-27T00:32:38Z

test/parallel/test-inspector-workers-flat-list.js

+}
+
+function runWorkerThread() {
+  let worker = null;


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

addaleax · 2019-07-27T00:33:43Z

test/parallel/test-inspector-workers-flat-list.js

+
+let rootWorker = null;
+
+function runTest() {


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

addaleax · 2019-07-27T00:34:23Z

test/parallel/test-inspector-workers-flat-list.js

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


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

nodejs-github-bot · 2019-07-29T00:24:31Z

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

Trott · 2019-07-29T05:23:50Z

I didn't investigate to confirm, but the CI results look like there are relevant failures when running tests from workers (with tools/test.py --worker). https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/8271/testReport/

Trott

Putting in a request changes so no one lands this without noticing that there's a CI failure that needs to be addressed.

eugeneo · 2019-07-29T16:17:46Z

@Trott thanks for reminding! Those two tests should not run in a worker as child workers are now reported from the top level only.

test updated

nodejs-github-bot · 2019-07-29T17:08:06Z

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

Trott · 2019-07-29T17:30:24Z

Looks like parallel/test-inspector-workers-flat-list fails everywhere now because it uses common.skipIfWorker() but also new Worker(__filename). It needs to either launch a worker in some other file or else be able to detect if it was launched from this file (say, by checking for a specific value in workerData) and not skipIfWorker() in that case.

Trott · 2019-07-29T21:07:40Z

Looks like parallel/test-inspector-workers-flat-list fails everywhere now because it uses common.skipIfWorker() but also new Worker(__filename). It needs to either launch a worker in some other file or else be able to detect if it was launched from this file (say, by checking for a specific value in workerData) and not skipIfWorker() in that case.

I took the liberty of committing the fix for that myself. Hope that's OK. If not, remove my commit and add your preferred change.

Trott · 2019-07-29T22:39:25Z

Well, that's what I get for using the GitHub interface as an IDE. Let's try again...

nodejs-github-bot · 2019-07-30T02:44:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24754/ ✔️

Trott · 2019-07-30T03:33:28Z

CI passed!

Trott · 2019-07-30T03:33:46Z

@addaleax @jasnell Still looks good to you?

eugeneo · 2019-07-30T23:17:15Z

@Trott - thank you for fixing the tests!

Main thread (the one that WS endpoint connects to) should be able to report all workers. PR-URL: nodejs#28872 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2019-07-31T04:18:01Z

Landed in 7435dc8

Main thread (the one that WS endpoint connects to) should be able to report all workers. PR-URL: #28872 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

inspector: report all workers

8cbf72e

Main thread (the one that WS endpoint connects to) should be able to report all workers.

eugeneo added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Jul 27, 2019

eugeneo mentioned this pull request Jul 27, 2019

inspector: new API - Session.connectToMainThread #28870

Merged

4 tasks

addaleax approved these changes Jul 27, 2019

View reviewed changes

addaleax added the worker Issues and PRs related to Worker support. label Jul 27, 2019

squash: address code review comments

337bc4a

jasnell approved these changes Jul 29, 2019

View reviewed changes

Trott previously requested changes Jul 29, 2019

View reviewed changes

squash: do not use NodeWorker domain in a worker.

16eee7a

fixup

fdd7a24

This comment has been minimized.

Sign in to view

fixup

d804348

This comment has been minimized.

Sign in to view

Trott approved these changes Jul 30, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2019

Trott closed this Jul 31, 2019

eugeneo deleted the flat-workers branch July 31, 2019 18:27

BridgeAR mentioned this pull request Aug 6, 2019

v12.8.0 release proposal #29017

Merged

Uh oh!

Conversation

eugeneo commented Jul 27, 2019

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 29, 2019

Uh oh!

Trott commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

eugeneo commented Jul 29, 2019

Uh oh!

nodejs-github-bot commented Jul 29, 2019

Uh oh!

Trott commented Jul 29, 2019

Uh oh!

Trott commented Jul 29, 2019

Uh oh!

This comment has been minimized.

Trott commented Jul 29, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jul 30, 2019 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 30, 2019

Uh oh!

Trott commented Jul 30, 2019

Uh oh!

eugeneo commented Jul 30, 2019

Uh oh!

Trott commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Trott commented Jul 29, 2019 •

edited

Loading

nodejs-github-bot commented Jul 30, 2019 •

edited by Trott

Loading