inspector: perform DNS lookup for host #13478

eugeneo · 2017-06-05T17:22:58Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

inspector: performs a sync DNS lookup when the server is starting.

refack · 2017-06-05T17:51:24Z

Code LGTM, the question is does it make sense? Do people need a DNS lookup? Won't it be simpler to check for host_.c_str()[0] == '[' and call uv_ip6_addr?

refack

Needs new tests in test/cctest/test_inspector_socket_server.cc

sam-github

Looks good, but the URL formatting isn't valid, will need to have the [...] placed around the address so the output URLs are valid:

% ./node --inspect=::1:9876
Debugger listening on ws://::1:9876/6fcffeb2-55bf-438b-b886-582a2d517513
For help see https://nodejs.org/en/docs/inspector
> %                                                                                    core/node ((a392ece...) $) % ./node --inspect=[::1]:9876
Debugger listening on ws://::1:9876/1fa763a7-4ca3-4040-a666-6eb8d3b268f9

sam-github · 2017-06-05T17:59:15Z

src/inspector_socket_server.cc

+  int err = uv_getaddrinfo(loop_, &req, nullptr, host_.c_str(),
+                           std::to_string(port_).c_str(), nullptr);
+  if (err < 0) {
+    fprintf(out_, "%s is not a valid host name\n", host_.c_str());


Is there a more specific error message that can be reported? Is invalid host name the only reason for getaddrinfo to fail?

eugeneo · 2017-06-05T19:00:33Z

Code LGTM, the question is does it make sense? Do people need a DNS lookup? Won't it be simpler to check for host_.c_str()[0] == '[' and call uv_ip6_addr?

One stupid thing I noticed was any symbolic name (non-existing, localhost) would always bind to *. With this lookup invalid names (e.g. with typos) will error out, localhost binds to 127.0.0.1 (e.g. no outside access).

v8.0.0:

$ node --inspect=google.com
Debugger listening on ws://google.com:9229/bd2d9e4f-b9b5-4503-b38e-df62a8082fbd

(actually listening *:9229)

This patch:

$ ./node --inspect=google.com
Starting inspector on google.com:9229 failed: address not available

sam-github · 2017-06-05T19:26:39Z

Yes, definitely works better now.

refack · 2017-06-05T22:41:18Z

One stupid thing I noticed was any symbolic name (non-existing, localhost) would always bind to *. With this lookup invalid names (e.g. with typos) will error out, localhost binds to 127.0.0.1 (e.g. no outside access).

Ack.
_{Still IMHO needs tests}

eugeneo · 2017-06-06T00:22:54Z

@refack Tests added. Will see if they make it through the CI :)

eugeneo · 2017-06-06T00:23:15Z

@sam-github Fixed the URL

eugeneo · 2017-06-06T00:59:05Z

Please, don't review for now - I will upload a different version tomorrow.

eugeneo · 2017-06-06T21:57:18Z

Ready for review. CI: https://ci.nodejs.org/job/node-test-pull-request/8520/

refack · 2017-06-06T22:11:07Z

test/cctest/test_inspector_socket_server.cc

@@ -26,7 +26,7 @@ static const char WS_HANDSHAKE_RESPONSE[] =
  {                                                                            \
    Timeout timeout(&loop);                                                    \
    while ((condition) && !timeout.timed_out) {                                \
-      uv_run(&loop, UV_RUN_NOWAIT);                                            \
+      uv_run(&loop, UV_RUN_ONCE);                                              \


How does this change the current tests?

It does not. Simply results in lower number of loops.

bnoordhuis · 2017-06-07T10:25:11Z

src/inspector_socket_server.cc

@@ -376,11 +385,25 @@ void InspectorSocketServer::SendListResponse(InspectorSocket* socket) {

 bool InspectorSocketServer::Start() {
  CHECK_EQ(state_, ServerState::kNew);
-  sockaddr_in addr;
+  struct addrinfo hints;
+  memset(&hints, 0, sizeof(struct addrinfo));


sizeof(hints) or just struct addrinfo hints().

I will admit I don't quite understand why you pass the port number if you aren't interested in service name lookups (AI_NUMERICSERV.)

In my opinion, made the code slightly easier - prefills the sockaddr structures.

bnoordhuis · 2017-06-07T10:37:45Z

src/inspector_socket_server.cc

-  uv_ip4_addr(host_.c_str(), port_, &addr);
-  int err = uv_tcp_bind(&server_,
-                        reinterpret_cast<const struct sockaddr*>(&addr), 0);
+  err = uv_tcp_bind(&server_, req.addrinfo->ai_addr, 0);


Only binds to the first address. Acceptable for now but sooner or later someone is going to file a bug about that.

Now this class works with a vector of sockets. This should also help if there's ever a need to support listening on multiple ports (e.g. someone mentioned before something about changing the port at the runtime).

One issue this implementation is that different sockets will bind to different ports if requested port is 0 - while only first socket port is surfaced in API and console messages. I did not want to further complicate this implementation at this time, but an API that provides list of addresses and ports can be created later.

One issue this implementation is that different sockets will bind to different ports if requested port is 0

That seems reasonable to me.

bnoordhuis · 2017-06-07T10:39:39Z

test/cctest/test_inspector_socket_server.cc

+bool has_ipv6_address() {
+  uv_interface_address_s* addresses;
+  int address_count;
+  int err = uv_interface_addresses(&addresses, &address_count);


Needs a matching call to uv_free_interface_addresses().

done. I'm bit fuzzy on this - but should uv_free_* be called if respective call (in this case to uv_interface_addresses) failed?

You normally only call it on success but it's safe to call on error if you initialize addresses and address_count to nullptr / zero.

eugeneo · 2017-06-07T22:02:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8543/

eugeneo · 2017-06-07T23:54:47Z

(Please do not review for now, fixing)

eugeneo · 2017-06-08T18:00:06Z

I ended up moving more code around then I originally intended. My goal was to better separate responsibilities between server, server socket and session objects.

Please review.

CI: https://ci.nodejs.org/job/node-test-pull-request/8557/

refack · 2017-06-08T18:44:58Z

src/inspector_socket_server.cc

-    PrintDebuggerReadyMessage(host_, port_, delegate_->GetTargetIds(), out_);
+  for (addrinfo* address = req.addrinfo; address != nullptr;
+       address = address->ai_next) {
+    err = ServerSocket::Listen(this, address->ai_addr, loop_);


If I understand this correctly for localhost we'll probably get ['127.0.0.1', '[::1]'] and try to listen to them all?
But print only the first success?

We are not printing out resolved address. Instead localhost:port will be printed. Only one port will be outputted - which is only a problem when port set to 0.

I can live with that

refack · 2017-06-08T19:12:25Z

src/inspector_socket_server.cc

-  char buf[1024];
-  snprintf(buf, sizeof(buf), "%s:%d/%s", host.c_str(), port, id.c_str());
-  return buf;
+  // Host is valid (socket was bound) so colon means it's a v6 IP address


Should we merge this with the code in inspector_agent.cc@node::inspector::URL() after the if?
/cc @sam-github

I was not aware of that code. I will share the code for generating the URL. It will still be slightly different - e.g. in json/list response it echoes IP address the client used to access the server while other places will print the host name specified from the command line (--inspect=0.0.0.0 will respond with 127.0.0.1 or external IP depending on the client but print out 0.0.0.0).

It's new code (less than a week old)

I found it. Moved the function to inspector_io.{h,cc}. Since the _server class does not depend on anything in the Node (so it is easier to test it) I copied a declaration to the inspector_socket_server.cc.

refack · 2017-06-08T19:23:44Z

a few questions:

I assume most of the refactoring is to enable multiple listening sockets?
Should we update the docs?
Should we add JS tests?
Should we test that if IPv4 & IPv6 are enabled calling with localhost binds to both?

eugeneo · 2017-06-08T20:25:00Z

I assume most of the refactoring is to enable multiple listening sockets?

Yes.

Should we update the docs?

Don't think so. This should not be user-observable - we never claimed IPv6 was supported (nor did we explicitly call it unsupported). Multiple interfaces is also a bug fix - there is still no way to start listening on different port numbers and such.

Should we add JS tests?

IMHO, this change has no observable impact on wire clients. IPv6 on the low level is tested in cctest.

Should we test that if IPv4 & IPv6 are enabled calling with localhost binds to both?

This is likely platform and configuration specific. There may be setups without IPv6 interface (or even without IPv4), localhost may not resolve to all of them, etc.

eugeneo · 2017-06-08T20:25:41Z

I looked at CI failures and they do not seem to be caused by this code as inspector server is not started there.

refack · 2017-06-08T20:27:15Z

Should we test that if IPv4 & IPv6 are enabled calling with localhost binds to both?

This is likely platform and configuration specific. There may be setups without IPv6 interface (or even without IPv4), localhost may not resolve to all of them, etc.

Ack (on my machine 'localhost' resolves only to IPv4)

refack · 2017-06-09T02:49:21Z

src/inspector_agent.cc

-  url += "/";
-  url += ids[0];
-
+  std::string url = FormatWsAddress(io->host(), io->port(), ids[0], true);


Solves the IPv6 URL bug 👍

refack · 2017-06-09T02:50:53Z

src/inspector_socket_server.cc

@@ -12,6 +12,10 @@
 namespace node {
 namespace inspector {

+std::string FormatWsAddress(const std::string& host, int port,


Circular dependency thing?
You should put a comment...

Added a comment.

TimothyGu · 2017-06-09T06:28:12Z

DNS lookup aside, the entire host parsing process looks fairly complicated. I understand it would be a separate venture from this PR, but is there any value in sharing the WHATWG URL parser's host parser or host state (includes port parsing), which distinguishes between domains, IPv4, and IPv6 addresses in a standard-compliant manner?

eugeneo · 2017-06-09T20:48:43Z

@TimothyGu Thank you. I will look into it - but not for this PR (there's no address parsing/validation in this PR - it simply passes the string to DNS).

refack

💯

eugeneo · 2017-06-16T15:39:27Z

Thank you for the reviews. New CI: https://ci.nodejs.org/job/node-test-pull-request/8694/

bnoordhuis · 2017-06-16T15:47:13Z

src/inspector_socket_server.cc

+
+ private:
+  explicit ServerSocket(InspectorSocketServer* server)
+                        : tcp_socket_(uv_tcp_t()), server_(server), port_(-1) {}


Just 4-space indent relative to the previous line is enough.

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

eugeneo · 2017-06-16T16:50:50Z

CI have not indicated any relevant failures. Landed as 6e2c29b

refack · 2017-06-16T16:55:33Z

I think it's a good fix, but I don't yet trust our test suite coverage, can we let it cook for a while before porting to v8.x (going on gut feeling)?

eugeneo · 2017-06-16T16:57:43Z

I don't think it is pressing enough. I added labels - can remove (or manually backport) if need be.

addaleax · 2017-06-16T17:01:23Z

@refack I would not let it wait. For one, to some degree having the latest bugfixes and features is what Current releases are there for; and then, as I’ve mentioned before it would likely be a good idea to do a RC process for 8.2.0, because that’s going to come with V8 5.9 and we’ve handled V8 upgrades the same way in the past.

If you feel really strongly, you can re-add the labels, but for now I removed them again.

(edit: Also, I would apply dont-land-on-v6.x only if it really shouldn’t land; otherwise we have baking-for-lts for that)

refack · 2017-06-16T17:08:26Z

@refack I would not let it wait. For one, to some degree having the latest bugfixes and features is what Current releases are there for; and then, as I’ve mentioned before it would likely be a good idea to do a RC process for 8.2.0, because that’s going to come with V8 5.9 and we’ve handled V8 upgrades the same way in the past.

If you feel really strongly, you can re-add the labels, but for now I removed them again.

Ack.

Ping @jkrems, @joshgav, @roblourens, @billti, @prigara if you have external tests you could run on tomorrow's nightly, that would be great.
tl;dr we now DNS resolve the host address for --inspect=localhost:9229 would be happy to know there are no regressions in 3rd party tools
[edit]
@ulitink, @segrey

mbraude · 2017-06-16T21:48:12Z

@refack We'll do a test pass with the Node Tools for Visual Studio over the weekend and I'll get back to you on Monday. FYI, I am setting up nightly automation runs on the latest nightly node builds that will give us early warning when we're broken.

refack · 2017-06-16T22:14:44Z

@mbraude that is great!

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

jkrems · 2017-06-18T02:35:53Z

Thanks for the ping! Ran node-inspect CI against the latest nightly and everything's still nice and green.

segrey · 2017-06-19T13:24:04Z

@refack Thanks, it doesn't break IntelliJ.

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2017-08-14T19:13:36Z

Could this be backported? v6.x inspector is pretty far behind

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Jun 5, 2017

refack approved these changes Jun 5, 2017

View reviewed changes

refack suggested changes Jun 5, 2017

View reviewed changes

sam-github suggested changes Jun 5, 2017

View reviewed changes

sam-github approved these changes Jun 6, 2017

View reviewed changes

refack approved these changes Jun 6, 2017

View reviewed changes

refack mentioned this pull request Jun 6, 2017

src: check whether inspector is doing io #13504

Merged

3 tasks

bnoordhuis requested changes Jun 7, 2017

View reviewed changes

refack approved these changes Jun 8, 2017

View reviewed changes

refack approved these changes Jun 9, 2017

View reviewed changes

bnoordhuis approved these changes Jun 16, 2017

View reviewed changes

inspector: perform DNS lookup for host

6e2c29b

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

eugeneo merged commit 6e2c29b into nodejs:master Jun 16, 2017

eugeneo deleted the ipv6 branch June 16, 2017 16:51

eugeneo added dont-land-on-v6.x labels Jun 16, 2017

addaleax removed dont-land-on-v6.x labels Jun 16, 2017

addaleax pushed a commit that referenced this pull request Jun 17, 2017

inspector: perform DNS lookup for host

f4f7996

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

refack mentioned this pull request Jun 19, 2017

known limitation: when running with --inspect=localhost:0 only one port is printed #13772

Closed

addaleax pushed a commit that referenced this pull request Jun 21, 2017

inspector: perform DNS lookup for host

c4529c6

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax pushed a commit that referenced this pull request Jun 24, 2017

inspector: perform DNS lookup for host

91b1144

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

rvagg pushed a commit that referenced this pull request Jun 29, 2017

inspector: perform DNS lookup for host

971009f

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax pushed a commit that referenced this pull request Jul 11, 2017

inspector: perform DNS lookup for host

a45792a

PR-URL: #13478 Fixes: #13477 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins added the dont-land-on-v6.x label Nov 14, 2017

inspector: perform DNS lookup for host #13478

inspector: perform DNS lookup for host #13478

Conversation

eugeneo commented Jun 5, 2017

Checklist

Affected core subsystem(s)

refack commented Jun 5, 2017

refack left a comment

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Jun 5, 2017 • edited Loading

sam-github commented Jun 5, 2017

refack commented Jun 5, 2017

eugeneo commented Jun 6, 2017

eugeneo commented Jun 6, 2017

eugeneo commented Jun 6, 2017

eugeneo commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Jun 7, 2017

eugeneo commented Jun 7, 2017

eugeneo commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 8, 2017

eugeneo commented Jun 8, 2017

eugeneo commented Jun 8, 2017

refack commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Jun 9, 2017

eugeneo commented Jun 9, 2017

refack left a comment

Choose a reason for hiding this comment

eugeneo commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Jun 16, 2017

refack commented Jun 16, 2017

eugeneo commented Jun 16, 2017

addaleax commented Jun 16, 2017 • edited Loading

refack commented Jun 16, 2017 • edited Loading

mbraude commented Jun 16, 2017

refack commented Jun 16, 2017

jkrems commented Jun 18, 2017

segrey commented Jun 19, 2017

MylesBorins commented Aug 14, 2017

eugeneo commented Jun 5, 2017 •

edited

Loading

addaleax commented Jun 16, 2017 •

edited

Loading

refack commented Jun 16, 2017 •

edited

Loading