Worker debug#21364
Worker debug #21364
Uh oh!
There was an error while loading. Please reload this page.
Conversation
eugeneo commented Jun 16, 2018
This pull request includes commit from #21182 |
eugeneo commented Aug 14, 2018
This PR is now ready for the review. @nodejs/diagnostics @nodejs/v8-inspector @nodejs/workers |
hybrist left a comment
hybrist left a comment There was a problem hiding this comment.
Not the person to review the C++ specifics of this but overall looks good!
eugeneo commented Aug 14, 2018
Out of scope for this PR, IMHO. |
pavelfeldman left a comment
pavelfeldman left a comment There was a problem hiding this comment.
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?
src/inspector/node_protocol.pdl Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can still see it here - https://paulirish.github.io/debugger-protocol-viewer/1-3/Target/
There was a problem hiding this comment.
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.
@dgozman had approved this protocol file when the implementation had started. |
pavelfeldman commented Aug 14, 2018
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. |
src/inspector/node_protocol.pdl Outdated
There was a problem hiding this comment.
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?
src/inspector/node_protocol.pdl Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds like a plan! I will implement the changes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good, I like "attachedToWorker/detachedFromWorker".
eugeneo added wip 
dgozman left a comment
dgozman left a comment There was a problem hiding this comment.
Looks great! Thank you for going through this.
src/inspector/node_protocol.pdl Outdated
There was a problem hiding this comment.
This description should probably talk about workers as well.
src/inspector/node_protocol.pdl Outdated
src/inspector/node_protocol.pdl Outdated
There was a problem hiding this comment.
Sounds good, I like "attachedToWorker/detachedFromWorker".
eugeneo commented Aug 21, 2018
@dgozman@pavelfeldman qq: should worker inspectors have the NodeTarget domains? |
dgozman commented Aug 21, 2018
@eugeneo If workers can spawn nested workers, it would be ideal to report them through the main NodeWorker domain if possible. |
jasnell commented Aug 21, 2018
jasnell added author ready 
src/inspector/worker_agent.cc Outdated
There was a problem hiding this comment.
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.
src/inspector/worker_agent.cc Outdated
There was a problem hiding this comment.
Is it intentional that this has 2 slashes, i.e. the host is being set to eval?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's what Chrome DevTools team suggested
Hm. I would simply pass url as is. Empty is empty, that should be fine.
src/inspector/worker_inspector.h Outdated
There was a problem hiding this comment.
Style nit: blank line before private: (I think the linter should complain about this?)
src/inspector_agent.cc Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It is used by a signal handler.
eugeneo commented Aug 25, 2018
@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 commented Sep 18, 2018
Landed as f28c6f7 |
targos commented Sep 19, 2018
Should this be backported to |
eugeneo commented Sep 19, 2018
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 commented Sep 19, 2018
@targos - sorry, forgot to tag you. I also did a CI run - seems ok - https://ci.nodejs.org/job/node-test-commit/21627/ |
targos commented Sep 19, 2018
@eugeneo Excellent! Would you like to open a backport PR? |
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]>
addaleax added the notable-change 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
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
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
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
9 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesThis is a proposed interface for debugging Node workers. It is largely similar to Chrome DevTools protocol with web-specific parts removed.