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

inspector: report all workers #28872

Closed
wants to merge 5 commits into from

Conversation

@eugeneo
Copy link
Contributor

commented Jul 27, 2019

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
inspector: report all workers
Main thread (the one that WS endpoint connects to) should be able
to report all workers.
@eugeneo eugeneo referenced this pull request Jul 27, 2019
4 of 4 tasks complete
worker_agent_.reset(); // Dispose before the dispatchers
if (worker_agent_) {
worker_agent_->disable();
worker_agent_.reset(); // Dispose before the dispatchers

This comment has been minimized.

Copy link
@addaleax

addaleax Jul 27, 2019

Member

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?

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 28, 2019

Author Contributor

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

}

function runWorkerThread() {
let worker = null;

This comment has been minimized.

Copy link
@addaleax

addaleax Jul 27, 2019

Member

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

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 28, 2019

Author Contributor

Done


let rootWorker = null;

function runTest() {

This comment has been minimized.

Copy link
@addaleax

addaleax Jul 27, 2019

Member

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

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 28, 2019

Author Contributor

Done

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

This comment has been minimized.

Copy link
@addaleax

addaleax Jul 27, 2019

Member

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

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 28, 2019

Author Contributor

Done

@addaleax addaleax added the worker label Jul 27, 2019

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

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
Copy link
Member

left a comment

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

@eugeneo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

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

@Trott Trott dismissed their stale review Jul 29, 2019

test updated

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

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.

@nodejs-github-bot

This comment was marked as outdated.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Copy link

commented Jul 29, 2019

@nodejs-github-bot

This comment has been minimized.

Copy link

commented Jul 30, 2019

@Trott
Trott approved these changes Jul 30, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

CI passed!

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@addaleax @jasnell Still looks good to you?

@Trott Trott added the author ready label Jul 30, 2019

@eugeneo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@Trott - thank you for fixing the tests!

Trott added a commit to Trott/io.js that referenced this pull request Jul 31, 2019
inspector: report all workers
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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Landed in 7435dc8

@Trott Trott closed this Jul 31, 2019

@eugeneo eugeneo deleted the eugeneo:flat-workers branch Jul 31, 2019

targos added a commit that referenced this pull request Aug 2, 2019
inspector: report all workers
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>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
inspector: report all workers
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>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
inspector: report all workers
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.