inspector: allow concurrent inspector sessions #20137

eugeneo · 2018-04-18T18:20:30Z

This change enables concurrent inspector sessions, through WebSocket
interface as well as JS interface, in any combination.

Checklist

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

eugeneo · 2018-04-18T18:21:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14366/ (note there might be platform-specific issues, I will be looking into them)

jasnell · 2018-04-18T18:57:17Z

Generally good with this, haven't reviewed the code yet so won't sign off just yet. But, I wanted to ask if you could articulate some of the issues that could present with this. For instance, suppose I have two concurrent sessions open... is it possible for one to interfere with the other? For instance, what impact does setting a breakpoint in one session have on the other? Can there be hidden side effects?

eugeneo · 2018-04-18T19:07:20Z

@jasnell there are many edge cases, and that is expected...

The main use-case had been to allow different tools use different "domains" concurrently - e.g. some IDE would connect to Chrome for debugging, while Chrome DevTools can still be used to inspect DOM. For Node, this should allow debuginng the application that uses the inspector module.

Breakpoints do not get reported to other frontends, to avoid confusing them.
Pause events are reported to all frontends.
"Debugger.resume" sent by any frontend will resume execution for all.

eugeneo · 2018-04-18T19:08:08Z

Another CI run, I had a bad testcase in the original version: https://ci.nodejs.org/job/node-test-pull-request/14367/

jasnell · 2018-04-18T19:26:59Z

Breakpoints do not get reported to other frontends, to avoid confusing them.

Pause events are reported to all frontends.

"Debugger.resume" sent by any frontend will resume execution for all.

1 and 2 are the concerns for me. What we'll end up with are pauses in one frontend that happens for no obvious reason. What I'd like to make sure of is that when the pause is reported, there is enough context included for one frontend to know that the pause originated in a concurrently connected session so at least there's the ability to reason about it. If that's already accounted for, then awesome, never mind me :-)

eugeneo · 2018-04-18T20:11:56Z

@jasnell this isn't that different from the debugger statement. Frontends already have to be ready for random pauses.

eugeneo · 2018-04-18T23:37:43Z

Found a bug in V8 inspector around multisession support. Updated this PR with a workaround for a said bug. New CI: https://ci.nodejs.org/job/node-test-pull-request/14373/

jasnell · 2018-04-18T23:44:51Z

@eugeneo ... thank you for the additional detail. As I've said I'm generally good with this I just want to make sure we have a good idea of what issues may come up with this. Any time we get into multi-session anything, especially with single loop, there are bound to be issues ;-) . Thank you for doing this!

jasnell

Generally LGTM but would love @addaleax and @bnoordhuis to take a look :-)

eugeneo · 2018-04-19T16:29:21Z

CI for a new version: https://ci.nodejs.org/job/node-test-pull-request/14386/

eugeneo · 2018-04-19T17:50:20Z

CI mostly green, looks like MacOS CI is busted and failes on all changes. There are no inspector-specific test failures.

TimothyGu · 2018-04-19T19:23:07Z

src/inspector_agent.cc

-        new ChannelImpl(client_.get(), delegate));
+  int connectFrontend(std::unique_ptr<InspectorSessionDelegate> delegate) {
+    events_dispatched_ = true;
+    int session_id = next_session_id_++;


Is it possible for this to have a race condition (e.g. WebSocket server and a JS inspector session launching at the same time)?

All API in this file are called only from the main thread. E.g. incoming WS session will "interrupt" V8 (or will use uv_async_t) before calling this function.

TimothyGu · 2018-04-19T19:23:30Z

src/inspector_agent.cc

+    // There is a bug in inspector that causes every session
+    // call V8InspectorClient::quitMessageLoopOnPause. We are using this flag
+    // to ignore those calls so message loop is spinning as long as there's a
+    // reason to expect inspector messages


Is there a crbug for this?

https://crbug.com/834056

Can you put that link in the comment?

I added it in a separate commit, will squash before merging.

bnoordhuis

Left some suggestions for improvements but LGTM either way.

bnoordhuis · 2018-04-20T10:13:33Z

src/inspector_agent.cc

+    events_dispatched_ = true;
+    int session_id = next_session_id_++;
+    channels_[session_id] =
+        std::make_unique<ChannelImpl>(client_.get(), std::move(delegate));


client_.get() looks kind of questionable. Could it be turned into a std::shared_ptr?

I do not want to imply shared ownership. I changed the code to use const std::unique_ptr&.

bnoordhuis · 2018-04-20T10:15:06Z

src/inspector_agent.cc

  std::unordered_map<void*, InspectorTimerHandle> timers_;
+  int next_session_id_;
+  bool events_dispatched_;


bool events_dispatched_ = false;?

Changed this and other similar fields.

bnoordhuis · 2018-04-20T10:16:27Z

src/inspector_agent.cc

+std::unique_ptr<InspectorSession> Agent::Connect(
+    std::unique_ptr<InspectorSessionDelegate> delegate) {
+  int session_id = client_->connectFrontend(std::move(delegate));
+  return std::make_unique<InspectorSession>(session_id, client_.get());


Maybe turn client_ into a std::shared_ptr?

bnoordhuis · 2018-04-20T10:19:57Z

src/inspector_js_api.cc

-    session->CheckIsCurrent();
-    Agent* inspector = env->inspector_agent();
-    inspector->Dispatch(ToProtocolString(env->isolate(), info[0])->string());
+    if (session->session_)


Can you put braces around the body?

bnoordhuis · 2018-04-20T10:20:32Z

src/inspector_js_api.cc

  Persistent<Function> callback_;
 };

+static bool InspectorEnabled(Environment* env) {
+  Agent* agent = env->inspector_agent();
+  return !!agent->io() || agent->HasConnectedSessions();


Can you do an explicit nullptr check instead of coercing to bool?

bnoordhuis · 2018-04-20T10:23:10Z

test/parallel/test-inspector-multisession-ws.js

+}, 150);
+session.on('Debugger.paused', () => {
+  session.disconnect();
+  process.exit(0);


It would be infinitesimally nicer to have node exit naturally by calling clearInterval(interval).

eugeneo · 2018-04-20T17:09:16Z

Thank you for the reviews, I addressed the comments.
New CI: https://ci.nodejs.org/job/node-test-pull-request/14405/

I will merge this PR after the weekend.

Trott · 2018-04-20T17:58:49Z

Relevant macOS test failure:

https://ci.nodejs.org/job/node-test-commit-osx/17976/nodes=osx1010/console

not ok 1107 parallel/test-inspector-multisession-ws
  ---
  duration_ms: 0.330
  severity: fail
  exitcode: 1
  stack: |-
    [test] Connecting to a child Node process
    [test] Testing /json/list
    [test] Connecting to a child Node process
    [test] Testing /json/list
    [err] Debugger listening on ws://127.0.0.1:50554/d51f55a4-567c-49b0-89cd-669ce131d100
    [err] For help, see: https://nodejs.org/en/docs/inspector
    [err] 
    { Error: write EPIPE
        at WriteWrap.afterWrite [as oncomplete] (net.js:836:14) errno: 'EPIPE', code: 'EPIPE', syscall: 'write' }
    1
  ...

Trott

Blocking on the test failure. Feel free to clear this if the test has been fixed.

eugeneo · 2018-04-24T18:38:00Z

Test failure is caused by #20254, I would like to land that PR before landing this one.

This change enables concurrent inspector sessions, through WebSocket interface as well as JS interface, in any combination. PR-URL: #20137

eugeneo · 2018-04-26T18:22:26Z

Rebased, squashed the commits, CI is green: https://ci.nodejs.org/job/node-test-commit/18054/

@Trott - please take another look.

ofrobots · 2018-04-26T18:29:37Z

Given this from statement @Trott:

Blocking on the test failure. Feel free to clear this if the test has been fixed.

Since the CI is green now, I think it is reasonable to dismiss that review.

CI is green.

eugeneo · 2018-04-26T19:54:13Z

Landed as 1300103

addaleax · 2018-04-27T21:10:46Z

Just got this CI failure for an AIX stress-test run: https://ci.nodejs.org/job/node-stress-single-test/1825/nodes=aix61-ppc64/console

It seems related to this PR? Not sure why it wouldn’t show up in regular CI…

richardlau · 2018-04-27T21:17:02Z

Not sure why it wouldn’t show up in regular CI…

I think because the stress job isn't using the compiler selector script added by nodejs/build#1240 so it's using the wrong level of gcc.

cc @nodejs/build @mhdawson

richardlau · 2018-04-27T21:19:16Z

e.g., https://ci.nodejs.org/job/node-stress-single-test/1825/nodes=aix61-ppc64/consoleFull

16:38:06 WARNING: C++ compiler too old, need g++ 4.9.4 or clang++ 3.4.2 (CXX=g++)

We should be using gcc 6.3 on AIX for v10.x and master as per nodejs/build#1240.

eugeneo · 2018-04-27T21:36:22Z

Should I remove make_unique or this is old failure and new AIX now uses newer STL?

richardlau · 2018-04-27T21:42:15Z

@eugeneo I believe it's an infrastructure issue with the stress job not using the correct level of C++ compiler on AIX (the normal CI jobs should be fine).

This PR is the first change that's actually caused the build to fail on older compilers (where previously you would be warned but the build would succeed), but there's a reason we specify the supported toolchains.

eugeneo · 2018-04-27T21:47:15Z

@richardlau Ok, thanks. Ping me if this needs to be fixed - this is a trivial change.

This change enables concurrent inspector sessions, through WebSocket interface as well as JS interface, in any combination. PR-URL: #20137 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Refs: nodejs/node#20137

* nodejs/node#22251 * nodejs/node#20137

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 Apr 18, 2018

eugeneo added the inspector Issues and PRs related to the V8 inspector protocol label Apr 18, 2018

jasnell approved these changes Apr 18, 2018

View reviewed changes

devsnek approved these changes Apr 19, 2018

View reviewed changes

TimothyGu reviewed Apr 19, 2018

View reviewed changes

bnoordhuis approved these changes Apr 20, 2018

View reviewed changes

bnoordhuis approved these changes Apr 21, 2018

View reviewed changes

Trott previously requested changes Apr 21, 2018

View reviewed changes

Trott mentioned this pull request Apr 24, 2018

inspector: Use default uv_listen backlog size #20254

Merged

3 tasks

inspector: allow concurrent inspector sessions

13a6d13

This change enables concurrent inspector sessions, through WebSocket interface as well as JS interface, in any combination. PR-URL: #20137

TimothyGu approved these changes Apr 26, 2018

View reviewed changes

eugeneo closed this Apr 26, 2018

richardlau mentioned this pull request Apr 27, 2018

citgm/stress jobs do not select compiler for AIX on v10.x/master nodejs/build#1254

Closed

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

MarshallOfSound added a commit to electron/electron that referenced this pull request Jun 18, 2018

Fix inspector tests, you can now have multiple inspector sessions

0fbdaa8

Refs: nodejs/node#20137

alexeykuzmin pushed a commit to electron/electron that referenced this pull request Jun 18, 2018

Fix inspector tests, you can now have multiple inspector sessions

206e43b

Refs: nodejs/node#20137

MarshallOfSound added a commit to electron/electron that referenced this pull request Jun 19, 2018

Fix inspector tests, you can now have multiple inspector sessions

de5bdc8

Refs: nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Oct 30, 2018

spec: update inspector-binding.js

1598fc2

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Nov 9, 2018

spec: update inspector-binding.js

c966017

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Nov 14, 2018

spec: update inspector-binding.js

daa892b

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Nov 29, 2018

spec: update inspector-binding.js

1e7c765

* nodejs/node#22251 * nodejs/node#20137

zcbenz pushed a commit to electron/electron that referenced this pull request Nov 30, 2018

spec: update inspector-binding.js

e65ed45

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Dec 2, 2018

spec: update inspector-binding.js

0286a9c

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Dec 3, 2018

spec: update inspector-binding.js

c491fa5

* nodejs/node#22251 * nodejs/node#20137

inspector: allow concurrent inspector sessions #20137

inspector: allow concurrent inspector sessions #20137

Conversation

eugeneo commented Apr 18, 2018

Checklist

eugeneo commented Apr 18, 2018

jasnell commented Apr 18, 2018

eugeneo commented Apr 18, 2018

eugeneo commented Apr 18, 2018

jasnell commented Apr 18, 2018

eugeneo commented Apr 18, 2018 • edited Loading

eugeneo commented Apr 18, 2018

jasnell commented Apr 18, 2018

jasnell left a comment

Choose a reason for hiding this comment

eugeneo commented Apr 19, 2018

eugeneo commented Apr 19, 2018

Choose a reason for hiding this comment

eugeneo Apr 19, 2018 • edited Loading

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

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

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 Apr 20, 2018

Trott commented Apr 20, 2018

Trott left a comment

Choose a reason for hiding this comment

eugeneo commented Apr 24, 2018

eugeneo commented Apr 26, 2018

ofrobots commented Apr 26, 2018

eugeneo commented Apr 26, 2018

addaleax commented Apr 27, 2018

richardlau commented Apr 27, 2018

richardlau commented Apr 27, 2018

eugeneo commented Apr 27, 2018

richardlau commented Apr 27, 2018

eugeneo commented Apr 27, 2018

eugeneo commented Apr 18, 2018 •

edited

Loading

eugeneo Apr 19, 2018 •

edited

Loading