inspector: allow --inspect=host:port from js #13228

sam-github · 2017-05-26T02:08:26Z

Use case is to be able to turn on/off the inspector websocket port, control the port and host it binds to, and determine the current UUID, so that information can be transmitted out-of-band to remote inspect clients.

/to @eugeneo , this builds on top of your recent work on js APIs for the inspector.
/to @nodejs/diagnostics As discussed in last WG meeting.

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)

src, inspector

sam-github · 2017-05-26T02:15:05Z

doc/api/inspector.md

+Return the UUID string of the listener, or undefined if the listener is not
+started.
+
+### ws.host([newHost])


technically, there are 2 objects here. the configuration, which can have a getter and setter (or a unified function like I did here), and that actual running listener (if it is running). its not possible to change the port on the fly, the current listener would have only a getter. So, there could be 3 functions... or one, like here, and a warning that the new value won't take effect until a new listener is started. Thoughts?

I am not sure ws should be a singleton.

Implementation-wise we can listen on multiple ports. Also, we were considering a pipe transport in addition to the WS transport for scenarios like subprocesses.

So, maybe expose in API support for multiple "servers"? Then you would start a server on a different port and shutdown the one that was running before.

sam-github · 2017-05-26T02:15:52Z

doc/api/inspector.md

+### ws.url.ws()
+
+Return a websocket URL that can be used to connect to the listener with
+a debugger that supports the inspect protocol.


this could also be ws.url(), its just a simple ws://host:port/uuid string.

Should be just a property

Would have to be at least a getter (the UUID isn't constant, it changes every time the server socket is closed).

sam-github · 2017-05-26T02:16:53Z

doc/api/inspector.md

+
+### ws.url.chrome()
+
+Return a chrome devtools URL that can be used to open the Chrome debugger


this URL is long, complex, and chrome specific, which is why I think having a method to insulate us from that is useful. Then again, these URL helpers are sugar, people can always construct the URLs themselves from host, port, UUID. Thoughts?

I think this API is not necessary - exactly because the URL can be reconstructed based on parameters.

sam-github · 2017-05-26T02:19:33Z

lib/inspector.js

+    id: wsId,
+    listen: wsListen,
+    unlisten: process._debugEnd,
+    // XXX find a way to just stop Io thread, not entire agent, so ID can be stable?


@eugeneo Am I thinking of it wrong? Would it be useful to have a way to stop the listener thread, then be able to start it again later with the same UUID? Or does it just not matter? I'm not sure what the UUID is supposed to identify, but if its a single still-living instance of the same process, then it should be persistent (debugEnd clears it). If its supposed to identify a specific instance of an agent listening on a TCP port, and change when the port is closed, the current behaviour is fine.

UUID is there for security, so web sites are not able to connect to Node instances running locally. So it does not represent anything.

It would likely be convinient if it stayed stable over the life of the Node process, but it does not matter much.

eugeneo · 2017-05-26T15:31:26Z

Thanks for looking into this!

I was planning to start a discussion about similar APIs. I believe it would be best to always start WS server from JS code (probably from the node_bootstrap.js).

eugeneo · 2017-05-26T15:31:52Z

(Sorry, thought that button clears the comment field)

eugeneo

Thanks for looking into this. I think we need some discussion to align the plans for the inspector future.

eugeneo · 2017-05-26T15:36:31Z

doc/api/inspector.md

+allow programatic control of the websocket, equivalent to use `--inspect*` CLI
+options.
+
+### ws.listen()


Maybe ws.listen(waitForFrontend[, host][, port])?

Also, I feel like the agent should always be started from JavaScript - node_bootstrap.js or something can call ws.listen based on command line arguments.

I was planning exposing something like inspector.onDebugSignal(callback) that would allow developers to customize WS server startup - e.g. to start a server on a different port.

What would the waitForFrontend argument be? What would it do?

It would be a boolean, and it would block the main thread until a debug client connects to the ws port and initiates a session, equivalent to the wait_for_connect boolean option of InspectorIo.

eugeneo · 2017-05-26T15:39:17Z

doc/api/inspector.md

+Return the UUID string of the listener, or undefined if the listener is not
+started.
+
+### ws.host([newHost])


I am not sure ws should be a singleton.

Implementation-wise we can listen on multiple ports. Also, we were considering a pipe transport in addition to the WS transport for scenarios like subprocesses.

So, maybe expose in API support for multiple "servers"? Then you would start a server on a different port and shutdown the one that was running before.

eugeneo · 2017-05-26T15:40:46Z

doc/api/inspector.md

+
+### ws.url.chrome()
+
+Return a chrome devtools URL that can be used to open the Chrome debugger


I think this API is not necessary - exactly because the URL can be reconstructed based on parameters.

eugeneo · 2017-05-26T15:43:27Z

lib/inspector.js

+    id: wsId,
+    listen: wsListen,
+    unlisten: process._debugEnd,
+    // XXX find a way to just stop Io thread, not entire agent, so ID can be stable?


UUID is there for security, so web sites are not able to connect to Node instances running locally. So it does not represent anything.

It would likely be convinient if it stayed stable over the life of the Node process, but it does not matter much.

eugeneo · 2017-05-26T15:44:06Z

lib/internal/cluster/master.js

@@ -72,6 +72,7 @@ cluster.setupMaster = function(options) {

  process.nextTick(setupSettingsNT, settings);

+  // XXX cluster masters turn debug on in all children? does that work?


I believe there are tests for that. I distinctly remember having to deal with them :)

eugeneo · 2017-05-26T15:45:11Z

src/inspector_agent.cc

@@ -667,6 +669,51 @@ void Agent::PauseOnNextJavascriptStatement(const std::string& reason) {
    channel->schedulePauseOnNextStatement(reason);
 }

+void WsListen(const FunctionCallbackInfo<Value>& args) {


I feel like Ws* bindings should be in the inspector_io.cc.

eugeneo · 2017-05-26T17:30:24Z

UUID may belong to inspector (it now lives pretty much as long as Node process lives) and become another ctor argument for the io. Another thing I've been thinking about - what if Inspector WS was a module? It will still need native code and another thread (as it needs to work while main thread is paused on a breakpoint) but it could use JS bindings to talk to the inspector backend.

…

On Thu, May 25, 2017 at 7:19 PM Sam Roberts ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/inspector.js <#13228 (comment)>: > module.exports = { + ws: { + id: wsId, + listen: wsListen, + unlisten: process._debugEnd, + // XXX find a way to just stop Io thread, not entire agent, so ID can be stable? @eugeneo <https://github.com/eugeneo> Am I thinking of it wrong? Would it be useful to have a way to stop the listener thread, then be able to start it again later with the same UUID? Or does it just not matter? I'm not sure what the UUID is supposed to identify, but if its a single still-living instance of the same process, then it should be persistent (debugEnd clears it). If its supposed to identify a specific instance of an agent listening on a TCP port, and change when the port is closed, the current behaviour is fine. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13228 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARkrT3a1vlPp7WUJMz7E08PN76i-Q25ks5r9ja4gaJpZM4NnGAk> .

eugeneo · 2017-05-26T18:59:08Z

@sam-github - I would start a discussion before landing this - #13247

I am wondering if this server should be separate from the core inspector.

jasnell · 2017-05-29T21:17:44Z

doc/api/inspector.md

+The Inspector can be interacted with via a websocket-based protocol. These APIs
+allow programatic control of the websocket, equivalent to use `--inspect*` CLI
+options.
+


I am very hesitant to do this. If we expose an API for this, then we need to make certain that this cannot be used for anything other than interaction via the Inspector protocol.

It can't be. As is, all this PR does (ATM) is allow instead of forcing --inspect to be provided as a CLI option, it allows node to startup, and then the js API calls the same C++ APIs called to turn on the listener that would have been called if --inspect had been provided. There is zero general purpose protocol or websocket capability exposed.

jasnell · 2017-05-29T21:18:43Z

doc/api/inspector.md

+Start the websocket listener.
+
+### ws.unlisten()
+


ws.close() is more idiomatic and consistent

ok. though it sounds like @eugeneo is looking for quite a different API, this is more of a POC at the moment.

jasnell · 2017-05-29T21:19:05Z

doc/api/inspector.md

+### ws.id()
+
+Return the UUID string of the listener, or undefined if the listener is not
+started.


Should just make this a property.

Maybe you think its a single unchanging value? But it isn't, if you run start.js you'll see 2 or 4 values of this UUID printed. So, it could be a getter, but not a fixed-value property.

jasnell · 2017-05-29T21:20:20Z

doc/api/inspector.md

+### ws.port([newPort])
+
+* port {number} Set a new port to bind to (will not effect a currently
+  started listener). Optional.


If there is an existing listener that is bound to a port, what happens?

right now, nothing, it has no effect (until the listeners is closed and listen is called again).

jasnell · 2017-05-29T21:21:25Z

lib/inspector.js

+function addr() {
+  const id = wsId();
+  if (!id)
+    throw new Error('inspector did not start');


If introducing new errors, please use internal/errors

jasnell · 2017-05-29T21:22:08Z

lib/inspector.js

+}
+
+function wsUrl() {
+  return 'ws://' + addr();


`ws://${addr()}`

likewise for chromeUrl?

jasnell · 2017-05-29T21:23:30Z

start.js

@@ -0,0 +1,39 @@
+const assert = require('assert');


I assume this file wasn't supposed to be checked in?

Its a WIP, this would eventually be test code, but it shows how to call the APIs for now.

PR-URL: nodejs#13228 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-06-07T20:31:48Z

Marking this as semver-minor post landing. If anyone feels this is incorrect, let me know

refack · 2017-06-07T20:33:28Z

Marking this as semver-minor post landing. If anyone feels this is incorrect, let me know

Isn't the inspector module experimental?

PR-URL: #13228 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

refack · 2017-06-08T14:15:04Z

lib/inspector.js

@@ -83,5 +83,8 @@ class Session extends EventEmitter {
 }

 module.exports = {
+  open: (port, host, wait) => open(port, host, !!wait),
+  close: process._debugEnd,
+  url: url,


Re: #9659 I now think this should have been a URL not a string...

Retrospectively document the changes history of the `inspector.close` API. PR-URL: #44628 Refs: #44489 Refs: #13228 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>

Retrospectively document the changes history of the `inspector.close` API. PR-URL: nodejs#44628 Refs: nodejs#44489 Refs: nodejs#13228 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>

Retrospectively document the changes history of the `inspector.close` API. PR-URL: #44628 Refs: #44489 Refs: #13228 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>

Retrospectively document the changes history of the `inspector.close` API. PR-URL: nodejs/node#44628 Refs: nodejs/node#44489 Refs: nodejs/node#13228 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. debugger inspector Issues and PRs related to the V8 inspector protocol labels May 26, 2017

sam-github force-pushed the api-start-inspector branch 2 times, most recently from c6df413 to 0b0a3ec Compare May 26, 2017 02:11

sam-github commented May 26, 2017

View reviewed changes

mscdex removed the debugger label May 26, 2017

eugeneo closed this May 26, 2017

eugeneo reopened this May 26, 2017

eugeneo reviewed May 26, 2017

View reviewed changes

sam-github mentioned this pull request May 26, 2017

Move Inspector WS server into a separate module. #13247

Closed

sam-github force-pushed the api-start-inspector branch from 0b0a3ec to 8d06ca1 Compare May 26, 2017 21:03

jasnell reviewed May 29, 2017

View reviewed changes

jasnell added the wip Issues and PRs that are still a work in progress. label May 30, 2017

jasnell changed the title ~~inspector: JavaScript bindings for the websocket~~ [WIP] inspector: JavaScript bindings for the websocket May 30, 2017

sam-github force-pushed the api-start-inspector branch from 8d06ca1 to 3169ee5 Compare June 1, 2017 00:12

sam-github force-pushed the api-start-inspector branch 2 times, most recently from 09cfba7 to a86f3f5 Compare June 6, 2017 18:32

inspector: allow --inspect=host:port from js

2791b36

PR-URL: nodejs#13228 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

sam-github force-pushed the api-start-inspector branch from a86f3f5 to 2791b36 Compare June 6, 2017 21:41

sam-github merged commit 2791b36 into nodejs:master Jun 6, 2017

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 7, 2017

jasnell pushed a commit that referenced this pull request Jun 7, 2017

inspector: allow --inspect=host:port from js

9be8b63

PR-URL: #13228 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

refack reviewed Jun 8, 2017

View reviewed changes

sam-github mentioned this pull request Jun 9, 2017

Turn Node Debug on/off (call V8 API) RuntimeTools/appmetrics#388

Open

refack mentioned this pull request Jun 11, 2017

refactor test/inspector #13622

Closed

sam-github deleted the api-start-inspector branch June 14, 2017 20:31

refack mentioned this pull request Jun 19, 2017

Added inspect port overriding in cluster #13761

Closed

4 tasks

refack mentioned this pull request Jul 23, 2017

inspector: Allow require in Runtime.evaluate #8837

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

legendecas mentioned this pull request Sep 13, 2022

doc,inspector: document changes of inspector.close #44628

Closed


		### ws.url.chrome()

		Return a chrome devtools URL that can be used to open the Chrome debugger

		@@ -72,6 +72,7 @@ cluster.setupMaster = function(options) {

		process.nextTick(setupSettingsNT, settings);

		// XXX cluster masters turn debug on in all children? does that work?

inspector: allow --inspect=host:port from js #13228

inspector: allow --inspect=host:port from js #13228

Conversation

sam-github commented May 26, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented May 26, 2017

eugeneo commented May 26, 2017

eugeneo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented May 26, 2017 via email

eugeneo commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 7, 2017

refack commented Jun 7, 2017

Choose a reason for hiding this comment

sam-github commented May 26, 2017 •

edited

Loading