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

Increment debug port for each child process. #874

Closed
wants to merge 1 commit into from

Conversation

tcf909
Copy link

@tcf909 tcf909 commented Oct 2, 2018

When trying to debug a sub-command style program I ran into a port conflict with --inspect. This increments the debug port for spawned processes.

@abetomo
Copy link
Collaborator

abetomo commented Oct 21, 2018

CI has failed.
Could you check it.

@shadowspawn
Copy link
Collaborator

The code looks a bit wild on first read and specific to node options. However, the port issue does cause confusion when it occurs so worthy of further consideration. #533 #838

@shadowspawn
Copy link
Collaborator

This looks like might be a solution on node side?

nodejs/node#5025

bind to random port with --debug-port=0

@tcf909
Copy link
Author

tcf909 commented Apr 8, 2019

the debug-port=0 is a good option, but will only support node 8.x later branches.

If this is something that people will want I'll refine it (I have done so on my own project) to also include handling environment variables.

Port conflicts are definitely an issue with the way commander handles sub commands (given the spawn nature of the process).

@tcf909
Copy link
Author

tcf909 commented Apr 8, 2019

@abetomo The CI is failing on syntax not on functionality. The lint seems extremely specific.

@abetomo abetomo requested a review from shadowspawn April 9, 2019 00:35
@shadowspawn
Copy link
Collaborator

I am leaning towards noting the port issue for debugging and including the two available debugging approaches in the README. i.e. debug sub-command directly, or use node support.

I like the idea of suggesting a standard node solution that works for other products too.

LTS maintenance for node 6 ends this month so I don't think node 8 is a problem moving forward.

However, I only just found --debug-port=0 and planning to wait for a while and see if there are comments on that as a solution. I have added it to each of the related issues I am aware of. I have not tried it myself yet.

@shadowspawn
Copy link
Collaborator

And I forgot to say explicitly @tcf909 , thanks for the contributions so far, whatever we decide!

@shadowspawn shadowspawn added the docs README (or other docs) could be improved label May 17, 2019
@jamime
Copy link

jamime commented May 17, 2019

@shadowspawn I would say that using --inspect 0 (which is already supported) is not sustainable for development. Each time you start the process you need to attach to a new port. If we use incrementing ports you'll only need to define a few connections.

@shadowspawn
Copy link
Collaborator

shadowspawn commented May 17, 2019

Note to self: Visual Studio Code detects these patterns:

Whether or not a process is in "debug mode" is determined by analyzing 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).

Also autoAttachChildProcesses: https://code.visualstudio.com/docs/nodejs/nodejs-debugging#_automatically-attach-debugger-to-nodejs-subprocesses

Code for VSCode pattern and port detection:
https://github.com/microsoft/vscode/blob/668068e26f30f1fcfc925579b900b58111b44086/src/vs/code/electron-browser/processExplorer/processExplorerMain.ts#L25

@shadowspawn
Copy link
Collaborator

The nodejs cluster module has a spawn which increments debug port:
https://nodejs.org/docs/latest-v8.x/api/cluster.html

Long thread for node with people having port conflicts with spawn/fork. There is mention of debugging inspectors which can auto-detect child processes in debug mode (terminology?), initially in Chrome dev tools:
nodejs/node#9435

@shadowspawn shadowspawn removed docs README (or other docs) could be improved needs discussion labels May 21, 2019
@shadowspawn
Copy link
Collaborator

After seeing that nodejs cluster does port incrementing, I am more open to that approach.

@tcf909
You have a complex pattern for numeric host ip (?) in this PR. I note that the Visual Studio Code flag check just looks for pure port, and the nodejs documentation says it could be host:port. Not seeing the point in looking for subclass of numeric IP when host could equally be any numeric IP or localhost or a dns name. Is there a reference and/or reason for that complex pattern?

@tcf909
Copy link
Author

tcf909 commented May 23, 2019

No specific reason. We don't accept host names in our code base for listening ports and wanted to make sure valid ips are passed through or caught at the point of identification and thrown (rather than passing down the stack and wondering what is wrong). We found different versions of node handle various scenarios differently when malformed data was passed.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tcf909

  1. I don't see any advantage to strong checking of the ip number, or that port is finite, as by the time we see them they have already been seen and used by node? I would prefer a simpler pattern.
  2. We don't want to increment port number "0" since now know that has its own special behaviour.
  3. Detect when port number not explicitly specified, and increment from the default port.
  4. Lint (as previously noted)

Let us know if you aren't interested in working through these changes. For me, deciding it is reasonable to increment the port was harder than the code! Can be picked up by someone else.

@shadowspawn
Copy link
Collaborator

This is being considered for v3, but will need some work and to pass reviews. I am interesting in having a go (original author is not currently active).

@shadowspawn
Copy link
Collaborator

Closing favour of #991

Thank you for your contributions.

@shadowspawn shadowspawn closed this Jul 7, 2019
@shadowspawn shadowspawn removed this from the v3.0.0 milestone Jul 7, 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

Successfully merging this pull request may close these issues.

4 participants