Worker debug #21364

eugeneo · 2018-06-16T00:32:10Z

Checklist

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

This is a proposed interface for debugging Node workers. It is largely similar to Chrome DevTools protocol with web-specific parts removed.

eugeneo · 2018-06-16T00:33:00Z

This pull request includes commit from #21182

eugeneo · 2018-08-14T17:24:49Z

This PR is now ready for the review.

@nodejs/diagnostics @nodejs/v8-inspector @nodejs/workers

alexkozy

I am wondering: should we instrument worker postMessage and onmessage with V8Inspector instrumentation for step-into worker and async stacks cross worker boundary?

alexkozy · 2018-08-14T17:26:29Z

src/inspector/node_protocol_config.json

-        "namespace": ["node", "inspector", "protocol"],
-        "options": [
-            {
-                "domain": "NodeTracing"


Why this one is removed?

jkrems

Not the person to review the C++ specifics of this but overall looks good!

eugeneo · 2018-08-14T18:09:59Z

I am wondering: should we instrument worker postMessage and onmessage with V8Inspector instrumentation for step-into worker and async stacks cross worker boundary?

Out of scope for this PR, IMHO.

pavelfeldman

We've recently introduced flattened protocol operation for the Target domain for the sake of efficiency and I wonder if we should commit the new stuff into Node right away instead of landing the deprecated one... @dgozman, wdyt?

pavelfeldman · 2018-08-14T18:38:41Z

src/inspector/node_protocol.pdl

+      string url
+
+  # Sends protocol message over session with given id.
+  command sendMessageToTarget


This does not exist in the new flattened world, sessionId attribute is passed as a top-level parameter instead. This allows us to not JSON-serialize and wrap all the commands that go into the nested targets such as workers.

I can still see it here - https://paulirish.github.io/debugger-protocol-viewer/1-3/Target/

We have not migrated all the use cases to flattened yet, it also is a public API method, so deprecation would beed to be graceful. Basically, it'll soon become deprecated and a year later it'll become noop.

eugeneo · 2018-08-14T18:38:59Z

@dgozman had approved this protocol file when the implementation had started.

pavelfeldman · 2018-08-14T18:41:32Z

Well, since then we've introduced the flattened operation... Furthermore, we were considering moving the workers off the Target domain discovery, but that is still up in the air. Anyhow, let's see what he says.

pavelfeldman · 2018-08-14T18:54:10Z

src/inspector/node_protocol.pdl

+      boolean autoAttach
+      # Whether to pause new targets when attaching to them. Use `Runtime.runIfWaitingForDebugger`
+      # to run paused targets.
+      boolean waitForDebuggerOnStart


As I see it, we have two ways of proceeding with the worker inspection:

Consider NodeTarget to be a duck-typed Target domain

This implies that flattened operation is in place. Flatten should not be hard for you to add, simply copy protocol from here: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/inspector/browser_protocol.pdl?q=browser_protocol&sq=package:chromium&g=0&l=5950 and add corresponding sniffing logic. When we move the front-end to be flatten by default, it will 'just work'.

Name things with what they are and rename this domain to NodeWorker.

Basically rename all the 'target' mentions here with the corresponding 'worker' mentions. Then you don't need to add flattened operation, Target domain is special and it adds the sessionId aspect into the protocol, other domains, including NodeWorker would not be special, so they would still need to wrap the messages. That's fine. We would still be lying to ourselves because we would be creating a target on the front-end for each worker, but I think that is tolerable.

@dgozman: wdyt?

dgozman · 2018-08-17T20:36:23Z

src/inspector/node_protocol.pdl

@@ -37,3 +37,59 @@ experimental domain NodeTracing
  # Signals that tracing is stopped and there is no trace buffers pending flush, all data were
  # delivered via dataCollected events.
  event tracingComplete
+
+# Supports additional targets discovery and allows to attach to them.
+experimental domain NodeTarget


After some discussion, we think that we'll not be able to keep NodeTarget domain close to Chromium's Target domain on the frontend side in the long term. Therefore, it would be easier to keep this domain very node-worker-specific to reduce the complexity and maintenance for the Node community.

My proposal would be to just rename things, keeping semantics the same:

NodeTarget -> NodeWorker

TargetID -> WorkerID

TargetInfo -> WorkerInfo

sendMessageToTarget -> sendMessageToWorker

setAutoAttach(true, waitForDebuggerOnStart) -> enable(waitForDebuggerOnStart)

setAutoAttach(false) -> disable()

attachedToTarget -> workerCreated

detachedFromTarget -> workerDestroyed

receivedMessageFromTarget -> receivedMessageFromWorker

What do you think? Other than that, things look good to me.

Sounds like a plan! I will implement the changes.

Did that. One thing I kinda feel strongly is that worker(Created|Destroyed) would be a misnomer, so I renamed it to attachedToWorker/detachedFromWorker. I would also consider naming them simply "attached" or "detached" as they are already have NodeWorker prefix.

Sounds good, I like "attachedToWorker/detachedFromWorker".

dgozman

Looks great! Thank you for going through this.

dgozman · 2018-08-20T18:10:37Z

src/inspector/node_protocol.pdl

@@ -37,3 +37,58 @@ experimental domain NodeTracing
  # Signals that tracing is stopped and there is no trace buffers pending flush, all data were
  # delivered via dataCollected events.
  event tracingComplete
+
+# Supports additional targets discovery and allows to attach to them.


This description should probably talk about workers as well.

dgozman · 2018-08-20T18:11:03Z

src/inspector/node_protocol.pdl

+  command enable
+    parameters
+      # Whether to pause new targets when attaching to them. Use `Runtime.runIfWaitingForDebugger`
+      # to run paused targets.


targets -> workers?

dgozman · 2018-08-20T18:12:52Z

src/inspector/node_protocol.pdl

@@ -37,3 +37,59 @@ experimental domain NodeTracing
  # Signals that tracing is stopped and there is no trace buffers pending flush, all data were
  # delivered via dataCollected events.
  event tracingComplete
+
+# Supports additional targets discovery and allows to attach to them.
+experimental domain NodeTarget


Sounds good, I like "attachedToWorker/detachedFromWorker".

eugeneo · 2018-08-21T02:16:02Z

@dgozman @pavelfeldman qq: should worker inspectors have the NodeTarget domains?

dgozman · 2018-08-21T17:03:29Z

@eugeneo If workers can spawn nested workers, it would be ideal to report them through the main NodeWorker domain if possible.

jasnell · 2018-08-21T18:24:37Z

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

addaleax · 2018-08-21T21:12:16Z

src/inspector/worker_agent.cc

+  void WorkerCreated(const std::string& title,
+                 const std::string& url,
+                 bool waiting,
+                 std::shared_ptr<MainThreadHandle> target);


Style nit: Here and above, the parameters aren’t aligned (and we generally only indent the initializers starting with : by 2 spaces.)

Side note: Running make format-cpp (preceded by make format-cpp-build, if necessary) would make sure you don’t have to worry about style nits anymore.

addaleax · 2018-08-21T21:13:25Z

src/inspector/worker_agent.cc

+      .setWorkerId(id)
+      .setTitle(title)
+      // Placeholder URL still needed for some frontends to resolve paths
+      .setUrl(url.empty() ? "file://eval" : url)


Is it intentional that this has 2 slashes, i.e. the host is being set to eval?

That's what Chrome DevTools team suggested, but they do not have equivalent feature in Chrome. It might be ok to set it to another value.

That's what Chrome DevTools team suggested

Hm. I would simply pass url as is. Empty is empty, that should be fine.

Empty URL it is.

addaleax · 2018-08-21T21:20:52Z

src/inspector/worker_inspector.h

+  bool WaitForConnect() {
+    return wait_;
+  }
+ private:


Style nit: blank line before private: (I think the linter should complain about this?)

addaleax · 2018-08-21T21:21:26Z

src/inspector_agent.cc

+  if (start_io_thread_async.data == this) {
+    start_io_thread_async.data = nullptr;
+    // This is global, will never get freed
+    uv_close(reinterpret_cast<uv_handle_t*>(&start_io_thread_async), nullptr);


This is interesting – why is start_io_thread_async global? That doesn’t seem quite right (or thread-safe, for that matter). These things should probably per-Environment or per-Isolate?

It is used by a signal handler.

eugeneo · 2018-08-25T00:49:27Z

@addaleax Thank you for the review. I addressed the comments and rebased the patch to make sure it works on top of current head.

eugeneo · 2018-09-18T16:10:47Z

Landed as f28c6f7

targos · 2018-09-19T06:57:35Z

Should this be backported to v10.x-staging? The change lands cleanly but the test fails:

$ out/Release/node --experimental-worker /home/mzasso/git/nodejs/v10.x/test/parallel/test-worker-debug.js
Test basic debug scenario
/home/mzasso/git/nodejs/v10.x/test/common/index.js:662
const crashOnUnhandledRejection = (err) => { throw err; };
                                             ^

AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
+ expected - actual

- 0
+ 2
    at WorkerSession.waitForBreakAfterCommand (/home/mzasso/git/nodejs/v10.x/test/parallel/test-worker-debug.js:111:12)

eugeneo · 2018-09-19T16:57:32Z

I definitely see a lot of value in introducing this in the same Node version the workers are introduced. This difference should be due to some V8 change - I adjusted the test case and the test works - https://github.com/eugeneo/node/tree/v10-workers-debug

eugeneo · 2018-09-19T18:07:03Z

@targos - sorry, forgot to tag you. I also did a CI run - seems ok - https://ci.nodejs.org/job/node-test-commit/21627/

targos · 2018-09-19T18:18:57Z

@eugeneo Excellent! Would you like to open a backport PR?
It looks like you could just create one from that branch and target v10.x-staging. Feel free to ping me on it and link the CI run.

Introduce a NodeTarget inspector domain modelled after ChromeDevTools Target domain. It notifies inspector frontend attached to a main V8 isolate when workers are starting and allows passing messages to inspectors on their isolates. All inspector functionality is enabled on worker isolates. Backport-PR-URL: #22954 PR-URL: #21364 Reviewed-By: Aleksei Koziatinskii <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect protocol support to allow use of WebSockets over HTTP/2. #23284 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * url * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * util * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * Added support for `BigInt` numbers in `util.format()`. #22097 * V8 API * A number of V8 C++ APIs have been marked as deprecated since they have been removed in the upstream repository. Replacement APIs are added where necessary. #23159 * Windows * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * Workers * Debugging support for Workers using the DevTools protocol has been implemented. #21364 * The public `inspector` module is now enabled in Workers. #22769 * Added new collaborators: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

mscdex added the wip Issues and PRs that are still a work in progress. label Jun 16, 2018

eugeneo mentioned this pull request Aug 10, 2018

inspector: tie objects lifetime to the thread they belong to #22242

Merged

2 tasks

eugeneo changed the title ~~WIP: Worker debug~~ Worker debug Aug 14, 2018

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 worker Issues and PRs related to Worker support. and removed wip Issues and PRs that are still a work in progress. labels Aug 14, 2018

alexkozy approved these changes Aug 14, 2018

View reviewed changes

jkrems approved these changes Aug 14, 2018

View reviewed changes

pavelfeldman suggested changes Aug 14, 2018

View reviewed changes

pavelfeldman reviewed Aug 14, 2018

View reviewed changes

dgozman reviewed Aug 17, 2018

View reviewed changes

eugeneo added wip Issues and PRs that are still a work in progress. and removed wip Issues and PRs that are still a work in progress. labels Aug 17, 2018

dgozman reviewed Aug 20, 2018

View reviewed changes

pavelfeldman approved these changes Aug 20, 2018

View reviewed changes

jasnell requested a review from addaleax August 21, 2018 18:22

jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 21, 2018

addaleax reviewed Aug 21, 2018

View reviewed changes

eugeneo deleted the worker-debug branch September 18, 2018 16:10

targos added the backport-requested-v10.x label Sep 19, 2018

eugeneo mentioned this pull request Sep 19, 2018

[v10.x backport] inspector: workers debugging #22954

Closed

2 tasks

targos added backported-to-v10.x and removed backport-requested-v10.x labels Oct 7, 2018

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 7, 2018

gauravmahto mentioned this pull request Jul 15, 2019

Debug worker_threads #26609

Open

Worker debug #21364

Worker debug #21364

Conversation

eugeneo commented Jun 16, 2018

Checklist

eugeneo commented Jun 16, 2018

eugeneo commented Aug 14, 2018

alexkozy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems left a comment

Choose a reason for hiding this comment

eugeneo commented Aug 14, 2018

pavelfeldman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Aug 14, 2018 • edited Loading

pavelfeldman commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgozman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Aug 21, 2018

dgozman commented Aug 21, 2018

jasnell commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Aug 25, 2018

eugeneo commented Sep 18, 2018

targos commented Sep 19, 2018

eugeneo commented Sep 19, 2018

eugeneo commented Sep 19, 2018

targos commented Sep 19, 2018

alexkozy left a comment •

edited

Loading

eugeneo commented Aug 14, 2018 •

edited

Loading