FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

inspector: Put connect and disconnect events into message queue by eugeneo · Pull Request #7271 · nodejs/node · GitHub

/ node Public

inspector: Put connect and disconnect events into message queue - #7271

Closed
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:async-connect
ConversationCommits (1)ChecksFiles changed
Closed

inspector: Put connect and disconnect events into message queue#7271
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:async-connect

Conversation

eugeneo commented Jun 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

inspector

Description of change

Putting connect and disconnect events into the message queue removes some unlikely race conditions. It also allows reusing the V8 interrupt when frontend connects to Node.js running an infinite loop.

CC: @ofrobots

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 11, 2016

eugeneo commented Jun 11, 2016

Copy link
Copy Markdown
Contributor Author

Please note that this PR needs fixes uploaded in #7268 - hence the 2 commits.

mscdex added inspector Issues and PRs related to the V8 inspector protocol c++ Issues and PRs that require attention from people who are familiar with C++. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 11, 2016

ofrobots commented Jun 17, 2016

Copy link
Copy Markdown
Contributor

@eugeneo Now that #7268 has landed, can you rebase this on top of master?

eugeneo commented Jun 17, 2016

Copy link
Copy Markdown
Contributor Author

@ofrobots Done. Thanks!

bnoordhuis commented Jun 22, 2016

Copy link
Copy Markdown
Member

@eugeneo Can you rebase? Thanks.

eugeneo commented Jun 22, 2016

Copy link
Copy Markdown
Contributor Author

I rebased the change. Thanks!

bnoordhuis reviewed Jun 23, 2016
View reviewed changes
Comment thread src/inspector_agent.cc Outdated

bnoordhuis Jun 23, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just a suggestion but you could add a using blink::protocol::String16; at the top.

EDIT: I guess it already exists somewhere? I see that line 197 uses a non-qualified name.

eugeneo Jun 24, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done. String16STD.h already imports it into the default namespace - so I'll be using it.

eugeneo commented Jun 29, 2016

Copy link
Copy Markdown
Contributor Author

@bnoordhuis Please take another look. I addressed the comments and rebased the change. Thanks!

addaleax reviewed Jun 29, 2016
View reviewed changes
Comment thread src/inspector_agent.cc Outdated

addaleax Jun 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

extra space?

eugeneo Jun 29, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yep, artifact of line joining. Fixed/reuploaded. Thanks!

eugeneo commented Jun 30, 2016

Copy link
Copy Markdown
Contributor Author

@bnoordhuis Thanks for the review. I updated the code.

bnoordhuis commented Jul 1, 2016

Copy link
Copy Markdown
Member

LGTM

ofrobots commented Jul 1, 2016

Copy link
Copy Markdown
Contributor

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

inspector: Unify event queues
4430fc6 
Current implementation tracks connected/disconnected status separately
which potentially introduces race condition.
This change introduces notion of session IDs and also posts
connect/disconnect events into the same queue as the messages. This way
Node knows what session given response belongs to and can discard
messages if the frontend for that session had disconnected.

This also fixes an issue when frontend was unable to attach to V8
instance that was running infinite loop.

eugeneo commented Jul 1, 2016

Copy link
Copy Markdown
Contributor Author

@ofrobots I updated the CL with a fix for the libc++ (Mac/BSD build issue). The only change was that MessageQueue vector now stores String16 and not cons String16.

ofrobots commented Jul 1, 2016

Copy link
Copy Markdown
Contributor

New CI: https://ci.nodejs.org/job/node-test-pull-request/3156/. Looks green.

ofrobots commented Jul 14, 2016

Copy link
Copy Markdown
Contributor

Apologies for the delay. Since it has been ~2 weeks since the last CI, I reran a new one here: https://ci.nodejs.org/job/node-test-pull-request/3299/.

ofrobots commented Jul 14, 2016

Copy link
Copy Markdown
Contributor

Looks green modulo arm-fanned jobs which seems to be a jenkins slave issue.

ofrobots pushed a commit that referenced this pull request Jul 15, 2016
inspector: Unify event queues
4220c24 
Current implementation tracks connected/disconnected status separately
which potentially introduces race condition.
This change introduces notion of session IDs and also posts
connect/disconnect events into the same queue as the messages. This way
Node knows what session given response belongs to and can discard
messages if the frontend for that session had disconnected.

This also fixes an issue when frontend was unable to attach to V8
instance that was running infinite loop.

PR-URL: #7271
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

ofrobots commented Jul 15, 2016

Copy link
Copy Markdown
Contributor

Landed as 4220c24.

ofrobots closed this Jul 15, 2016
evanlucas pushed a commit that referenced this pull request Jul 18, 2016
inspector: Unify event queues
43b5bf4 
Current implementation tracks connected/disconnected status separately
which potentially introduces race condition.
This change introduces notion of session IDs and also posts
connect/disconnect events into the same queue as the messages. This way
Node knows what session given response belongs to and can discard
messages if the frontend for that session had disconnected.

This also fixes an issue when frontend was unable to attach to V8
instance that was running infinite loop.

PR-URL: #7271
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
evanlucas mentioned this pull request Jul 18, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
inspector: Unify event queues
989a2a1 
Current implementation tracks connected/disconnected status separately
which potentially introduces race condition.
This change introduces notion of session IDs and also posts
connect/disconnect events into the same queue as the messages. This way
Node knows what session given response belongs to and can discard
messages if the frontend for that session had disconnected.

This also fixes an issue when frontend was unable to attach to V8
instance that was running infinite loop.

PR-URL: #7271
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL