[WIP] Node support for the inspector "Target" domain by eugeneo · Pull Request #16629 · nodejs/node

eugeneo · 2017-10-30T23:58:08Z

Checklist

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

Affected core subsystem(s)

inspector: new (internal) API was added.

This is a proposed implementation of the inspector protocol extensibility for the Node. Second commit implements a "Target" domain handler. Note that right now targets created through fork and spawn are not automatically surfacing here. I will publish a Node module that demos the protocol in a separate repository.

In the future, we may look into implementing "Network" domain to surface network interactions.

I am looking for any feedback.

Should this be a public API? I can see tooling vendors or Node module providers looking to implement custom domains in the future.

eugeneo · 2017-10-31T00:06:00Z

This is Node module that can be used to test this functionality: https://github.com/eugeneo/node-targets-discovery

At this time, public version of the Chrome DevTools does not support this protocol for Node. I will work on adding it and it should be in Chrome Canary soon.

vsemozhetbyt · 2017-10-31T10:28:16Z

~~Just a reminder that both ES6 class and constructor function need to be added in type-parser.js~~

fwiw this isn’t part of the external API documentation

vsemozhetbyt · 2017-10-31T10:35:51Z

A nit: liftime -> lifetime.

vsemozhetbyt · 2017-10-31T10:36:49Z

A nit: a inspector -> an inspector.

vsemozhetbyt · 2017-10-31T10:37:50Z

A nit: immidiately -> immediately.

auchenberg · 2017-10-31T18:25:19Z

@eugeneo Would be great with an overview of the implemented Target API commands and events, so tooling vendors can validate the compatibility with CDP 1.2

addaleax · 2017-10-31T18:34:24Z

fwiw this isn’t part of the external API documentation

addaleax · 2017-10-31T18:39:45Z

Can the Symbol string match the variable name here, so it’s more obvious what the symbol refers to?

addaleax · 2017-10-31T18:40:32Z

For JS we usually go with camelCase, i.e. firstCall

addaleax · 2017-10-31T18:48:27Z

bool IsConnected() const { … }?

addaleax · 2017-10-31T18:52:03Z

Why is message.is8Bit() always false? If it is, can you add a comment explaining why + a CHECK for that?

At this point, it just // Should never happen - but I added a second branch just in case.

BridgeAR · 2017-11-23T21:57:52Z

Ping @eugeneo

TimothyGu · 2017-11-24T00:56:35Z

I assume we don't want the client to be able to access private/underscore-prefixed methods, so there should be some kind of opt-in mechanism. Maybe requiring handler.supportedMethods to be a Set of supported methods would be a good idea?

I am planning to implement validation against protocol JSON (see Chrome DevTools). This validation will add a lot of non-trivial code and I want to make this code review focused on basic functionality.

TimothyGu · 2017-11-24T00:58:57Z

We shouldn't allow parsed.method with more than one dots.

Added a simple Regex validation to exclude private methods and such.

The regex still allows parsed.method with more than two dots, since method as an element returned from .split() cannot contain a dot. You would have to check parsed.method.split('.') directly.

I added a check for that scenario.

This change introduces internal API for extending inspector protocol through JS API. Ref: #16627

'Target' domain handler implementation is extensible, enabling the user code to report targets and implement the transport. Ref: #16627

eugeneo · 2017-11-28T18:57:18Z

Uploaded a rebased version and addressed the comments.

eugeneo · 2017-11-28T19:04:40Z

@auchenberg

Would be great with an overview of the implemented Target API commands and events, so tooling vendors can validate the compatibility with CDP 1.2

Plan is to eventually expose protocol JSON (extracted from Chrome DevTools protocol.json) from the inspector HTTP endpoint.

auchenberg · 2017-11-28T20:01:20Z

@eugeneo

Plan is to eventually expose protocol JSON (extracted from Chrome DevTools protocol.json) from the inspector HTTP endpoint.

Isn't this already supported by HTTP endpoint as /json/protocol? The served protocol.json should for sure be updated to reflect the new Target domains...

TimothyGu · 2017-11-28T19:25:42Z


+void RegisterDispatcherFactory(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  if (args.Length() == 1 || (args[0]->IsNull() || args[0]->IsFunction())) {


Shouldn't this only be args[0]->IsFunction() (args.Length() check is included in that statement, since if args.Length() == 0 args[0] would be undefined, and I don't think we want to allow null)?

TimothyGu · 2017-11-28T19:36:06Z

+  InspectorSessionDelegate* const delegate_;
+  std::unique_ptr<v8_inspector::V8InspectorSession> session_;
+  std::unique_ptr<MessageDispatcher> dispatcher_;
+  MessageDispatcherFactory* dispatcher_factory_;


What about using a shared_ptr for a lot of these dumb pointers?

TimothyGu · 2017-11-28T19:43:58Z

+    const [ domain, method, tail ] =
+        parsed.method ? parsed.method.split('.') : [];
+
+    if (!domain || !method || !method.match(/^[a-zA-Z]\w*$/) || tail)


Still doesn't work with 'Domain.method.', since tail would evaluate to falsey.

TimothyGu · 2017-11-28T19:47:44Z

 }

-static void* GetAsyncTask(int64_t asyncId) {
+void* GetAsyncTask(int64_t asyncId) {


Why is the static removed?

TimothyGu · 2017-11-28T20:13:51Z


+class JSDispatcher;
+
+class JSDispatcherInterface : private AsyncWrap {


Instead of having three top-level classes, can you reorganize it so that it's clearer to the reader that these three concepts in fact form one cohesive module? Maybe something like

class JSDispatcher : public MessageDispatcher { public: class Factory : public MessageDispatcherFactory {}; private: class Interface : private AsyncWrap {}; JSDispatcher(std::unique_ptr<DispatcherSession>&& session, Interface* interface) {} };

and then later

new JSDispatcher::Factory()

In fact if you do so you might be able to get rid of a few accessors too.

TimothyGu · 2017-11-28T20:26:04Z

+    tmpl->InstanceTemplate()->SetInternalFieldCount(1);
+    tmpl->SetClassName(
+        FIXED_ONE_BYTE_STRING(env_->isolate(),
+                              "InspectorDispatcherConnection"));


Instead of creating such a template every single time, why not cache it in the Environment?

TimothyGu · 2017-11-28T20:27:23Z

+                        dispatcher_(nullptr) {
+    Wrap(object, this);
+    env->SetMethod(object, "sendMessageToFrontend",
+                   JSDispatcherInterface::SendMessageToFrontend);


This method should be set in the FunctionTemplate rather than on every invocation.

TimothyGu · 2017-11-28T20:29:11Z

+  void RegisterDispatcherFactory(
+      std::unique_ptr<MessageDispatcherFactory> factory);
+
  // These methods are called by the WS protocol and JS binding to create


This method is called

TimothyGu · 2017-11-28T20:41:26Z

+class InspectorIo;
+class NodeInspectorClient;
+
+class InspectorSession {


Please add some documentation to make this more maintainable. Namely, I'd like to see

What this class is for? Especially how it differs from the similarly named DispatcherSession.

Who creates this class?

Who will own this class?

Who will hold references to this class, other than its owner?

As I understand this class

is for the session delegate (WebSocket or JS binding) to control a connection to the inspector Agent, both for sending messages through Dispatch() and to disconnect from the inspector Agent by deleteing an object of this class,

is created when the Agent connects to a session delegate

is owned by either the session delegate itself or the owner of the session delegate

other classes do not have references to it

if this is correct then please articulate it. And please do so for all the newly created classes in this PR.

TimothyGu · 2017-11-28T20:44:33Z

+  virtual bool HandleMessage(const v8_inspector::StringView& message) = 0;
+};
+
+class MessageDispatcherFactory {


Similarly to my recommendation below for JSDispatcher, this interface could be nested into the MessageDispatcher interface.

eugeneo · 2017-11-28T20:51:14Z

@auchenberg

The served protocol.json should for sure be updated to reflect the new Target domains...

The goal is not to special case Target domain but to provide an interface to write new domains. So the HTTP server will need to be updated to be able to build the JSON at runtime, based on information provided by custom domains.

TimothyGu · 2017-11-28T20:54:59Z

Should this be a public API?

My vote is No for now. This API is too dangerous in general, and can crash node very easily if misused.

BridgeAR · 2018-01-31T02:03:42Z

Ping @eugeneo

eugeneo · 2018-01-31T16:39:13Z

This is still something I really want to land. I am currently focused on my main project, but I am hoping to work on inspector-over-pipe (which is a prerequisite for this functionality) in Q2.

eugeneo · 2018-02-26T22:22:41Z

During our testing we realized that this approach is not a good fit for the Node platform as it relies on parent target outliving the child target. We are working on an alternative implementation for the child process debugging that will not have this limitation.

Still, ability to add custom "domains" definitely still has value for the ecosystem in our opinion (though now it loses major use-case). Please let me know if there is interest in adding that support to the Node platform without adding the "Target" domain.

TimothyGu · 2018-02-27T00:46:38Z

@eugeneo

Please let me know if there is interest in adding that support to the Node platform without adding the "Target" domain.

Still, ability to add custom "domains" definitely still has value for the ecosystem in our opinion (though now it loses major use-case). There definitely is. See nodejs/diagnostics#75. I'll reopen this for now.

BridgeAR · 2018-04-10T00:51:16Z

@TimothyGu @eugeneo is it planned to work on this again?

eugeneo · 2018-04-13T16:56:29Z

@BridgeAR as I mentioned above, this is currently not the approach we want to pursue wrt multiprocess debugging. Still, the code in this branch solves another important issue by providing a way to extend the Inspector protocol. Basically, this PR is here in case there is a need for that.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Oct 30, 2017

eugeneo self-assigned this Oct 31, 2017

eugeneo added child_process Issues and PRs related to the child_process subsystem. inspector Issues and PRs related to the V8 inspector protocol. labels Oct 31, 2017

eugeneo mentioned this pull request Oct 31, 2017

inspector: implement sub process inspection. #16627

Closed

4 tasks

vsemozhetbyt reviewed Oct 31, 2017

View reviewed changes

addaleax reviewed Oct 31, 2017

View reviewed changes

auchenberg mentioned this pull request Nov 10, 2017

Node.js Cluster Example Fails under VS Code microsoft/vscode#3201

Closed

TimothyGu reviewed Nov 24, 2017

View reviewed changes

Eugene Ostroukhov added 4 commits November 28, 2017 09:58

inspector: add support for protocol extension

62193ba

This change introduces internal API for extending inspector protocol through JS API. Ref: #16627

inspector: add 'Target' domain handler

d5124bd

'Target' domain handler implementation is extensible, enabling the user code to report targets and implement the transport. Ref: #16627

inspector: No built-in domain override

962c616

WIP - addressing code review comments, to be squashed.

926a5c2

(To be squashed) Reject malformed method name

3901aa5

TimothyGu suggested changes Nov 28, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Jan 31, 2018

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Feb 7, 2018

eugeneo closed this Feb 26, 2018

TimothyGu reopened this Feb 27, 2018

eugeneo closed this Apr 26, 2018

eugeneo deleted the custom_dispatcher branch April 26, 2018 17:34

TimothyGu assigned eugeneo and TimothyGu and unassigned eugeneo Apr 26, 2018


		class JSDispatcher;

		class JSDispatcherInterface : private AsyncWrap {

Conversation

eugeneo commented Oct 30, 2017

Checklist

Affected core subsystem(s)

Uh oh!

eugeneo commented Oct 31, 2017

Uh oh!

vsemozhetbyt Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

auchenberg commented Oct 31, 2017

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!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Nov 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugeneo Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

eugeneo commented Nov 28, 2017

Uh oh!

eugeneo commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auchenberg commented Nov 28, 2017

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!

vsemozhetbyt Oct 31, 2017 •

edited

Loading

eugeneo Nov 28, 2017 •

edited

Loading

eugeneo commented Nov 28, 2017 •

edited

Loading

TimothyGu commented Feb 27, 2018 •

edited

Loading