-
Notifications
You must be signed in to change notification settings - Fork 30k
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
[WIP] Node support for the inspector "Target" domain #16629
Conversation
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. |
* domain {string} - a JavaScript identifier string that serves as a unique | ||
identifier for the messages that target this domain. Domains should be unique. | ||
It is not permitted to override built-in Node.js domains. | ||
* constructor {ES6 class|constructor function} - creates a protocol handler that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder that both ES6 class
and constructor function
need to be added in type-parser.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw this isn’t part of the external API documentation
## Terminology | ||
|
||
* *Inspector session* - message exchange between protocol client and Node.js | ||
* *Protocol handler* - object with a liftime of the inspector session that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit: liftime -> lifetime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
formats for specific and related functionality. Node.js exposes protocol | ||
domains provided by the V8. These include `Runtime`, `Debugger`, `Profiler` and | ||
`HeapProfiler`. | ||
* *Message* - a JSON string passed between Node.js backend and a inspector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit: a inspector -> an inspector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* *Message* - a JSON string passed between Node.js backend and a inspector | ||
protocol client. Message types are: request, response and notification. Protocol | ||
client only sends out requests. Messages are asynchronous and client should not | ||
assume response will be sent immidiately after serving the request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit: immidiately -> immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@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 |
* domain {string} - a JavaScript identifier string that serves as a unique | ||
identifier for the messages that target this domain. Domains should be unique. | ||
It is not permitted to override built-in Node.js domains. | ||
* constructor {ES6 class|constructor function} - creates a protocol handler that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw this isn’t part of the external API documentation
const { registerDispatcherFactory } = process.binding('inspector'); | ||
|
||
const domainHandlerClasses = new Map(); | ||
const SessionTerminatedSymbol = Symbol('DisposeAgent'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the Symbol string match the variable name here, so it’s more obvious what the symbol refers to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
function createResponseCallback(session, id) { | ||
let first_call = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For JS we usually go with camelCase, i.e. firstCall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/inspector_io.cc
Outdated
bool WaitForFrontendMessageWhilePaused() override; | ||
void SendMessageToFrontend(const v8_inspector::StringView& message) override; | ||
bool IsConnected() { return !!session_; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool IsConnected() const { … }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/inspector_js_api.cc
Outdated
Local<Value> argument; | ||
if (message.length() > 0) { | ||
MaybeLocal<String> v8string = | ||
String::NewFromTwoByte(isolate, message.characters16(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is message.is8Bit()
always false? If it is, can you add a comment explaining why + a CHECK
for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, it just // Should never happen
- but I added a second branch just in case.
Ping @eugeneo |
|
||
const handler = this._getOrCreateHandler(domain); | ||
|
||
if (!handler || !util.isFunction(handler[method])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return; | ||
} | ||
const parsed = parseSuppressingError(message); | ||
const [ domain, method ] = parsed.method ? parsed.method.split('.') : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't allow parsed.method
with more than one dots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a simple Regex validation to exclude private methods and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check for that scenario.
Uploaded a rebased version and addressed the comments. |
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 |
@@ -267,6 +421,17 @@ void IsEnabled(const FunctionCallbackInfo<Value>& args) { | |||
args.GetReturnValue().Set(env->inspector_agent()->enabled()); | |||
} | |||
|
|||
void RegisterDispatcherFactory(const FunctionCallbackInfo<Value>& args) { | |||
Environment* env = Environment::GetCurrent(args); | |||
if (args.Length() == 1 || (args[0]->IsNull() || args[0]->IsFunction())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)?
InspectorSessionDelegate* const delegate_; | ||
std::unique_ptr<v8_inspector::V8InspectorSession> session_; | ||
std::unique_ptr<MessageDispatcher> dispatcher_; | ||
MessageDispatcherFactory* dispatcher_factory_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using a shared_ptr
for a lot of these dumb pointers?
const [ domain, method, tail ] = | ||
parsed.method ? parsed.method.split('.') : []; | ||
|
||
if (!domain || !method || !method.match(/^[a-zA-Z]\w*$/) || tail) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still doesn't work with 'Domain.method.'
, since tail would evaluate to falsey.
@@ -213,7 +367,7 @@ void InspectorConsoleCall(const FunctionCallbackInfo<Value>& info) { | |||
call_args.data()).FromMaybe(Local<Value>()); | |||
} | |||
|
|||
static void* GetAsyncTask(int64_t asyncId) { | |||
void* GetAsyncTask(int64_t asyncId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the static
removed?
@@ -32,6 +32,171 @@ std::unique_ptr<StringBuffer> ToProtocolString(Isolate* isolate, | |||
return StringBuffer::create(StringView(*buffer, buffer.length())); | |||
} | |||
|
|||
class JSDispatcher; | |||
|
|||
class JSDispatcherInterface : private AsyncWrap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tmpl->InstanceTemplate()->SetInternalFieldCount(1); | ||
tmpl->SetClassName( | ||
FIXED_ONE_BYTE_STRING(env_->isolate(), | ||
"InspectorDispatcherConnection")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating such a template every single time, why not cache it in the Environment?
dispatcher_(nullptr) { | ||
Wrap(object, this); | ||
env->SetMethod(object, "sendMessageToFrontend", | ||
JSDispatcherInterface::SendMessageToFrontend); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should be set in the FunctionTemplate rather than on every invocation.
@@ -67,13 +101,13 @@ class Agent { | |||
v8::Local<v8::Function> enable_function, | |||
v8::Local<v8::Function> disable_function); | |||
|
|||
void RegisterDispatcherFactory( | |||
std::unique_ptr<MessageDispatcherFactory> factory); | |||
|
|||
// These methods are called by the WS protocol and JS binding to create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is called
class InspectorIo; | ||
class NodeInspectorClient; | ||
|
||
class InspectorSession { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bydelete
ing 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.
virtual bool HandleMessage(const v8_inspector::StringView& message) = 0; | ||
}; | ||
|
||
class MessageDispatcherFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to my recommendation below for JSDispatcher
, this interface could be nested into the MessageDispatcher
interface.
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. |
My vote is No for now. This API is too dangerous in general, and can crash |
Ping @eugeneo |
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. |
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. |
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. |
@TimothyGu @eugeneo is it planned to work on this again? |
@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. |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected 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
andspawn
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.