inspector: fix inspector hung while disconnecting by alexkozy · Pull Request #8021 · nodejs/node

alexkozy · 2016-08-08T19:27:27Z

Checklist

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

Affected core subsystem(s)

Description of change

If user during stepping make some actions (e.g. stepOver native call)
that requests program break and disconnect DevTools frontend then
AgentImpl won't be disconnected until other message from frontend.

The root of issue:

Inspector requests program break.
User requests disconnect (e.g. refresh page with DevTools frontend).
On program break V8Inspector call runMessageLoopOnPause on
V8NodeInspector.
Message loop will wait until next message from frontend.
After message Agent will be disconnected.
We need to ignore runMessageLoopOnPause on step 3 during disconnecting.

alexkozy · 2016-08-08T19:27:53Z

/cc @eugeneo @ofrobots @dgozman

eugeneo · 2016-08-08T19:35:23Z

src/inspector_agent.cc

This seems prone to race condition. Variable is read and modified on 2 threads without any synchronization.

I wonder if it would be feasable to ::DispatchMessages when entering the main loop.

alexkozy · 2016-08-08T21:27:00Z

Improved base approach. Please take another look.

eugeneo · 2016-08-08T21:53:00Z

src/inspector_agent.cc

This access is not guarded by a mutex...

If user during stepping make some actions (e.g. stepOver native call) that requests program break and disconnect DevTools frontend then AgentImpl won't be disconnected until other message from frontend. The root of issue: 1. Inspector requests program break. 2. User requests disconnect (e.g. refresh page with DevTools frontend). 3. On program break V8Inspector call runMessageLoopOnPause on V8NodeInspector. 4. Message loop will wait until next message from frontend. 5. After message Agent will be disconnected. We can dispatch all pending message on step 3 to solve a problem.

alexkozy · 2016-08-08T22:26:18Z

Uploaded another approach mentioned by @eugeneo in comment.

eugeneo · 2016-08-08T22:41:42Z

LGTM

bnoordhuis · 2016-08-09T12:14:53Z

src/inspector_agent.cc

      return;
    terminated_ = false;
    running_nested_loop_ = true;
+    agent_->DispatchMessages();


Is this thread-safe? DispatchMessages() reads and writes dispatching_messages_ without holding any locks.

eugeneo · 2016-08-09T15:52:46Z

@bnoordhuis DispatchMessages is only called on the main thread (the thread where main V8 loop runs). That flag is only used in that function to prevent reentrance.

bnoordhuis · 2016-08-10T13:12:28Z

I guess I don't understand your previous comment about thread safety then.

alexkozy · 2016-08-10T16:10:03Z

In previous patches I use variable on IO thread (in ::OnRemoteIO) and on main thread (in ::DispatchMessages). IO thread can receives another disconnect message when main thread is processing previous one.

bnoordhuis · 2016-08-19T09:58:30Z

LGTM

ofrobots · 2016-08-22T16:16:21Z

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

alexkozy · 2016-08-22T17:33:39Z

I checked test results and it looks like failed tests don't relate to this change. Could you please try to rerun CI?

bnoordhuis · 2016-08-22T21:29:13Z

sequential/test-crypto-timing-safe-equal is known flaky and due to be reverted, see #8225.

parallel/test-fs-utimes failing on the ppcbe-ubuntu1404 buildbot is... interesting... but unlikely to be caused by this PR.

ofrobots · 2016-08-23T17:19:42Z

The failures indeed look unrelated, but here's a new CI regardless: https://ci.nodejs.org/job/node-test-pull-request/3802/

If user make some actions during (e.g. stepOver native call) that requests program break and disconnect DevTools frontend then AgentImpl won't be disconnected until other message from frontend. The root of issue: 1. Inspector requests program break. 2. User requests disconnect (e.g. refresh page with DevTools frontend). 3. On program break V8Inspector call runMessageLoopOnPause on V8NodeInspector. 4. Message loop will wait until next message from frontend. 5. After message Agent will be disconnected. We can dispatch all pending message on step 3 to solve a problem. PR-URL: #8021 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

ofrobots · 2016-08-23T20:51:51Z

New CI had different hiccups but plinux tests did pass.

Landed as f7a23a2.

If user make some actions during (e.g. stepOver native call) that requests program break and disconnect DevTools frontend then AgentImpl won't be disconnected until other message from frontend. The root of issue: 1. Inspector requests program break. 2. User requests disconnect (e.g. refresh page with DevTools frontend). 3. On program break V8Inspector call runMessageLoopOnPause on V8NodeInspector. 4. Message loop will wait until next message from frontend. 5. After message Agent will be disconnected. We can dispatch all pending message on step 3 to solve a problem. PR-URL: #8021 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

@joshgav

Notable changes: * **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154 * **inspector**: * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021 * add support for uncaught exception (Aleksei Koziatinskii) #8043 * **meta**: add @joshgav to collaborators (Josh Gavant) #8146 * **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145 * ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143

Notable changes: * **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154 * **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054 * **inspector**: * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021 * add support for uncaught exception (Aleksei Koziatinskii) #8043 * **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145 * ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143 PR-URL: #8253

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 Aug 8, 2016

eugeneo reviewed Aug 8, 2016
View reviewed changes

src/inspector_agent.cc Outdated

Copy link

Contributor

eugeneo Aug 8, 2016

Choose a reason for hiding this comment

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

This access is not guarded by a mutex...

bnoordhuis reviewed Aug 9, 2016
View reviewed changes

ofrobots closed this Aug 23, 2016

evanlucas mentioned this pull request Aug 24, 2016

2016-08-26, Version 6.5.0 (Current) #8253

Merged

MylesBorins added the dont-land-on-v4.x label Sep 9, 2016

Uh oh!

Conversation

alexkozy commented Aug 8, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

alexkozy commented Aug 8, 2016

Uh oh!

eugeneo Aug 8, 2016

Choose a reason for hiding this comment

Uh oh!

alexkozy commented Aug 8, 2016

Uh oh!

eugeneo Aug 8, 2016

Choose a reason for hiding this comment

Uh oh!

alexkozy commented Aug 8, 2016

Uh oh!

eugeneo commented Aug 8, 2016

Uh oh!

bnoordhuis Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

eugeneo commented Aug 9, 2016

Uh oh!

bnoordhuis commented Aug 10, 2016

Uh oh!

alexkozy commented Aug 10, 2016

Uh oh!

bnoordhuis commented Aug 19, 2016

Uh oh!

ofrobots commented Aug 22, 2016

Uh oh!

alexkozy commented Aug 22, 2016

Uh oh!

bnoordhuis commented Aug 22, 2016

Uh oh!

ofrobots commented Aug 23, 2016

Uh oh!

ofrobots commented Aug 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants