inspector: split main thread interface from transport #21182

eugeneo · 2018-06-06T23:20:39Z

Workers debugging will require interfacing between the "main" inspector
and per-worker isolate inspectors. This is consistent with what WS
interface does. This change is a refactoring change and does not change
the functionality.

Checklist

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

eugeneo · 2018-06-06T23:25:12Z

src/signal_wrap.cc

@@ -90,7 +90,7 @@ class SignalWrap : public HandleWrap {
 #if defined(__POSIX__) && HAVE_INSPECTOR
    if (signum == SIGPROF) {
      Environment* env = Environment::GetCurrent(args);
-      if (env->inspector_agent()->IsStarted()) {
+      if (env->inspector_agent()->IsListening()) {


Here I am unsure about the semantics. Do we want this warning printed out when Inspector is installed on the isolate (which is currently "always", because of JS API and _debugProcess support) or when the WS server is running.

eugeneo · 2018-06-06T23:27:02Z

src/inspector_agent.cc

-  client_ =
-      std::shared_ptr<NodeInspectorClient>(
-          new NodeInspectorClient(parent_env_, platform));
+  client_ = std::make_shared<NodeInspectorClient>(parent_env_, platform);


My understanding is that make_shared should be fine as it is a part of C++11 (make_unique was introduced shortly after C++11)

eugeneo · 2018-06-06T23:28:08Z

src/inspector_agent.h

 };

 class Agent {
 public:
  explicit Agent(node::Environment* env);
  ~Agent();

-  // Create client_, may create io_ if option enabled
+  // // Create client_, may create io_ if option enabled


Fixed locally, will upload when addressing future review comments.

eugeneo · 2018-06-06T23:30:20Z

src/inspector_io.cc

  int id_;
 };

 // Passed to InspectorSocketServer to handle WS inspector protocol events,
 // mostly session start, message received, and session end.
 class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
 public:
-  InspectorIoDelegate(InspectorIo* io, const std::string& target_id,
+  InspectorIoDelegate(std::shared_ptr<RequestQueue> queue,
+                      std::weak_ptr<MainThreadInterface> interface,


Looks like this is a first usage of std::weak_ptr in Node codebase. Standard seems to require it to be thread-safe. I would like to confirm with people familiar with less wide-spread platforms it is ok to use it here.

eugeneo · 2018-06-06T23:31:50Z

test/parallel/test-warn-sigprof.js

@@ -1,3 +1,4 @@
+// Flags: --inspect=0


This change is because I assume handling SIGPROF should only produce the warning when the WS server is running

eugeneo · 2018-06-06T23:33:13Z

test/cctest/test_inspector_socket_server.cc

@@ -84,6 +83,7 @@ class InspectorSocketServerTest : public ::testing::Test {
  }
 };

+


Will remove this delta in next upload.

addaleax

Nice work! I’ll need more time to look at this in detail, but I’ll try to do it if I find that time.

addaleax · 2018-06-07T16:19:47Z

node.gyp

        'src/connection_wrap.h',
        'src/connect_wrap.h',
        'src/debug_utils.h',
        'src/env.h',
        'src/env-inl.h',
+        'src/exceptions.h',


These two headers don’t exist anymore, I think we want to remove them from this PR?

addaleax · 2018-06-07T16:20:19Z

src/inspector/main_thread_interface.cc

+ public:
+  explicit DeleteSessionRequest(std::unique_ptr<InspectorSession> session)
+                                : session_(std::move(session)) {}
+  void Call() {


Can you mark these override?

addaleax · 2018-06-07T16:20:54Z

src/inspector/main_thread_interface.cc

+
+void DispatchFromInterrupt(v8::Isolate* isolate,
+                           void* interface) {
+  reinterpret_cast<MainThreadInterface*>(interface)->DispatchMessages();


style nit: static_cast

eugeneo · 2018-06-07T18:43:55Z

Thanks for the review.
Please note there are tests failing (apparently, everywhere except Debian I'm using locally) - I will be addressing them. I do not expect having to change anything drastically.

eugeneo · 2018-06-11T18:48:57Z

This PR needs bit more significant changes then anticipated, it is still in progress.

eugeneo · 2018-06-16T00:29:31Z

I've uploaded a new change and this PR is now ready for review. This change addresses a race condition that caused some inspector tests to become unreliable, I will run stress tests to confirm it.

eugeneo · 2018-06-17T01:56:25Z

CI link (all green): https://ci.nodejs.org/job/node-test-pull-request/15500/

eugeneo · 2018-06-17T05:04:59Z

Stress tests: https://ci.nodejs.org/job/node-stress-single-test/1926/nodes=win2016-1p-vs2017/console

My understanding is that the tests were not flaky. Did I have to remove them from the flaky tests list first?

TimothyGu

A few nits.

TimothyGu · 2018-06-17T04:39:47Z

src/inspector_io.h

 #include <memory>
 #include <stddef.h>

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

-
 // Forward declaration to break recursive dependency chain with src/env.h.


This comment should move with class Environment.

TimothyGu · 2018-06-17T04:48:04Z

src/inspector_io.cc

+    for (const auto& outgoing : GetMessages()) {
+      int session_id = std::get<1>(outgoing);
+      switch (std::get<0>(outgoing)) {
+      case TransportAction::kKill:


One more level of indentaiton.

jasnell

LGTM but I'd like @addaleax to review also :-)

eugeneo · 2018-07-09T16:31:35Z

I did a rebase. I was unable to reintegrate WS messages logging, it was not implemented correctly (was accessing env from the non-main thread). Is it really necessary?

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

eugeneo · 2018-07-09T16:32:14Z

@addaleax can you please take a look?

eugeneo · 2018-07-09T17:40:54Z

Another CI, previous push excluded some fixes from another workstation: https://ci.nodejs.org/job/node-test-pull-request/15772/

addaleax · 2018-07-09T16:37:31Z

src/inspector/main_thread_interface.cc

+template <typename T>
+class DeleteRequest : public Request {
+ public:
+  explicit DeleteRequest(T object)


nit: I’d use std::shared_ptr<T> object here explicitly, and std::shared_ptr<T> object_; as the member, to be clearer about how this is being used

Done. Added some asserts to ensure resetting the pointer actually destroys the object.

addaleax · 2018-07-09T16:39:28Z

src/inspector/main_thread_interface.cc

+void DispatchFromInterrupt(v8::Isolate* isolate,
+                           void* interface) {
+  static_cast<MainThreadInterface*>(interface)->DispatchMessages();
+}


nit: This can just be an inline lambda, that makes it a bit more readable since it’s much closer to the call site then

addaleax · 2018-07-09T16:42:16Z

src/inspector/main_thread_interface.cc

+  ~CrossThreadInspectorSessionBlock() {
+    DeleteOnAnotherThread(handle_, std::move(session_));
+  }
+  void Connect(


style nit: Leave a space between method definitions

addaleax · 2018-07-09T16:44:01Z

src/inspector/main_thread_interface.cc

+      new DeleteRequest<Type>(std::move(object))));
+}
+
+class CrossThreadInspectorSessionBlock {


It might be good to add a comment explaining what “Block” means in this context (It’s not clear to me at this point)

I renamed the class, hope now it is more clear. It is meant to keep the parts of the state that live on another thread separate.

addaleax · 2018-07-09T16:44:39Z

src/inspector/main_thread_interface.cc

+
+ private:
+  void DispatchOnMainThread(std::unique_ptr<StringBuffer> message) {
+  }


Is this unused?

addaleax · 2018-07-09T17:27:01Z

src/inspector/main_thread_interface.h

+  // when we reenter the DispatchMessages function.
+  MessageQueue dispatching_message_queue_;
+  bool dispatching_messages_ = false;
+  Mutex state_lock_;


Can you add a comment indicating which members exactly are protected by this lock?

Done. Also, renamed the lock.

addaleax · 2018-07-09T17:29:10Z

src/inspector_agent.cc

+      int session_id, std::shared_ptr<NodeInspectorClient> client)
+      : session_id_(session_id), client_(client) {}
+  ~SameThreadInspectorSession() override;
+  void Dispatch(v8_inspector::StringView message) override;


const v8_inspector::StringView&?

addaleax · 2018-07-09T17:36:35Z

src/inspector_io.cc

+      std::deque<std::tuple<TransportAction, int,
+                  std::unique_ptr<v8_inspector::StringBuffer>>>;
+
+  explicit RequestQueueData(uv_loop_t* loop):


(tiny) style nit: put : on the next line

addaleax · 2018-07-09T17:38:10Z

src/inspector_io.cc

+ public:
+  using MessageQueue =
+      std::deque<std::tuple<TransportAction, int,
+                  std::unique_ptr<v8_inspector::StringBuffer>>>;


Might be worth using std::deque<Message> for some struct Message { … }; instead, for readability?

addaleax · 2018-07-09T17:40:37Z

src/inspector_io.cc

+        node::ContainerOf(&RequestQueueData::async_,
+                          reinterpret_cast<uv_async_t*>(handle));
+    delete wrapper;
+  }


I’d personally make both of these lambdas as well, for readability

eugeneo · 2018-07-09T21:09:12Z

Thank you for the review. I addressed the comments.

eugeneo · 2018-07-10T16:38:36Z

Investigating a test failure.

eugeneo · 2018-07-10T21:01:51Z

Stress test for the fixed Windows failure: https://ci.nodejs.org/job/node-stress-single-test/1943/

New CI for this change: https://ci.nodejs.org/job/node-test-pull-request/15786/

eugeneo · 2018-07-10T21:48:34Z

100 iterations of the tests marked as flaky on Windows - https://ci.nodejs.org/job/node-stress-single-test/nodes=win2012r2-mp-vs2017/1944/

Looks like no failures, there is some weird macOS 10.10 failure, will need to investigate.

eugeneo · 2018-07-12T00:01:09Z

I redid management for objects living on another thread. CI is green now: https://ci.nodejs.org/job/node-test-pull-request/15816/

I also run a stress test on a number of inspector flaky tests, seems like they should be fixed now.

Delta with the reviewed version: 14cb71a

I do not think the changes are big enough to invalidate the approvals, but I will wait a couple days in case there are any comments.

Workers debugging will require interfacing between the "main" inspector and per-worker isolate inspectors. This is consistent with what WS interface does. This change is a refactoring change and does not change the functionality. PR-URL: #21182 Fixes: #21725 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2018-07-13T18:29:50Z

CI failure in the status bar looks weird (edit: see Eugene’s run below)

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

eugeneo · 2018-07-13T18:33:35Z

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

There is one failure, when building V8 files on CentOS7. I am fairly confident this can be ignored. I will land the change.

addaleax · 2018-07-13T23:05:07Z

Landed in 39977db

Workers debugging will require interfacing between the "main" inspector and per-worker isolate inspectors. This is consistent with what WS interface does. This change is a refactoring change and does not change the functionality. PR-URL: nodejs#21182 Fixes: nodejs#21725 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

After nodejs#21182, the test is no longer flaky. Since it doesn’t use a static port, moving it to parallel seems justified.

Workers debugging will require interfacing between the "main" inspector and per-worker isolate inspectors. This is consistent with what WS interface does. This change is a refactoring change and does not change the functionality. PR-URL: #21182 Fixes: #21725 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Workers debugging will require interfacing between the "main" inspector and per-worker isolate inspectors. This is consistent with what WS interface does. This change is a refactoring change and does not change the functionality. PR-URL: nodejs#21182 Fixes: nodejs#21725 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2018-07-15T16:59:27Z

Unfortunately, this change seems to have increased the failure of sequential/test-debug-prompt test on OS X 10.10 (Yosemite) in CI from about 0.01% failure to about a 24% failure. See #21724. I'm not sure if the problem lies here or in the test or is something specific to older versions of OS X.

EDIT: PR to possibly fix test: #21826.

After #21182, the test is no longer flaky. Since it doesn’t use a static port, moving it to parallel seems justified. PR-URL: #21806 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 6, 2018

eugeneo commented Jun 6, 2018

View reviewed changes

addaleax reviewed Jun 7, 2018

View reviewed changes

eugeneo mentioned this pull request Jun 7, 2018

Inspector tests leaving around stale processes nodejs/reliability#5

Closed

eugeneo mentioned this pull request Jun 16, 2018

Worker debug #21364

Merged

3 tasks

TimothyGu reviewed Jun 17, 2018

View reviewed changes

jasnell approved these changes Jun 21, 2018

View reviewed changes

addaleax reviewed Jul 9, 2018

View reviewed changes

addaleax mentioned this pull request Jul 9, 2018

Investigate flaky sequential/test-inspector-break-when-eval #21725

Closed

addaleax approved these changes Jul 9, 2018

View reviewed changes

eugeneo added the wip Issues and PRs that are still a work in progress. label Jul 10, 2018

eugeneo removed the wip Issues and PRs that are still a work in progress. label Jul 10, 2018

eugeneo added the wip Issues and PRs that are still a work in progress. label Jul 10, 2018

eugeneo removed the wip Issues and PRs that are still a work in progress. label Jul 12, 2018

addaleax added the inspector Issues and PRs related to the V8 inspector protocol label Jul 13, 2018

addaleax approved these changes Jul 13, 2018

View reviewed changes

addaleax closed this Jul 13, 2018

addaleax added a commit to addaleax/node that referenced this pull request Jul 13, 2018

test: move inspector test back to parallel, unmark flaky

b11cf20

After nodejs#21182, the test is no longer flaky. Since it doesn’t use a static port, moving it to parallel seems justified.

addaleax mentioned this pull request Jul 13, 2018

test: move inspector test back to parallel, unmark flaky #21806

Closed

3 tasks

eugeneo deleted the asyncs_io_prepare_worker branch July 14, 2018 00:23

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

codebytere mentioned this pull request Jun 19, 2019

repl.start Causes Crash in Renderer electron/electron#18872

Closed

3 tasks

pipobscure mentioned this pull request Nov 12, 2019

VSCode 1.40 crash after startup with segmentation fault microsoft/vscode#84233

Closed

		@@ -84,6 +83,7 @@ class InspectorSocketServerTest : public ::testing::Test {
		}
		};

inspector: split main thread interface from transport #21182

inspector: split main thread interface from transport #21182

Conversation

eugeneo commented Jun 6, 2018

Checklist

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

addaleax 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Jun 7, 2018

eugeneo commented Jun 11, 2018

eugeneo commented Jun 16, 2018

eugeneo commented Jun 17, 2018

eugeneo commented Jun 17, 2018

TimothyGu 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

jasnell left a comment

Choose a reason for hiding this comment

eugeneo commented Jul 9, 2018

eugeneo commented Jul 9, 2018

eugeneo commented Jul 9, 2018

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 Jul 9, 2018

eugeneo commented Jul 10, 2018

eugeneo commented Jul 10, 2018

eugeneo commented Jul 10, 2018

eugeneo commented Jul 12, 2018

addaleax commented Jul 13, 2018 • edited Loading

eugeneo commented Jul 13, 2018

addaleax commented Jul 13, 2018

Trott commented Jul 15, 2018 • edited Loading

addaleax commented Jul 13, 2018 •

edited

Loading

Trott commented Jul 15, 2018 •

edited

Loading