inspector: split HTTP/WS server from the inspector #9630

eugeneo · 2016-11-16T01:47:39Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

inspector: HTTP server code was reworked and moved to a new file. No impact if the inspector is not started.

Description of change

Both our team experiments and some embedder request indicate a potential
in implementing alternative transport for inspector - e.g. IPC pipes or
custom embedder APIs. This change moves all HTTP specific code into a
separate class and is a first attempt at defining a boundary between the
inspector agent and transport. This API will be refined as new
transports are implemented.
Note that even without considering alternative transports, this change
enables better testing of the HTTP server (Valgrind it possible to identify
and fix some existing memory leaks).

CC: @ofrobots

bnoordhuis · 2016-11-16T08:55:01Z

Related test failures on the OS X buildbot: https://ci.nodejs.org/job/node-test-commit-osx/6038/nodes=osx1010/console - some inspector cctests are failing.

eugeneo · 2016-11-17T00:30:48Z

I fixed the issues CI identified. New CI run (https://ci.nodejs.org/job/node-test-pull-request/4863/) seems to pass on all platforms, except ARM. Failures there seem to have to do with infrastructure failures unrelated to this PR.

bnoordhuis · 2016-11-17T20:23:56Z

node.gyp

@@ -865,10 +867,11 @@
    {
      'target_name': 'cctest',
      'type': 'executable',
-      'dependencies': [ 'deps/gtest/gtest.gyp:gtest' ],
+      'dependencies': ['deps/gtest/gtest.gyp:gtest' ],


Unnecessary change.

bnoordhuis · 2016-11-17T20:25:15Z

src/inspector_agent.cc

+                         const std::string& script_name, bool wait);
+  InspectorSession* StartSession(const std::string& id) override;
+  void MessageReceived(InspectorSession* session,
+                               const std::string& message) override;


Alignment is off here.

bnoordhuis · 2016-11-17T20:26:36Z

src/inspector_agent.cc

+  void EndSession(InspectorSession* session) override;
+  const std::vector<std::string> GetTargetIds() override;
+  const std::string GetTargetTitle(const std::string& id) override;
+  const std::string GetTargetUrl(const std::string& id) override;


Returning const values (not references) is kind of odd. Doesn't g++ complain about that?

It did not complain (neither did clang/Mac), I removed the const none the less.

bnoordhuis · 2016-11-17T20:28:32Z

src/inspector_agent.cc

 bool AgentImpl::IsStarted() {
  return !!platform_;
 }

 void AgentImpl::WaitForDisconnect() {
  shutting_down_ = true;
+  Write(0, StringView());


Why is this necessary? An explaining comment would be good.

This is a temporary "command" to stop accepting new connections. In next pull request there will be a better API with meaningful requests as opposed to magical strings...

bnoordhuis · 2016-11-17T20:31:10Z

src/inspector_agent.cc

-  }
-  if (err != 0) {
+  InspectorAgentDelegate delegate(this, script_path, script_name_, wait_);
+  delegate_ = &delegate;


You don't seem to reset delegate_ to nullptr anywhere.

bnoordhuis · 2016-11-17T20:32:31Z

src/inspector_agent.cc

    uv_loop_close(&child_loop_);
    uv_sem_post(&start_sem_);
    return;
  }
-  PrintDebuggerReadyMessage(port_, id_);
+  server_ = &server;


Ditto for server_, it looks like.

bnoordhuis · 2016-11-17T20:35:09Z

src/inspector_agent.h

 #if !HAVE_INSPECTOR
 #error("This header can only be used when inspector is enabled")
 #endif

+


Unnecessary whitespace change.

bnoordhuis · 2016-11-17T20:37:08Z

src/inspector_agent.h

@@ -37,6 +40,10 @@ class Agent {
  bool IsConnected();
  void WaitForDisconnect();

+  void PostMessage(char* data, size_t len);
+  void StartSession();
+  void EndSession();


These methods don't seem to be used or defined anywhere.

Removed, at some points was considering a slightly different API.

bnoordhuis · 2016-11-17T20:37:57Z

src/inspector_socket_server.cc

+  fprintf(stderr,
+    "Debugger listening on port %d.\n"
+    "Warning: This is an experimental feature and could change at any time.\n",
+    port);


Can you line up the arguments?

bnoordhuis · 2016-11-17T20:52:07Z

src/inspector_socket_server.cc

+  }
+}
+
+std::string GetProcessTitle() {


Seems to be unused.

Yes, this ultimately remained in inspector_agent.cc...

eugeneo · 2016-11-17T23:01:37Z

@bnoordhuis Thank you for reviewing it. I addressed your comments (and also did another pass through function names and such). Please take another look.

eugeneo · 2016-11-22T02:04:12Z

I am looking to rework this quite a bit, to define an easier to use API. Please do not review it for now.

eugeneo · 2016-11-22T22:33:36Z

PR had been reworked, please review. I replaced the Session object with an integer session_id, it simplified the API.

bnoordhuis · 2016-11-24T13:06:43Z

src/inspector_socket_server.cc

+    auto found = std::find(callbacks_.begin(), callbacks_.end(), callback);
+    if (found == callbacks_.end()) {
+      callbacks_.push_back(callback);
+    }


Not at all critical but doesn't it make more sense to use a std::set?

bnoordhuis · 2016-11-24T13:09:14Z

src/inspector_socket_server.cc

+                            const uv_buf_t* buf);
+  static void ReleaseMemory(InspectorSocket* socket, int code) {
+    delete SocketSession::From(socket);
+  }


Unused, it seems?

Removed, thanks.

bnoordhuis · 2016-11-24T13:21:40Z

src/inspector_socket_server.cc

+}
+
+bool InspectorSocketServer::TargetExists(const std::string& id) {
+  const std::vector<std::string>& targetIds = delegate_->GetTargetIds();


bnoordhuis · 2016-11-24T13:34:57Z

test/cctest/test_inspector_socket_server.cc

+    }
+    EXPECT_EQ(0, err);
+  }
+};


Can you share this code with test_inspector_socket.cc?

That test is due for a more comprehensive cleanup so I will do it then (this will require adding new files, etc).

bnoordhuis · 2016-11-24T13:35:10Z

test/cctest/test_inspector_socket_server.cc

+  }
+
+  bool StartSession(int session_id,
+                                 const std::string& target_id) override {


Funny alignment.

bnoordhuis · 2016-11-24T13:36:20Z

test/cctest/test_inspector_socket_server.cc

+    uv_ip4_addr(host.c_str(), PORT, &addr);
+    int err = uv_tcp_connect(&connect_, &socket_,
+                            reinterpret_cast<const sockaddr*>(&addr),
+                            Connected_);


Indentation is off.

bnoordhuis · 2016-11-24T13:36:35Z

test/cctest/test_inspector_socket_server.cc

+    uv_ip4_addr(host.c_str(), PORT, &addr);
+    int err = uv_tcp_connect(&connect_, &socket_,
+                            reinterpret_cast<const sockaddr*>(&addr),
+                            ConnectionMustFail_);


bnoordhuis · 2016-11-24T13:37:50Z

test/cctest/test_inspector_socket_server.cc

+      wrapper->eof_ = true;
+    } else {
+      wrapper->contents_.insert(wrapper->contents_.end(), buf->base,
+                               buf->base + read);


Indentation is off.

bnoordhuis · 2016-11-24T13:38:41Z

test/cctest/test_inspector_socket_server.cc

+
+class ServerHolder {
+ public:
+  template<typename Delegate>


Teeny tiny nit: we usually put a space before the <.

bnoordhuis · 2016-11-24T13:40:48Z

test/cctest/test_inspector_socket_server.cc

+  socket1.TestHttpRequest("/json/list", "[ ]");
+  socket1.Close();
+  uv_run(&loop, UV_RUN_DEFAULT);
+}


Can you fix the long lines in this file?

eugeneo · 2016-11-28T19:51:17Z

@bnoordhuis Thank you for the review. I uploaded updated commit, please take another look.

eugeneo · 2016-12-02T17:51:52Z

@bnoordhuis I've updated the change, fixing the cpplint comment on the new test.

eugeneo · 2016-12-02T19:17:55Z

I see a FreeBSD test failure that does not seem to be connected to this change.

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2016-12-05T12:51:01Z

src/inspector_socket_server.cc

+
+// static
+void InspectorSocketServer::ServerClosedCallback(uv_handle_t* server) {
+  InspectorSocketServer* sserver = InspectorSocketServer::From(server);


Suggestion: s/sserver/socket_server/ - the difference between server and sserver is very subtle.

bnoordhuis · 2016-12-05T12:52:13Z

test/cctest/test_inspector_socket_server.cc

+#include <sstream>
+
+using InspectorSocketServer = node::inspector::InspectorSocketServer;
+using SocketServerDelegate = node::inspector::SocketServerDelegate;


Isn't the linter complaining about a using directive outside a namespace?

I moved the directives inside the namespace. Linter was fine - don't know if it is because of its settings.

eugeneo · 2016-12-06T22:26:41Z

Looks like https://ci.nodejs.org/job/node-test-pull-request/5271/ is all green but some glitch is preventing thew results to be reported back to GitHuib.

eugeneo · 2016-12-06T22:50:32Z

Landed as 42da740

MylesBorins · 2016-12-20T20:56:37Z

@Fishrock123 this is another example of something that likely should not have gotten the dont-land label

MylesBorins · 2016-12-21T19:27:54Z

@eugeneo should this be backported to v6 or is this an example of something that requires the latest version of inspector api?

eugeneo · 2016-12-21T20:08:06Z

This is a refactoring and does not require a new V8 inspector. I am not sure if it is needed for v6 (I'm not familiar with the policies) as main goal is to enable development of new features.

Both our team experiments and some embedder request indicate a potential in implementing alternative transport for inspector - e.g. IPC pipes or custom embedder APIs. This change moves all HTTP specific code into a separate class and is a first attempt at defining a boundary between the inspector agent and transport. This API will be refined as new transports are implemented. Note that even without considering alternative transports, this change enables better testing of the HTTP server (Valgrind made it possible to identify and fix some existing memory leaks). PR-URL: #9630 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

gibfahn · 2017-03-11T01:31:18Z

If this is backported it should be backported with #10657

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Nov 16, 2016

eugeneo mentioned this pull request Nov 16, 2016

Feature request: API for debugging node child processes #9621

Closed

nodejs-github-bot added dont-land-on-v7.x labels Nov 16, 2016

bnoordhuis requested changes Nov 17, 2016

View reviewed changes

eugeneo mentioned this pull request Nov 17, 2016

Debugger doesn't work for processes which fork other processes #9435

Closed

bnoordhuis requested changes Nov 24, 2016

View reviewed changes

bnoordhuis mentioned this pull request Nov 24, 2016

test: run cpplint on files in test/cctest #9787

Merged

bnoordhuis approved these changes Dec 5, 2016

View reviewed changes

jasnell approved these changes Dec 5, 2016

View reviewed changes

bnoordhuis approved these changes Dec 6, 2016

View reviewed changes

eugeneo closed this Dec 6, 2016

eugeneo merged commit 42da740 into nodejs:master Dec 6, 2016

eugeneo deleted the socket_server branch December 6, 2016 22:50

MylesBorins removed the dont-land-on-v7.x label Dec 20, 2016

MylesBorins added dont-land-on-v4.x labels Dec 20, 2016

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

gibfahn mentioned this pull request Mar 11, 2017

build: don't bundle deps/zlib if --shared-zlib set #10657

Merged

2 tasks

MylesBorins added the backport-requested-v6.x label Apr 17, 2017

inspector: split HTTP/WS server from the inspector #9630

inspector: split HTTP/WS server from the inspector #9630

Conversation

eugeneo commented Nov 16, 2016

Checklist

Affected core subsystem(s)

Description of change

bnoordhuis commented Nov 16, 2016

eugeneo commented Nov 17, 2016

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

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 Nov 17, 2016 • edited Loading

eugeneo commented Nov 22, 2016

eugeneo commented Nov 22, 2016

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

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 Nov 28, 2016

eugeneo commented Dec 2, 2016

eugeneo commented Dec 2, 2016

bnoordhuis left a 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 Dec 6, 2016

eugeneo commented Dec 6, 2016

MylesBorins commented Dec 20, 2016

MylesBorins commented Dec 21, 2016

eugeneo commented Dec 21, 2016

gibfahn commented Mar 11, 2017

eugeneo commented Nov 17, 2016 •

edited

Loading