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: JavaScript bindings for the inspector #12263

Closed
wants to merge 1 commit into
base: master
from

Conversation

@eugeneo
Contributor

eugeneo commented Apr 6, 2017

Adds bindings for accessing Inspector backend from the JavaScript code
on the main context. Following methods were added to process.inspector:

  • connect(callback) - begins a new inspector session. Callback argument
    will be notified when the message is received.
  • disconnect() - destroys the session, disables agents and frees the memory
  • dispatch(message) - sends a message to inspector dispatcher.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

CC: @ofrobots

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Apr 6, 2017

Contributor

The intent of this API is not to create a comprehensive "debugger" API or "profiler" API - instead, the goal is to provide a generic way to talk to inspector so such APIs could be designed for solving specific issues.

Contributor

eugeneo commented Apr 6, 2017

The intent of this API is not to create a comprehensive "debugger" API or "profiler" API - instead, the goal is to provide a generic way to talk to inspector so such APIs could be designed for solving specific issues.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Apr 6, 2017

Contributor

PPC failure needs to be investigated, I am looking into it.

Contributor

eugeneo commented Apr 6, 2017

PPC failure needs to be investigated, I am looking into it.

@mscdex mscdex removed the test label Apr 6, 2017

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 6, 2017

Member

I don't really get what this is trying to accomplish. I mean, I see what it does but not why. What is the longer-term goal here? A public debugger API?

Member

bnoordhuis commented Apr 6, 2017

I don't really get what this is trying to accomplish. I mean, I see what it does but not why. What is the longer-term goal here? A public debugger API?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 6, 2017

Contributor

The debug context is being removed in V8 by EOY. The objective is to provide an alternative mechanism to do in-process debug tooling.

  1. Node core is dependent on the debug context for some internal functionality (example). It would be good to plumb them over the inspector now.
  2. The ecosystem depends on the debug context for in-process debug tooling (example userspace code dependent on this, another example).
Contributor

ofrobots commented Apr 6, 2017

The debug context is being removed in V8 by EOY. The objective is to provide an alternative mechanism to do in-process debug tooling.

  1. Node core is dependent on the debug context for some internal functionality (example). It would be good to plumb them over the inspector now.
  2. The ecosystem depends on the debug context for in-process debug tooling (example userspace code dependent on this, another example).
@eugeneo

This comment has been minimized.

Show comment
Hide comment
Contributor

eugeneo commented Apr 6, 2017

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Apr 7, 2017

Member
  1. Node core is dependent on the debug context for some internal functionality. It would be good to plumb them over the inspector now.

An even better solution is to add C++ bindings for the cases that need them. What has already happened for proxies and promises should also be done to MapIterator and SetIterator, and GeneratorObject too as a bonus.

I don't oppose the second goal of this PR, but it's simply impractical to go through the overhead of the inspector, when all I want to know is the iterated [[Map]] of an iterator object.

Member

TimothyGu commented Apr 7, 2017

  1. Node core is dependent on the debug context for some internal functionality. It would be good to plumb them over the inspector now.

An even better solution is to add C++ bindings for the cases that need them. What has already happened for proxies and promises should also be done to MapIterator and SetIterator, and GeneratorObject too as a bonus.

I don't oppose the second goal of this PR, but it's simply impractical to go through the overhead of the inspector, when all I want to know is the iterated [[Map]] of an iterator object.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 7, 2017

Member

Node core is dependent on the debug context for some internal functionality (example). It would be good to plumb them over the inspector now.

I agree with @TimothyGu that using the inspector is complete overkill for that.

The ecosystem depends on the debug context for in-process debug tooling (example userspace code dependent on this, another example).

strong-agent's use is not important; I wrote that code, I can rewrite it. I imagine the same is true for GCP.

Member

bnoordhuis commented Apr 7, 2017

Node core is dependent on the debug context for some internal functionality (example). It would be good to plumb them over the inspector now.

I agree with @TimothyGu that using the inspector is complete overkill for that.

The ecosystem depends on the debug context for in-process debug tooling (example userspace code dependent on this, another example).

strong-agent's use is not important; I wrote that code, I can rewrite it. I imagine the same is true for GCP.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 7, 2017

Contributor

I'm more concerned about the ecosystem usage. The linked GCP module is an in-process debugger for in-production use (outbound connections only). It needs to set breakpoints, gather stacks, evaluate expressions on specific frames. Today, it is pure JavaScript, building on top of the vm.runInDebugContext api. Once that API goes away, we'll need an alternative and having to go to a C++ native module would be reduce the number of people who can use this module.

I haven't done a very deep analysis of the ecosystem, but there are quite a few other modules that also depend on vm.runInDebugContext and they will be forced to C++ native modules unless core offers another way to offer similar functionality in JS.

I think offering JS bindings to the inspector opens up the possibility for the ecosystem to write really cool debug and profiling tooling. Just throwing out ideas: profilers / heapdumps that don't have dependencies on native code or requiring core to add apis for heapdumps; debug functionality integrated into test-frameworks like mocha etc.

Contributor

ofrobots commented Apr 7, 2017

I'm more concerned about the ecosystem usage. The linked GCP module is an in-process debugger for in-production use (outbound connections only). It needs to set breakpoints, gather stacks, evaluate expressions on specific frames. Today, it is pure JavaScript, building on top of the vm.runInDebugContext api. Once that API goes away, we'll need an alternative and having to go to a C++ native module would be reduce the number of people who can use this module.

I haven't done a very deep analysis of the ecosystem, but there are quite a few other modules that also depend on vm.runInDebugContext and they will be forced to C++ native modules unless core offers another way to offer similar functionality in JS.

I think offering JS bindings to the inspector opens up the possibility for the ecosystem to write really cool debug and profiling tooling. Just throwing out ideas: profilers / heapdumps that don't have dependencies on native code or requiring core to add apis for heapdumps; debug functionality integrated into test-frameworks like mocha etc.

@TimothyGu

A review of the code itself, assuming this PR is to go forward.

Show outdated Hide outdated src/inspector_agent.cc
if (len > buffer.size() - 1) {
len = buffer.size() - 1;
}
buffer[len] = '\0';

This comment has been minimized.

@TimothyGu

This comment has been minimized.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Thanks! Did that, made code simpler.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Thanks! Did that, made code simpler.

Show outdated Hide outdated src/inspector_agent.cc
Local<v8::Private> DelegatePrivateKey(Isolate* isolate) {
return v8::Private::ForApi(
isolate, node::OneByteString(isolate, "Inspector#Delegate"));

This comment has been minimized.

@TimothyGu

TimothyGu Apr 7, 2017

Member

Can you use the existing private symbol infrastructure?

@TimothyGu

TimothyGu Apr 7, 2017

Member

Can you use the existing private symbol infrastructure?

This comment has been minimized.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done. Not sure if it should be surrounded with #if to only be enabled when building with the inspector...

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done. Not sure if it should be surrounded with #if to only be enabled when building with the inspector...

Show outdated Hide outdated src/inspector_agent.cc
void Dispatch(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
if (info.Length() != 1 || !info[0]->IsString()) {

This comment has been minimized.

@TimothyGu

TimothyGu Apr 7, 2017

Member

The Length check isn't necessary, since if one calls the function without arguments info[0] will be Undefined.

@TimothyGu

TimothyGu Apr 7, 2017

Member

The Length check isn't necessary, since if one calls the function without arguments info[0] will be Undefined.

This comment has been minimized.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done.

Show outdated Hide outdated src/inspector_agent.cc
return;
}
Agent* inspector = GetInspectorChecked(env);
if (!inspector->delegate()

This comment has been minimized.

@TimothyGu

TimothyGu Apr 7, 2017

Member

inspector can be nullptr here. I'd even return a Maybe<Agent*> from GetInspectorChecked() and maybe GetDelegate() to ensure nullptr is always checked for.

@TimothyGu

TimothyGu Apr 7, 2017

Member

inspector can be nullptr here. I'd even return a Maybe<Agent*> from GetInspectorChecked() and maybe GetDelegate() to ensure nullptr is always checked for.

This comment has been minimized.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done, though I am not entirely sold on using Maybe for pointers...

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done, though I am not entirely sold on using Maybe for pointers...

Show outdated Hide outdated src/inspector_agent.cc
void CreateJSBindingsSession(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
if (info.Length() != 1 || !info[0]->IsFunction()) {

This comment has been minimized.

@TimothyGu

TimothyGu Apr 7, 2017

Member

Ditto about Length() check.

@TimothyGu

TimothyGu Apr 7, 2017

Member

Ditto about Length() check.

This comment has been minimized.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 7, 2017

Contributor

Done.

@sam-github

Generally I'm in favour of introspectability of the runtime. This PR introduces new js APIs with no documentation, so it is WIP, but in the absence of docs, its hard to comment on it as a feature unless you already are familiar with the v8 c++ inspector session API.

@TimothyGu

Echoing @sam-github's concerns here as well. I'm fine with committing the code right now (other than two nits below) as a first draft, but it has to stay behind a flag or disabled by default in some other way, since we would want to be a bit more careful when making new APIs.

Other than the fact that docs are missing, I wonder if these added APIs are as polished as we could make it. To this end, it might be necessary to make an EP for this feature. A few questionable things that came to my mind:

  1. that we are putting everything into process.inspector; maybe we should at one point have a separate require('inspector') (or some other module name) for these user-facing interfaces
  2. that we are creating a new session object from scratch every time connect() is called; making an inspector session a class seems appropriate for this case (const session = new inspector.Session((notification) => { ... }); session.disconnect();)
  3. that only one inspector can be attached at one time; it doesn't take much imagination to think of an instance where the user wishes to debug an application that uses the JS-land inspector API; or am I missing something?
Show outdated Hide outdated test/inspector/test-bindings.js
scopeCallback = () => (breakpointHit = true);
debuggedFunction();
assert(!breakpointHit);
testExecution();

This comment has been minimized.

@TimothyGu

TimothyGu Apr 7, 2017

Member

The test looks fairly convoluted, but I don't have any better idea to refactor it.

@TimothyGu

TimothyGu Apr 7, 2017

Member

The test looks fairly convoluted, but I don't have any better idea to refactor it.

Show outdated Hide outdated src/inspector_agent.cc
return;
}
inspector->Dispatch(ToProtocolString(env->isolate(),
info[0])->string());

This comment has been minimized.

@TimothyGu

TimothyGu Apr 7, 2017

Member

Indentation is off

@TimothyGu

TimothyGu Apr 7, 2017

Member

Indentation is off

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Fixed

@eugeneo

eugeneo Apr 17, 2017

Contributor

Fixed

Show outdated Hide outdated src/inspector_agent.cc
|| delegate != GetDelegate(env, info.This()).FromMaybe(nullptr)) {
env->ThrowError("Inspector is not connected");
return;
}

This comment has been minimized.

@TimothyGu

TimothyGu Apr 7, 2017

Member

The handling here basically means that if dispatch.call(null) is executed, two errors will be attempted to be thrown (one in GetDelegate()'s Get(), one here), which of course won't work. The approach taken by GetInspectorChecked() (i.e. throwing an error in GetDelegate() if the delegate is nullptr) should be better.

@TimothyGu

TimothyGu Apr 7, 2017

Member

The handling here basically means that if dispatch.call(null) is executed, two errors will be attempted to be thrown (one in GetDelegate()'s Get(), one here), which of course won't work. The approach taken by GetInspectorChecked() (i.e. throwing an error in GetDelegate() if the delegate is nullptr) should be better.

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

I redid the code. Original version was written before the signal handler was merged in. Now V8 inspector is always present and there's no need to check if it exists.

@eugeneo

eugeneo Apr 17, 2017

Contributor

I redid the code. Original version was written before the signal handler was merged in. Now V8 inspector is always present and there's no need to check if it exists.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Apr 7, 2017

Contributor

Thank you for the feedback.

I will work on docs next week, will update this PR then. I will also look into user-land JS module.

  1. At this point, there can be only one concurrent V8 inspector session. Same is true for Chrome - you cannot connect two frontends to the same page.
Contributor

eugeneo commented Apr 7, 2017

Thank you for the feedback.

I will work on docs next week, will update this PR then. I will also look into user-land JS module.

  1. At this point, there can be only one concurrent V8 inspector session. Same is true for Chrome - you cannot connect two frontends to the same page.
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 8, 2017

Member

I think offering JS bindings to the inspector opens up the possibility for the ecosystem to write really cool debug and profiling tooling.

I'm sympathetic but this PR feels a little ad hoc. @TimothyGu already pointed out a few issues and it's not hard to poke more holes. This should be an EP first.

Member

bnoordhuis commented Apr 8, 2017

I think offering JS bindings to the inspector opens up the possibility for the ecosystem to write really cool debug and profiling tooling.

I'm sympathetic but this PR feels a little ad hoc. @TimothyGu already pointed out a few issues and it's not hard to poke more holes. This should be an EP first.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 10, 2017

Contributor

An EP sounds good. We can get that put together.

Can we add this as an experimental API in the interim? This allows all existing users of vm.runInDebugContext (given #12243) to at least have an alternative to prototype with.

Contributor

ofrobots commented Apr 10, 2017

An EP sounds good. We can get that put together.

Can we add this as an experimental API in the interim? This allows all existing users of vm.runInDebugContext (given #12243) to at least have an alternative to prototype with.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 10, 2017

Member

Can we add this as an experimental API in the interim?

I think that would be alright, the question is where? The v8 module?

Member

bnoordhuis commented Apr 10, 2017

Can we add this as an experimental API in the interim?

I think that would be alright, the question is where? The v8 module?

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Apr 11, 2017

Contributor
Contributor

eugeneo commented Apr 11, 2017

Show outdated Hide outdated test/inspector/test-bindings.js
assert.deepStrictEqual(failures, []);
assert.strictEqual(cur, 5);
scopeCallback = null;
session.disconnect();

This comment has been minimized.

@dgozman

dgozman Apr 13, 2017

I'd also add test for:

  • connect twice;
  • disconnect twice;
  • disconnect without connect;
  • send 1000 messages in a row;
  • pass non-function as callback;
  • dispatch without connect.
@dgozman

dgozman Apr 13, 2017

I'd also add test for:

  • connect twice;
  • disconnect twice;
  • disconnect without connect;
  • send 1000 messages in a row;
  • pass non-function as callback;
  • dispatch without connect.
}
Local<Object> session = Object::New(env->isolate());
env->SetMethod(session, "dispatch", Dispatch);
env->SetMethod(session, "disconnect", Disconnect);

This comment has been minimized.

@dgozman

dgozman Apr 13, 2017

I like the API overall.

@dgozman

dgozman Apr 13, 2017

I like the API overall.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Apr 17, 2017

Contributor

I have updated the branch with a new JS API. I also added initial revision of the docs for the module.

Contributor

eugeneo commented Apr 17, 2017

I have updated the branch with a new JS API. I also added initial revision of the docs for the module.

@jasnell

Still working through the whole PR but some initial requests...

Show outdated Hide outdated lib/inspector.js
assert(connect, 'Inspector is not available');
exports.Session = class extends EventEmitter {

This comment has been minimized.

@jasnell

jasnell Apr 17, 2017

Member

would prefer ...

class Session extends EventEmitter { /** ... **/ }

module.exports = {
  Session
};

for consistency with changes made recently in other modules.

@jasnell

jasnell Apr 17, 2017

Member

would prefer ...

class Session extends EventEmitter { /** ... **/ }

module.exports = {
  Session
};

for consistency with changes made recently in other modules.

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Done.

Show outdated Hide outdated lib/inspector.js
const connect = process.binding('inspector').connect;
const EventEmitter = require('events');
assert(connect, 'Inspector is not available');

This comment has been minimized.

@jasnell

jasnell Apr 17, 2017

Member

Not convinced this should be an assert as opposed to a throw.

@jasnell

jasnell Apr 17, 2017

Member

Not convinced this should be an assert as opposed to a throw.

This comment has been minimized.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Indeed; the assert is generally used for testing rather than internally.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Indeed; the assert is generally used for testing rather than internally.

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Switched to exception, added a test.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Switched to exception, added a test.

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

(Sorry, it was about a different assert below).

@eugeneo

eugeneo Apr 17, 2017

Contributor

(Sorry, it was about a different assert below).

Show outdated Hide outdated lib/inspector.js
exports.Session = class extends EventEmitter {
constructor() {
super();
this._connection = connect((message) => this._onMessage(message));

This comment has been minimized.

@jasnell

jasnell Apr 17, 2017

Member

Would prefer using internal Symbols for these instead of _-prefixed properties.

@jasnell

jasnell Apr 17, 2017

Member

Would prefer using internal Symbols for these instead of _-prefixed properties.

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Can you take a look if I did it properly?

@eugeneo

eugeneo Apr 17, 2017

Contributor

Can you take a look if I did it properly?

@changsi-an

This comment has been minimized.

Show comment
Hide comment
@changsi-an

changsi-an Apr 17, 2017

Thank @ofrobots for considering about ecosystem usage. A product feature from my organization is going to heavily rely on vm.runInDebugContext

Is vm.runInDebugContext going to disappear from a Node uses V8 5.8? Do we know which Node version it will be?
What will be the best way to tell programmably whether vm.runInDebugContext is still available or whether the new 'inspect' module is available?

changsi-an commented Apr 17, 2017

Thank @ofrobots for considering about ecosystem usage. A product feature from my organization is going to heavily rely on vm.runInDebugContext

Is vm.runInDebugContext going to disappear from a Node uses V8 5.8? Do we know which Node version it will be?
What will be the best way to tell programmably whether vm.runInDebugContext is still available or whether the new 'inspect' module is available?

It can be accessed using:
```js
const inspector = require('inspector');

This comment has been minimized.

@TimothyGu

TimothyGu Apr 17, 2017

Member

This might conflict with existing npm "inspector" package by @AndreasMadsen. Pinging him to see if that package is still useful.

@TimothyGu

TimothyGu Apr 17, 2017

Member

This might conflict with existing npm "inspector" package by @AndreasMadsen. Pinging him to see if that package is still useful.

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Thanks!

@eugeneo

eugeneo Apr 17, 2017

Contributor

Thanks!

This comment has been minimized.

@TimothyGu
@TimothyGu

This comment has been minimized.

@ofrobots

ofrobots May 16, 2017

Contributor

If we don't hear back from @AndreasMadsen, I think we should rename the module to avoid conflict with the existing module on npm.

@ofrobots

ofrobots May 16, 2017

Contributor

If we don't hear back from @AndreasMadsen, I think we should rename the module to avoid conflict with the existing module on npm.

This comment has been minimized.

@eugeneo

eugeneo May 16, 2017

Contributor

Any suggestions for an alternative name? I see node-inspector and v8-inspectortaken, as well as jsinspector (so js-inspector seems to be available).

We could call it inspector-session - but the plan is to provide more inspector controls, such as start/stop server on specific port, programmatic access to UUID, adding domains, etc.

@eugeneo

eugeneo May 16, 2017

Contributor

Any suggestions for an alternative name? I see node-inspector and v8-inspectortaken, as well as jsinspector (so js-inspector seems to be available).

We could call it inspector-session - but the plan is to provide more inspector controls, such as start/stop server on specific port, programmatic access to UUID, adding domains, etc.

Show outdated Hide outdated doc/api/inspector.md
session.on('Debugger.paused', debuggerPausedCallback);
```
### session.post(method, [params], [callback])

This comment has been minimized.

@TimothyGu

TimothyGu Apr 17, 2017

Member
### session.post(method[, params][, callback])
@TimothyGu

TimothyGu Apr 17, 2017

Member
### session.post(method[, params][, callback])

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Done

@eugeneo

eugeneo Apr 17, 2017

Contributor

Done

Show outdated Hide outdated doc/api/inspector.md
<!-- YAML
added: v8.0.0
-->

This comment has been minimized.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Add argument type information:

* method {string} 
* params {Object}
* callback {Function}
@TimothyGu

TimothyGu Apr 17, 2017

Member

Add argument type information:

* method {string} 
* params {Object}
* callback {Function}

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

Done

@eugeneo

eugeneo Apr 17, 2017

Contributor

Done

Show outdated Hide outdated lib/inspector.js
}
post(method, params, callback) {
assert(this._connection, 'Session is not connected');

This comment has been minimized.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Ditto about assert.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Ditto about assert.

Show outdated Hide outdated lib/inspector.js
_onMessage(message) {
const parsed = JSON.parse(message);
if (parsed['id']) {

This comment has been minimized.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Is the use of ['id'] for performance?

@TimothyGu

TimothyGu Apr 17, 2017

Member

Is the use of ['id'] for performance?

This comment has been minimized.

@eugeneo

eugeneo Apr 17, 2017

Contributor

No. It was more because this is coming from JSON deserialization so the "object" is more of a "map"

@eugeneo

eugeneo Apr 17, 2017

Contributor

No. It was more because this is coming from JSON deserialization so the "object" is more of a "map"

Show outdated Hide outdated lib/inspector.js
const connect = process.binding('inspector').connect;
const EventEmitter = require('events');
assert(connect, 'Inspector is not available');

This comment has been minimized.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Indeed; the assert is generally used for testing rather than internally.

@TimothyGu

TimothyGu Apr 17, 2017

Member

Indeed; the assert is generally used for testing rather than internally.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Apr 17, 2017

Member

@changsi-an

Is vm.runInDebugContext going to disappear from a Node uses V8 5.8? Do we know which Node version it will be?

According to @ofrobots, it will disappear at the end of the year, which is Node.js v10.x (i.e. not V8 5.8).

What will be the best way to tell programmably whether vm.runInDebugContext is still available or whether the new 'inspect' module is available?

Checking if vm.runInDebugContext is defined should be the best way.

Member

TimothyGu commented Apr 17, 2017

@changsi-an

Is vm.runInDebugContext going to disappear from a Node uses V8 5.8? Do we know which Node version it will be?

According to @ofrobots, it will disappear at the end of the year, which is Node.js v10.x (i.e. not V8 5.8).

What will be the best way to tell programmably whether vm.runInDebugContext is still available or whether the new 'inspect' module is available?

Checking if vm.runInDebugContext is defined should be the best way.

Show outdated Hide outdated lib/inspector.js
class Session extends EventEmitter {
constructor() {
super();
this._connection = connect((message) => this[onMessageSymbol](message));

This comment has been minimized.

@TimothyGu

TimothyGu Apr 18, 2017

Member

These properties should be using symbols also.

@TimothyGu

TimothyGu Apr 18, 2017

Member

These properties should be using symbols also.

This comment has been minimized.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Done.

Show outdated Hide outdated doc/api/inspector.md
* method {string}
* params {Object}
* callback {Function([result][, error])}

This comment has been minimized.

@TimothyGu

TimothyGu Apr 18, 2017

Member

We don't usually put the arguments in the type annotation.

@TimothyGu

TimothyGu Apr 18, 2017

Member

We don't usually put the arguments in the type annotation.

This comment has been minimized.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Removed.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Removed.

Show outdated Hide outdated doc/api/inspector.md
Posts a message to the inspector backend. Optional `callback` will be notified
when a response is received. `callback` is a function that accepts two
optional arguments - message-specific result and error.

This comment has been minimized.

@TimothyGu

TimothyGu Apr 18, 2017

Member

In Node.js the norm is (err, data).

@TimothyGu

TimothyGu Apr 18, 2017

Member

In Node.js the norm is (err, data).

This comment has been minimized.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Done.

Show outdated Hide outdated doc/api/inspector.md
when a response is received. `callback` is a function that accepts two
optional arguments - message-specific result and error.
### session.disconnect

This comment has been minimized.

@TimothyGu

TimothyGu Apr 18, 2017

Member
### session.disconnect()
@TimothyGu

TimothyGu Apr 18, 2017

Member
### session.disconnect()

This comment has been minimized.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Done.

Show outdated Hide outdated lib/inspector.js
post(method, params, callback) {
if (!this._connection)
throw new Error('Session is not connected');
const id = this._nextId++;

This comment has been minimized.

@TimothyGu

TimothyGu Apr 18, 2017

Member

IDs are only alive for the duration of a Session, right? In other words, if I open and close one session, and then open another session, is it guaranteed that the IDs from the two sessions will not conflict?

@TimothyGu

TimothyGu Apr 18, 2017

Member

IDs are only alive for the duration of a Session, right? In other words, if I open and close one session, and then open another session, is it guaranteed that the IDs from the two sessions will not conflict?

This comment has been minimized.

@eugeneo

eugeneo Apr 24, 2017

Contributor

Session will not see responses/notifications for messages from another session (either closed or open)

@eugeneo

eugeneo Apr 24, 2017

Contributor

Session will not see responses/notifications for messages from another session (either closed or open)

Show outdated Hide outdated doc/api/inspector.md
At any given moment there may be only one connected session (created either
with this API or by connecting to an inspector socket). Creating a second
session using this API will throw an exception.

This comment has been minimized.

@changsi-an

changsi-an Apr 24, 2017

Can an external debugger, like Chrome Dev Tools, interrupt a inspector session created this way?

@changsi-an

changsi-an Apr 24, 2017

Can an external debugger, like Chrome Dev Tools, interrupt a inspector session created this way?

This comment has been minimized.

@eugeneo

eugeneo Apr 24, 2017

Contributor

No. It will detect there's an ongoing session and close the socket, same behaviour as what you get if you are trying to connect from 2 frontends concurrently.

@eugeneo

eugeneo Apr 24, 2017

Contributor

No. It will detect there's an ongoing session and close the socket, same behaviour as what you get if you are trying to connect from 2 frontends concurrently.

This comment has been minimized.

@changsi-an

changsi-an Apr 24, 2017

Gotcha, thanks :)

@changsi-an

changsi-an Apr 24, 2017

Gotcha, thanks :)

This comment has been minimized.

@sam-github

sam-github May 11, 2017

Member

What about if an inspector protocol session is ongoing, will this throw? Also, info in your above comment should be added to the docs.

@sam-github

sam-github May 11, 2017

Member

What about if an inspector protocol session is ongoing, will this throw? Also, info in your above comment should be added to the docs.

@TimothyGu

I like the API much better than the old one, and thanks for fixing the process.inspector kludge as well.

Show outdated Hide outdated lib/inspector.js
[onMessageSymbol](message) {
const parsed = JSON.parse(message);
if (parsed['id']) {
var callback = this[messageCallbacksSymbol].get(parsed['id']);

This comment has been minimized.

@TimothyGu

TimothyGu Apr 24, 2017

Member

Nit: this can be a const

@TimothyGu

TimothyGu Apr 24, 2017

Member

Nit: this can be a const

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

Show outdated Hide outdated doc/api/inspector.md
Up-to-date version of the V8 inspector protocol is published on the
[protocol viewer][protocol-viewer]
### Event 'notification'

This comment has been minimized.

@TimothyGu

TimothyGu Apr 24, 2017

Member

Am I understanding correctly that the name of the event can actually be any event specified in Chrome Debugging Protocol? If so, you should make it clear in the heading that this isn't talking about a specific event named 'notification'.

@TimothyGu

TimothyGu Apr 24, 2017

Member

Am I understanding correctly that the name of the event can actually be any event specified in Chrome Debugging Protocol? If so, you should make it clear in the heading that this isn't talking about a specific event named 'notification'.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Actually, you can subscribe either to "protocolNotification" to recieve all notifications or to some specific notifications, like the 'Debugger.paused' one. I updated the doc.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Actually, you can subscribe either to "protocolNotification" to recieve all notifications or to some specific notifications, like the 'Debugger.paused' one. I updated the doc.

Show outdated Hide outdated doc/api/inspector.md
the method. E.g. in this snippet the `debuggerPausedCallback` will be called
whenever the program execution is suspended:
```js
session.on('Debugger.paused', debuggerPausedCallback);

This comment has been minimized.

@TimothyGu

TimothyGu Apr 24, 2017

Member

A link to https://chromedevtools.github.io/debugger-protocol-viewer/v8/Debugger/#event-paused can be helpful here, and an example showing the arguments debuggerPausedCallback should make it clearer too. If my understanding is correct, it should be something like

session.on('Debugger.paused', ({ callFrames, reason }) => {
  // ...
});

right?

@TimothyGu

TimothyGu Apr 24, 2017

Member

A link to https://chromedevtools.github.io/debugger-protocol-viewer/v8/Debugger/#event-paused can be helpful here, and an example showing the arguments debuggerPausedCallback should make it clearer too. If my understanding is correct, it should be something like

session.on('Debugger.paused', ({ callFrames, reason }) => {
  // ...
});

right?

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

Show outdated Hide outdated doc/api/inspector.md
added: v8.0.0
-->
Immediately closes the session. Some pending callbacks may not receive

This comment has been minimized.

@TimothyGu

TimothyGu Apr 24, 2017

Member

What callbacks are you referring to? The callbacks passed to session.post()?

@TimothyGu

TimothyGu Apr 24, 2017

Member

What callbacks are you referring to? The callbacks passed to session.post()?

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Yes.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Yes.

Show outdated Hide outdated doc/api/inspector.md
Posts a message to the inspector backend. Optional `callback` will be notified
when a response is received. `callback` is a function that accepts two
optional arguments - error and message-specific result.

This comment has been minimized.

@TimothyGu

TimothyGu Apr 24, 2017

Member

I'd like to see an example here as well.

@TimothyGu

TimothyGu Apr 24, 2017

Member

I'd like to see an example here as well.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

Show outdated Hide outdated lib/inspector.js
}
}
post(method, params, callback) {

This comment has been minimized.

@TimothyGu

TimothyGu Apr 24, 2017

Member

There should be type checks for method and callback.

@TimothyGu

TimothyGu Apr 24, 2017

Member

There should be type checks for method and callback.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

Show outdated Hide outdated doc/api/inspector.md
## Class: inspector.Session
<!-- YAML
added: v8.0.0

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Can you change v8.0.0 to REPLACEME.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Can you change v8.0.0 to REPLACEME.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

Show outdated Hide outdated doc/api/inspector.md
### Event 'notification'
<!-- YAML
added: v8.0.0

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Same comment about REPLACEME here and throughout the PR.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Same comment about REPLACEME here and throughout the PR.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

Show outdated Hide outdated lib/inspector.js
const parsed = JSON.parse(message);
if (parsed['id']) {
var callback = this[messageCallbacksSymbol].get(parsed['id']);
if (callback)

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Maybe check if callback is actually a function.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Maybe check if callback is actually a function.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

Show outdated Hide outdated test/inspector/test-bindings.js
} catch (error) {
// expected as the session already exists
}
assert(!secondSessionOpened);

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Can you use assert.strictEqual() please.

@cjihrig

cjihrig Apr 25, 2017

Contributor

Can you use assert.strictEqual() please.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done

Show outdated Hide outdated test/inspector/test-bindings.js
} catch (e) {
failedToSend = true;
}
assert.ok(failedToSend);

This comment has been minimized.

@cjihrig

cjihrig Apr 25, 2017

Contributor

strictEqual() here and throughout please.

@cjihrig

cjihrig Apr 25, 2017

Contributor

strictEqual() here and throughout please.

This comment has been minimized.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

@eugeneo

eugeneo Apr 25, 2017

Contributor

Done.

@eugeneo eugeneo referenced this pull request Apr 25, 2017

Merged

inspector: do not add 'inspector' property #12656

2 of 2 tasks complete
@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Apr 25, 2017

Contributor

I split this pull request into 2 commits - one is a refactoring to remove the process.inspector 'API' and the second commit is for actually creating the bindings.

First commit was also uploaded as #12656

Contributor

eugeneo commented Apr 25, 2017

I split this pull request into 2 commits - one is a refactoring to remove the process.inspector 'API' and the second commit is for actually creating the bindings.

First commit was also uploaded as #12656

anchnk added a commit to anchnk/node that referenced this pull request May 6, 2017

inspector: do not add 'inspector' property
'inspector' property is not an official API and should not be published
on process object, where the user may discover it.

This change was extracted from nodejs#12263
that will be focused on creating JS bindings.

PR-URL: nodejs#12656
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@sam-github

sam-github requested changes May 12, 2017 edited

 const fs = require('fs');                                                        
+ const inspector = require('inspector');                                          
+ const session = new inspector.Session();                                         
+                                                                                  
+ session.connect();                                                               
+                                                                                  
+ fs.writeFileSync('_.heapsnapshot', '');                                          
+                                                                                  
+ session.on('HeapProfiler.addHeapSnapshotChunk', function(m) {                    
+   fs.appendFileSync('_.heapsnapshot', m.params.chunk);                           
+ });                                                                              
+                                                                                  
+ session.post('HeapProfiler.takeHeapSnapshot', null, function(err, r) {           
+   console.log('Runtime.takeHeapSnapshot done:', err, r)                          
+   session.disconnect();                                                          
+ });

I got an example working, have some feedback based on it. Feel free to use the example in the docs, it shows how to use the event handling with a posted message.

Show outdated Hide outdated doc/api/inspector.md
the V8 inspector backend and receiving message responses and notifications.
It needs to be connected before the messages can be sent to the inspector
backend.

This comment has been minimized.

@sam-github

sam-github May 12, 2017

Member

The constructor needs docs, even though it doesn't take args, without I had to guess that session = new inspector.Session()

@sam-github

sam-github May 12, 2017

Member

The constructor needs docs, even though it doesn't take args, without I had to guess that session = new inspector.Session()

This comment has been minimized.

@eugeneo

eugeneo May 12, 2017

Contributor

Done.

@eugeneo

eugeneo May 12, 2017

Contributor

Done.

* method {string}
* params {Object}
* callback {Function}

This comment has been minimized.

@sam-github

sam-github May 12, 2017

Member

any errors thrown by the callback are caught and ignored, though a message is written to stderr:

Exception in the inspector message handler: Uncaught ReferenceError: result is not defined

That doesn't seem right, shouldn't it propogate up as an uncaught exception?

@sam-github

sam-github May 12, 2017

Member

any errors thrown by the callback are caught and ignored, though a message is written to stderr:

Exception in the inspector message handler: Uncaught ReferenceError: result is not defined

That doesn't seem right, shouldn't it propogate up as an uncaught exception?

This comment has been minimized.

@eugeneo

eugeneo May 12, 2017

Contributor

Native code now propagates the exception.

Sending "uncaughtException" event may be cleaner - I am not entirely sure. Also, how do I send that event - do I do process.emit('uncaughtException', e) or something like setTimeout(function() { throw exception; })

@eugeneo

eugeneo May 12, 2017

Contributor

Native code now propagates the exception.

Sending "uncaughtException" event may be cleaner - I am not entirely sure. Also, how do I send that event - do I do process.emit('uncaughtException', e) or something like setTimeout(function() { throw exception; })

Show outdated Hide outdated doc/api/inspector.md
```js
session.post('Runtime.evaluate', {'expression': '2 + 2'},
(error, {result}) => console.log(result.value));
```

This comment has been minimized.

@sam-github

sam-github May 12, 2017

Member

Examples are more useful with output shown:

// Output: { type: 'number', value: 4, description: '4' }
@sam-github

sam-github May 12, 2017

Member

Examples are more useful with output shown:

// Output: { type: 'number', value: 4, description: '4' }

This comment has been minimized.

@eugeneo

eugeneo May 12, 2017

Contributor

Done

@eugeneo

eugeneo May 12, 2017

Contributor

Done

Show outdated Hide outdated doc/api/inspector.md
It is also possible to subscribe to a specific inspector notification type by
specifying the method. E.g. this snippet will print the reason for application
been suspended when the [Debugger.paused][debugger-paused] event is recieved

This comment has been minimized.

@sam-github

sam-github May 12, 2017

Member

"received"

@sam-github

sam-github May 12, 2017

Member

"received"

This comment has been minimized.

@eugeneo

eugeneo May 12, 2017

Contributor

Done.

@eugeneo

eugeneo May 12, 2017

Contributor

Done.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo May 12, 2017

Contributor

@joshgav

why not land on 7.x?

Inspector needs to install some V8 listeners that conflict with the old debugger. There will be issues around debug signal handler and also bindings will have to be disabled when "-debug" is passed, making it confusing for the users.

Contributor

eugeneo commented May 12, 2017

@joshgav

why not land on 7.x?

Inspector needs to install some V8 listeners that conflict with the old debugger. There will be issues around debug signal handler and also bindings will have to be disabled when "-debug" is passed, making it confusing for the users.

Show outdated Hide outdated doc/api/inspector.md
The following snippet installs a listener on the [`Debugger.paused`][]
event, and prints the reason for program suspension whenever program
execution is suspended (through breakpoints, for example):
```js

This comment has been minimized.

@addaleax