Skip to content
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

RFC: Support for subprocess inspection #77

Closed
eugeneo opened this issue Dec 7, 2016 · 9 comments
Closed

RFC: Support for subprocess inspection #77

eugeneo opened this issue Dec 7, 2016 · 9 comments

Comments

@eugeneo
Copy link

eugeneo commented Dec 7, 2016

Chrome DevTools protocol has a domain for working with subtargets (https://chromedevtools.github.io/debugger-protocol-viewer/tot/Target/). In the browser case, subtargets are be web/service workers, nested IFrames, etc.

In the Node case a subtarget may be a forked Node process. Introducing such a domain would remove a need to separately connect to forked instances.

Current proposal is to introduce a dedicated inspector pipe between parent and child process (in addition to current ipc pipe). This way there will be no need to look for a free port and to start up inspector HTTP server in every subprocess.

@Qard
Copy link
Member

Qard commented Dec 7, 2016

That would be very cool. I wonder a bit about backwards compatibility importance though. Is there any existing tooling that deals with debugging a cluster, for example?

@auchenberg
Copy link

auchenberg commented Dec 7, 2016

The target domain in CDP 1.2 is marked as experimental, and after recent conversations with the Chrome DevTools team, it's my understanding that they aren't too happy about the API design, as it's essentially embedded CDP commands within the Target.sendMessageToTarget. This should probably be factored into the considerations of using this existing API.

(@paulirish or @pavelfeldman can probably clarify)

@jkrems
Copy link
Contributor

jkrems commented Dec 7, 2016

I implemented something like it when I was working on bugger[1]. As long as the worker is spawned via child_process#fork (e.g. using cluster), it's relatively straight-forward. The fact that it's just "CDP over CDP" makes the implementation really simple but I get why it isn't the most elegant API. One of the downsides was that workers weren't treated as fully featured debug targets in the UI (understandable given that they're not on the web).

[1] https://github.com/buggerjs/bugger/blob/master/lib/agents/worker/index.js

@eugeneo
Copy link
Author

eugeneo commented Dec 7, 2016

@Qard this would be Inspector-specific (e.g. require a --inspect). I don't think it would be possible to use existing IPC pipe (not sure what interplay would be if V8 is suspended on a breakpoint and such).

So, it most likely will be some new command line flag (--inspect-pipe=<path/to/pipe>?) I don't think it would have impact on backwards compatibility...

@pavelfeldman
Copy link

Target API is indeed not the most elegant extensibility implementation, but there are things that speak in its favor:

  • it is very simple and hackable
  • it proved to work for us in production for more than a year (it was powering workers and service workers before got renamed into Target)
  • it allows nested targets of various types

So I think that we could stick with it. Supporting it in Node means making it public. Let us review it and present to you.

@Qard
Copy link
Member

Qard commented Dec 7, 2016

I wonder if it'd be possible to provide an API for the targets feature to programmatically allow linking frontend to backend or linking isolated nodes in a distributed system. It'd be very cool to be able to inspect multiple levels of the stack at the same time, in the same UI.

@joshgav
Copy link
Contributor

joshgav commented Jan 19, 2017

Seems like this would be ideal for process managers like PM2 and indeed anyone relying on the cluster module and multiple processes per system. Right now they'd have to add args to child_process/cluster.fork, as e.g. here: https://github.com/nodejs/node/blob/master/test/parallel/test-cluster-debug-port.js

FWIW seems the name of the domain should be "Proxy" or "InspectorProxy" rather than "Target" though, as the original debuggee/target is in fact acting as that.

@Qard

allow linking frontend to backend

+1, would be great to somehow discover and be able to automatically attach to backend (i.e. Node) from frontend.

eugeneo referenced this issue in nodejs/node May 25, 2017
Both our team experiments and some embedder request indicate a potential
in implementing alternative transport for inspector - e.g. IPC pipes or
custom embedder APIs. This change moves all HTTP specific code into a
separate class and is a first attempt at defining a boundary between the
inspector agent and transport. This API will be refined as new
transports are implemented.
Note that even without considering alternative transports, this change
enables better testing of the HTTP server (Valgrind made it possible to
identify and fix some existing memory leaks).

PR-URL: #9630
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@refack
Copy link

refack commented May 25, 2017

Ref: nodejs/node#12941

@eugeneo
Copy link
Author

eugeneo commented May 16, 2019

This had been implemented some time ago.

@eugeneo eugeneo closed this as completed May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants