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

Support auto attach for node.js subprocesses (aka cluster support) #40123

Closed
weinand opened this issue Dec 12, 2017 · 25 comments
Closed

Support auto attach for node.js subprocesses (aka cluster support) #40123

weinand opened this issue Dec 12, 2017 · 25 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Dec 12, 2017

While working on the Process viewer, I noticed that we could easily support node.js "Clusters" (or any other spawned processes) with a similar approach.

Here is a sketch:

  • introduce a new launch config attribute "autoAttachChildren"
  • if a launch config has "autoAttachChildren" set to true, node-debug starts to track creation of subprocesses of the debuggee.
  • if a subprocess is recognised as being in debug mode (e.g. started with one of more of the flags --inspect, --inspect-brk, --inspect-port, --debug, --debug-brk, --debug-port), node-debug starts an "attach" debug session for the given port.

The result looks like this (and a first cut will be available in the next Insiders).

2017-12-12_17-16-00

I've verified the feature (for macOS only) with the example from the node.js documentation for 'cluster':

const cluster = require('cluster');
const http = require('http');
const numCPUs = require('os').cpus().length;

if (cluster.isMaster) {
	console.log(`Master ${process.pid} is running`);

	// Fork workers.
	for (let i = 0; i < numCPUs; i++) {
		cluster.fork();
	}

	cluster.on('exit', (worker, code, signal) => {
		console.log(`worker ${worker.process.pid} died`);
	});
} else {
	// Workers can share any TCP connection
	// In this case it is an HTTP server
	http.createServer((req, res) => {
		res.writeHead(200);
		res.end('hello world\n');
	}).listen(8000);

	console.log(`Worker ${process.pid} started`);
}

Here is the launch config:

{
	"type": "node",
	"request": "launch",
	"name": "Cluster",
	"program": "${workspaceFolder}/test.js",
	"autoAttachChildren": true
},

This already works fine for node.js < v8 (legacy).
For node.js >= v8 the problem is that all workers stop on entry, which requires some manual "continuing". The problem is that node-debug2 does not automatically send a "continue" request because it guesses that a "stop on entry" is desired. Node-debug1 had the same problem but I made it "guess" better in this case...

The work around for node.js >= v8 uses an explicit "--inspect" instead of "--inspect-brk":

{
	"type": "node",
	"request": "launch",
	"name": "Cluster (--inspect)",
	"runtimeArgs": [
		"--inspect=50000"
	],
	"port": 50000,
	"program": "${workspaceFolder}/test.js",
	"autoAttachChildren": true
},

The fix for node-debug was to only set this._stopOnEntry if it hasn't been set in the launch config:
see https://github.com/Microsoft/vscode-node-debug/blob/9a9ccb1703741fbad10e29d1769c0b8fc5cd3228/src/node/nodeDebug.ts#L1517

/cc @roblourens @auchenberg @isidorn

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Dec 12, 2017
@weinand weinand self-assigned this Dec 12, 2017
weinand added a commit to microsoft/vscode-node-debug that referenced this issue Dec 12, 2017
@weinand weinand added this to the December 2017/January 2018 milestone Dec 12, 2017
@roblourens
Copy link
Member

This is cool. Here's an issue from someone debugging with the cluster module: #39033

How does cluster.fork select a debug port? Does it automatically pick incrementing port numbers?

@weinand
Copy link
Contributor Author

weinand commented Dec 12, 2017

@roblourens yes, cluster increments port numbers like this (from the Process Viewer):

"legacy" as expected:
2017-12-12_22-03-51

"inspect" by using an interesting "inspect-port" flag:
2017-12-12_22-02-36

If a port is specified, "inspect-port" overrides the "inspect" port:
2017-12-12_22-08-18

@weeco
Copy link

weeco commented Dec 12, 2017

Finally! Looks like a nice feature. Does it have any downsides or would this feature be enabled by default?

@weinand
Copy link
Contributor Author

weinand commented Dec 12, 2017

"autoAttachChildren" is an idiotic attribute name.
Any suggestions for a better name?

@roblourens
Copy link
Member

I don't think it's that bad. That's exactly what it does. I can only think of some variations on it like attachToChildProcs. You probably don't want to tie it to cluster because it would work for any type of spawned process.

@roblourens
Copy link
Member

The generated config maybe should copy outFiles and other props from the original config.

@weeco
Copy link

weeco commented Dec 12, 2017

I agree with rob, for me the name of this setting tells what it is supposed to do. I prefer childProcess over children though.

@weinand
Copy link
Contributor Author

weinand commented Dec 12, 2017

Then let's go with "autoAttachChildProcesses" (yes, it's long but we have IntelliSense...)

roblourens added a commit to microsoft/vscode-node-debug2 that referenced this issue Dec 12, 2017
@jez9999
Copy link

jez9999 commented Dec 17, 2017

This is great - it should be built into VS Code. Anyone who wants to debug a parent and its child processes in Node wants this.

@weinand
Copy link
Contributor Author

weinand commented Dec 17, 2017

A first cut is already in VS Code Insiders.

@weinand
Copy link
Contributor Author

weinand commented Dec 20, 2017

@roblourens I've verified (by patching Insiders) that your fix would work fine.
But I couldn't build/package node-debug2 for the marketplace (how do you build node-debug2?). It would be great if you could publish a 1.20.0 version.

@roblourens
Copy link
Member

I just press cmd+shift+b :) What problem did you have? I just published 1.20.0.

@weinand
Copy link
Contributor Author

weinand commented Dec 20, 2017

gulp package fails for me (I wanted to produce a VSIX for installing it in Insiders).

@roblourens
Copy link
Member

It works for me

@weinand
Copy link
Contributor Author

weinand commented Dec 20, 2017

Strange.
I did:

git clean -xfd .
npm install
gulp package

and got:
2017-12-20_18-21-18

Are you using an 'npm link' for vscode-chrome-debug-core?

@roblourens
Copy link
Member

No, but I just cleaned everything and I see the same thing. Something wrong with my package-lock.json?

@roblourens
Copy link
Member

If I delete and regenerate the package-lock, there are a million changes and npm list doesn't fail...?

@weinand
Copy link
Contributor Author

weinand commented Dec 20, 2017

Interesting: after regenerate the package-lock not only all the error are gone, but the strange problem with the version attribute being a url is gone too:
2017-12-21_00-20-37

@roblourens
Copy link
Member

roblourens commented Dec 21, 2017

The last few times I've done something that changes the package-lock.json, I've seen it randomly add and remove those

@weinand
Copy link
Contributor Author

weinand commented Dec 21, 2017

This feature works now in Insiders.

Try this sample:

const cluster = require('cluster');
const http = require('http');
const numCPUs = require('os').cpus().length;

if (cluster.isMaster) {
	console.log(`Master ${process.pid} is running`);

	// Fork workers.
	for (let i = 0; i < numCPUs; i++) {
		cluster.fork();
	}

	cluster.on('exit', (worker, code, signal) => {
		console.log(`worker ${worker.process.pid} died`);
	});
} else {
	// Workers can share any TCP connection
	// In this case it is an HTTP server
	http.createServer((req, res) => {
		res.writeHead(200);
		res.end('hello world\n');
	}).listen(8000);

	console.log(`Worker ${process.pid} started`);
}

with this launch config:

{
	"type": "node",
	"request": "launch",
	"name": "Cluster",
	"program": "${workspaceFolder}/test.js",
	"autoAttachChildProcesses": true
},

@ddorkin-issart
Copy link

ddorkin-issart commented Jan 24, 2018

Hello @weinand I can't set up http server on each cluster according with your code.
My environments:

  1. windows 10 x64
  2. vs code version 1.20.0-insider.
  3. node v6.3.1

Output data:

Debugger listening on [::]:11036
Master 16144 is running
Debugger listening on [::]:11037
Debugger listening on [::]:11038
Debugger listening on [::]:11039
Debugger listening on [::]:11040

May be you can clarify why cluster.fork() command doesn't achieve next parts of script.

@weinand
Copy link
Contributor Author

weinand commented Jan 24, 2018

@ddorkin-issart How does your launch config look like?

@mdhornet90
Copy link

@weinand This is excellent when using npm, but is there anything special that needs to be done to incorporate nodemon on a cluster?

@weinand
Copy link
Contributor Author

weinand commented Jan 30, 2018

@mdhornet90 this feature has nothing to do with npm.
Here are the requirements:

  • in order to be able to track the subprocesses, we need the process id. For this we require that the main debuggee launched from the launch config is a node.js process and we use an "evaluate" to find its process id.
  • in order to be able to attach to the subprocesses they must be in debug mode. The feature determines whether a process is in debug mode by analysing the program arguments. Currently we detect the patterns --inspect, --inspect-brk, --inspect-port, --debug, --debug-brk, --debug-port (all optionally followed by a '=' and a port number).

@mdhornet90
Copy link

Alright, so this feature is independent of how node is started, thanks for the insight!

@vscodebot vscodebot bot locked and limited conversation to collaborators Feb 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

No branches or pull requests

6 participants