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: print all listening addresses #26008

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Feb 8, 2019

Some hostnames have multiple interfaces. Before this commit, the
inspector only printed the first one. Now, it prints them all.

No test. I can't think of a reliable way to test this on the CI matrix.

Fixes: #13772

CI: https://ci.nodejs.org/job/node-test-pull-request/20671/ - test failures possible, some tests probably assume the current output. Everything passes for me locally though.

Some hostnames have multiple interfaces. Before this commit, the
inspector only printed the first one. Now, it prints them all.

No test. I can't think of a reliable way to test this on the CI matrix.

Fixes: nodejs#13772
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Feb 8, 2019
@refack
Copy link
Contributor

refack commented Feb 8, 2019

Locally:

D:\code\4node\gyp>d:\refael\Downloads\node.exe --inspect=localhost:0
Debugger listening on ws://localhost:56121/d4d038da-ff6a-4e0b-ac0c-d5cad82c928a
Debugger listening on ws://localhost:56122/d4d038da-ff6a-4e0b-ac0c-d5cad82c928a
For help, see: https://nodejs.org/en/docs/inspector

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

test failures possible, some tests probably assume the current output. Everything passes for me locally though.

Since nothing failed, we probably should have a test for this.

@bnoordhuis
Copy link
Member Author

we probably should have a test for this

Yes, but how? CI machines may or may not be multi-homed, kind of difficult to test for.

I could write a test that checks with dns.lookup() if a hostname (and: what hostname?) resolves to more than one address but it would be dead code if no CI machine is actually multi-homed.

@bnoordhuis
Copy link
Member Author

The one failure seems to be infrastructural?

08:50:58 + docker exec node-ci-jessie /bin/sh -c 'cd /home/iojs/build/workspace/node-test-binary-arm && . /home/iojs/build/workspace/node-test-binary-arm/node-ci-exec'
08:51:00 OCI runtime exec failed: exec failed: container_linux.go:348: starting container process caused "process_linux.go:90: adding pid 26098 to cgroups caused \"failed to write 26098 to cgroup.procs: write /sys/fs/cgroup/cpu,cpuacct/docker/a4e4fbb3c89a89856f299285777bc58d01f4a0275208f0ff49a4db562fadbf55/cgroup.procs: invalid argument\"": unknown

@refack
Copy link
Contributor

refack commented Feb 10, 2019

I could write a test that checks with dns.lookup() if a hostname (and: what hostname?) resolves to more than one address but it would be dead code if no CI machine is actually multi-homed.

I think that if a machine is both IPv4 & IPv6 enabled in will assign two ports (well at least for windows it will)... And we have those predicates in test/common. Worth a shot.

@bnoordhuis
Copy link
Member Author

I think that if a machine is both IPv4 & IPv6 enabled in will assign two ports (well at least for windows it will)

If you're 100% positive it works that way for Windows I could write that test.

On Unices, localhost is only going to be mapped to two addresses when it has two entries in /etc/hosts but that's uncommon. Most distros call it something like ip6-localhost.

Copy link
Contributor

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

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

I just want to point out that this was not implemented originally because of concerns that some clients parsing the output might be broken due to unexpected second and such message. I strongly believe such clients should be fixed :)

@addaleax
Copy link
Member

@addaleax
Copy link
Member

addaleax commented Mar 1, 2019

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2019
@addaleax
Copy link
Member

addaleax commented Mar 1, 2019

@BridgeAR
Copy link
Member

BridgeAR commented Mar 4, 2019

Landed in df67cd0 🎉

@BridgeAR BridgeAR closed this Mar 4, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 4, 2019
Some hostnames have multiple interfaces. Before this commit, the
inspector only printed the first one. Now, it prints them all.

No test. I can't think of a reliable way to test this on the CI matrix.

PR-URL: nodejs#26008
Fixes: nodejs#13772
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Mar 4, 2019
Some hostnames have multiple interfaces. Before this commit, the
inspector only printed the first one. Now, it prints them all.

No test. I can't think of a reliable way to test this on the CI matrix.

PR-URL: #26008
Fixes: #13772
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
BridgeAR pushed a commit that referenced this pull request Mar 5, 2019
Some hostnames have multiple interfaces. Before this commit, the
inspector only printed the first one. Now, it prints them all.

No test. I can't think of a reliable way to test this on the CI matrix.

PR-URL: #26008
Fixes: #13772
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
Some hostnames have multiple interfaces. Before this commit, the
inspector only printed the first one. Now, it prints them all.

No test. I can't think of a reliable way to test this on the CI matrix.

PR-URL: #26008
Fixes: #13772
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants