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

can --inspect be made to bind debug ports for child processes too? #459

Closed
AndrewJDR opened this issue Nov 9, 2017 · 13 comments
Closed
Labels
bug research Needs design work, investigation, or prototyping. Implementation uncertain. you can do this Good candidate for a pull request.

Comments

@AndrewJDR
Copy link

repro steps (I did this on linux and bash):

git clone https://github.com/AndrewJDR/inspect-on-forked-processes-doesnt-work
cd inspect-on-forked-processes-doesnt-work
npm i
npm start

This starts a child process using cluster.fork(). Note that debugger listens fine on the master process:

Debugger listening on ws://127.0.0.1:9229/1ffaac86-3b3f-4097-aa90-df1eba66b6f4
launching child
in the parent: 13039
in the child: 13049

And checking netstat shows that 9229 is indeed bound, which is good.

$ netstat -lnp | grep 9229
tcp        0      0 127.0.0.1:9229          0.0.0.0:*               LISTEN      13039/node      

However, no debugging port is bound for the child process. The output of ps, the --inspect-port=9230 argument would lead one to believe that a debugger port 9230 is available for the child process:

$ ps aux | grep ts-node
13027 pts/30   S+     0:00 sh -c ts-node --inspect -F index.ts
13028 pts/30   Sl+    0:00 node ~/Documents/src/inspect-on-forked-processes-doesnt-work/node_modules/.bin/ts-node --inspect -F index.ts
13039 pts/30   Sl+    0:00 ~/.nvm/versions/node/v9.0.0/bin/node --inspect ~/Documents/src/inspect-on-forked-processes-doesnt-work/node_modules/ts-node/dist/_bin.js -F index.ts
13049 pts/30   Sl+    0:00 ~/.nvm/versions/node/v9.0.0/bin/node ~/Documents/src/inspect-on-forked-processes-doesnt-work/node_modules/ts-node/dist/_bin.js --inspect --inspect-port=9230 ~/Documents/src/inspect-on-forked-processes-doesnt-work/index.ts

However, this is unfortunately not the case, looking at netstat output:

$ netstat -lnp | grep 9230
(.. nothing to see here ..)

Can others reproduce this? If so, can this please be addressed so that we can debug child processes properly?

@AndrewJDR
Copy link
Author

AndrewJDR commented Nov 9, 2017

Another note, this does behave properly when using node directly, like so:

git clone https://github.com/AndrewJDR/inspect-on-forked-processes-doesnt-work
cd inspect-on-forked-processes-doesnt-work
npm i
tsc
cd dist
node --inspect index.js

produces:

Debugger listening on ws://127.0.0.1:9229/3ceeb68c-e9a0-481e-b69d-961c640dfd51
For help see https://nodejs.org/en/docs/inspector
launching child
Debugger listening on ws://127.0.0.1:9230/cf548fe0-7358-469d-af91-405eb886bafc
For help see https://nodejs.org/en/docs/inspector
in the parent: 16502
in the child: 16510

Which already looks better, since we have two "Debugger listening on..." messages instead of just the one I see with ts-node. Just to be sure, I confirmed via netstat that both ports are bound, and they are. So this seems like a ts-node issue, not a node issue.

@blakeembrey
Copy link
Member

That's interesting. ts-node just starts an underlying node.js process by spawning _bin.js. Can you check if changing __filename in

process.execArgv.unshift(__filename)
to join(__dirname, 'bin.js') fixes this? The result would basically be having the child process spawn a child node.js process the same way ts-node does.

@blakeembrey blakeembrey added you can do this Good candidate for a pull request. research Needs design work, investigation, or prototyping. Implementation uncertain. bug labels Nov 15, 2017
@AndrewJDR
Copy link
Author

AndrewJDR commented Nov 15, 2017

I tried process.execArgv.unshift(require('path').join(__dirname, 'bin.js')); and it didn't work. This is the error I get:

 $ npm start

> inspect-on-forked-processes-doesnt-work@1.0.0 start ~/Documents/src/inspect-on-forked-processes-doesnt-work
> ts-node --inspect index.ts

Debugger listening on ws://127.0.0.1:9229/ed2df97b-b3cd-4f42-a422-16bf231cecd2
For help see https://nodejs.org/en/docs/inspector
launching child
Starting inspector on 127.0.0.1:9229 failed: address already in use
in the parent: 15237

Basically it seems the child tries to bind to the same port as the parent.

@blakeembrey
Copy link
Member

That's interesting, thanks! It seems to be a tiny bit closer but not enough. Final test, can you try with node -r ts-node/register --inspect index.ts? If that works, I might refactor how ts-node is spawning the child node process - without any investigation it appears node.js might be doing some internal management for --inspect child spawning and changing the executable is breaking that management.

@AndrewJDR
Copy link
Author

@blakeembrey Yep, node -r ts-node/register --inspect index.ts works! Both ports are bound just fine using that command.

@blakeembrey
Copy link
Member

Awesome news! I'm not sure when I'll get around to hitting this, but I have a decent idea of how I may approach fixing it now. Basically, I'm going to need to refactor to use that command style of spawned command instead and remove the current approach used except in the case of opening the REPL. Thanks for your debugging of the issue 😄

@stelcheck
Copy link
Contributor

FWIW #476 would allow you to start inspect only on the child process.

Indeed, Node itself does quite a few things for managing ports, and that behavior might be a tad bit different depending on the version of Node you are using. The only safe and sound thing to do would either to capture flags in a non-node process, pass them to bin.ts and let it figure out how to set flags on _bin.ts, or getting rid of the child process tree overall (which would actually solve quite a few problem from what I can see).

@ORESoftware
Copy link

ORESoftware commented Dec 10, 2017

This issue is related #471

I get the same error (address already in use)

@blakeembrey
Copy link
Member

I would love to kill the process tree at this point. The main reason it's required is actually for REPL support, but I think it'd be good and/or fine to move toward ts-node for REPL only and node -r ts-node for everything else, or at the very least detect if it looks like REPL or regular usage and switch behaviour to prefix --require ts-node/register.

@stelcheck
Copy link
Contributor

Oh, I was under the impression it was because of v8 flags and general flag parsing/mangling?

@blakeembrey
Copy link
Member

blakeembrey commented Dec 12, 2017

You're right, we do need to proxy through the flags if we expose an executable ts-node. However, I think it could be done by instead prefixing -r ts-node/register before spawning the child (it seems it would fix this issue). Even better might be just to kill this feature to recommend node -r ts-node/register though - I'm not sure how many people prefer ts-node script.ts over node -r ts-node/register script.ts. I'd guess it's non-trivial, but I think it's a more stable pattern with no need to mess with child processes.

@stelcheck
Copy link
Contributor

#499 Would solve this issue.

@blakeembrey
Copy link
Member

The ts-node subprocess has been killed with #536.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug research Needs design work, investigation, or prototyping. Implementation uncertain. you can do this Good candidate for a pull request.
Projects
None yet
Development

No branches or pull requests

4 participants