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

inspector: SIGUSR1 should use next available port if 9922 is already used #16872

Closed
auchenberg opened this issue Nov 7, 2017 · 18 comments

Comments

@auchenberg
Copy link

commented Nov 7, 2017

When having multiple node processes running in parallel, and sending SIGUSR1 to one of them the Inspector server started and binding in port 9222. When sending SIGUSR1 to another Node process, the Inspector server can't be started as port 9222 is already used.

Starting inspector on 127.0.0.1:9229 failed: address already in use

This limits debugging, and sending SIGUSR1 to a process should start the Inspector server on the next available port instead of failing. The used port could be outputted to stdout for detection.

@auchenberg

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

@mike-kaufman

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2017

I think you can use --inspect-port=<port> at startup, but this means you know the available port when process launches. If you did support dynamically assigning a random port, you've introduced a discovery problem; if you want to determine which port is assigned, then you need to parse that process's stdout.

Ideally, if dynamic port assignment on SIGUSR1 was supported, there'd be:

  • a startup flag to specify a range of debug ports to choose from.
  • a startup flag to specify that if the debugger is attached, then the port + pid should be written to some location.

@mscdex mscdex added the inspector label Nov 8, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

@auchenberg Does --inspect-port=0 work for you? You can capture the address and port on stderr.

@auchenberg

This comment has been minimized.

Copy link
Author

commented Nov 16, 2017

@bnoordhuis I wasn't aware that --inspect-port=0 existed for dynamic port assignment. What do you think @mike-kaufman?

@mike-kaufman

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

This would probably work for most peoples' scenarios. For us, it would pose a few problems:

  1. port chosen is not restricted to some range. Our runtime environment needs some network configuration, so knowing the port range up front is key. There may be some workarounds to this on our end that I need to investigate, but these would be significantly more complex.

  2. I'm not keen to scan apps' stdout/stderr for the dynamically assigned port.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

I'm not keen to scan apps' stdout/stderr for the dynamically assigned port.

You'd have to do that with @auchenberg's proposed scheme too, right?

@eugeneo

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

There were some discussions about giving programmatic control over the port selection (e.g. to allow user code/module to register a handler for the debug signal). Would this work for you?

@dhanvikapila

This comment has been minimized.

Copy link

commented Nov 17, 2017

@eugeneo we love the signaling mechanism to put a Node process into inspect mode.

Not sure if this would work, but here is a suggestion -
9229 (default port) continues to stay the default for the first time when anyone signals a node process. In addition to what is done currently, the current HTTP server will also provide a special endpoint to query ( say http://[host]:9229/json/listall) which anyone could query to get all inspector endpoints.

On the signalling side, whenever a process is signaled - SIGUSR1, Node would pick a new unused port and use that.

I guess node would have to find a way to fig. out random ports that are not being used. Doesn't it already do that today to check if 9229 is available?

@mike-kaufman

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

There were some discussions about giving programmatic control over the port selection (e.g. to allow user code/module to register a handler for the debug signal). Would this work for you?

Not really, probably "too invasive" for our scenarios. So, we may have some work-arounds here that will be good enough. That said, if node runtime wanted to support dynamic port assignment, limited to a port range, and some reliable discovery mechanism, there'd be some switches like:

  • --inspect-port-range=1000:2000 // indicates the port range the debugger should choose from
  • --inspect-port-notification-directory=/some/path/ // indicates a path that node could write a file with name like pid-port.txt.

Then, a SIGUSR1 would pick one of those ports, drop a file in that location, and we can scan that directory to know which pid is listening on which port.

@auchenberg

This comment has been minimized.

Copy link
Author

commented Nov 17, 2017

@bnoordhuis @mike-kaufman I understand the need for having a consistent discovery mechanism, and listening to strout on MacOS + Windows seems to be challenging, but I'd be hesitant of having Node writing to disk as assumes the Node process has I/O access, permissions, etc.

What are the alternatives? Emit port on a predefined POSIX signal?

@eugeneo

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

Another thing that is in progress is subtargets - #16627

I think it should be possible for VSCode to run a "parrent" node and setup an inspector on it. Then it should be possible to send requests to start child nodes (once process.fork is supported for this scenario).

@eugeneo

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

@dhanvikapila I do not see how this could be implemented, 9229 might be taken by "unrelated" Node process (e.g. ran by a different user) or some non-Node process.

@auchenberg

What are the alternatives? Emit port on a predefined POSIX signal?

There is a requirement for inspector-over-pipe. It would likely look something like -inspector-pipe=/path/to/pipe.

@mike-kaufman
It should be possible to preallocate a port number if you have control over command-line options.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

Alternatively, if you're on a UNIX-like system, it's not too onerous to find out what ports the target process was listening on before and after signalling it. Worst case you shell out to lsof -p <pid>.

@dhanvikapila

This comment has been minimized.

Copy link

commented Nov 17, 2017

@eugeneo if my understanding is correct, event today if 9229 is unavailable, debugging will not work anyways since Node wont put the target process into inspect mode. Node cannot change this behavior without breaking backwards compatibility. I'm not claiming my proposal would be perfect but it provides a way forward when 9229 is available.

@bnoordhuis lsof -p <pid> might work but we are trying to avoid parsing command output.

@mike-kaufman

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

It should be possible to preallocate a port number if you have control over command-line options.

@eugeneo - that almost works. Issue here is if that pre-identified port is in-use when SIGUSR1 is received. Maybe we can get away with it, but my concern is at high enough scale it will show up.

Alternatively, if you're on a UNIX-like system, it's not too onerous to find out what ports the target process was listening on before and after signalling it. Worst case you shell out to

@bnoordhuis - yeah, that almost works too. Issue here is if node process opens an additional port between your SIGUSR1 and your lsof .... Maybe so unlikely that it will never occur, but again, at high enough scale, it will impact some people.

but I'd be hesitant of having Node writing to disk as assumes the Node process has I/O access, permissions, etc.

@auchenberg - I think it's safe to assume that if you specify a directory on command-line, there's perms sufficient to write to it.

@gireeshpunathil

This comment has been minimized.

Copy link
Member

commented May 16, 2018

looks like the discussion has identified a couple of viable workarounds for the reported problem (pre-allocating ports, discovery using lsof). Are we good to close this?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Since this issue hasn't seen any action in over a year I'm going to go ahead and close it out. Please reopen if necessary.

@bnoordhuis bnoordhuis closed this Feb 8, 2019

@june07

This comment has been minimized.

Copy link

commented Aug 20, 2019

Ran across this issue again myself, trying to have SIGUSR1 open the inspector on child processes when the default 9229 was already in use by parents and/or other some other process. This is my solution:

Pretty simple npm package that you can simply require('fiveo'), start your new process with INSPECT=9230 (or whatever [host:port]), done!

You can also use it without the INSPECT env and it will default to localhost:9229.

https://www.npmjs.com/package/fiveo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.