Skip to content

inspector: address race conditions - #8672

Closed
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:centos-test-failure
Closed

inspector: address race conditions#8672
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:centos-test-failure

Conversation

@eugeneo

@eugeneoeugeneo commented Sep 20, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector: bugfix

Description of change

There seems to be a race condition, when inspector receives message that
the frontend had disconnected while the runtime is shutting down.

Fixes: #8669

CC: @Trott@ofrobots

@nodejs-github-botnodejs-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 Sep 20, 2016
@eugeneoeugeneo mentioned this pull request Sep 20, 2016
Closed
@eugeneoeugeneo changed the title inspector: address race conditioninspector: address the race conditionSep 21, 2016
@eugeneoeugeneo changed the title inspector: address the race conditioninspector: address race conditionsSep 21, 2016
@eugeneo

eugeneo commented Sep 21, 2016

Copy link
Copy Markdown
ContributorAuthor

This is CI result for running this test 500 times on each platform - https://ci.nodejs.org/job/node-stress-single-test/949/

There are failures on PI platforms, but I doubt they are caused by this change.

Please take another look.

Eugene Ostroukhov
inspector: address race conditions
13b62ce
Stress tests uncovered 2 race conditions, when IO events happened during
V8 entering event loop on pause or during Node.js shutdown.
Fixes: #8669
@eugeneo

eugeneo commented Sep 22, 2016

Copy link
Copy Markdown
ContributorAuthor

I have reworked this patch a bit. I got rid of the second mutex, slightly renamed variables and extracted blocks into properly named functions. Please take another look.

CI: https://ci.nodejs.org/job/node-test-pull-request/4224/
CI with stress testing: https://ci.nodejs.org/job/node-stress-single-test/953/ (there seem to be some failures that do not seem to be caused by this change)

bnoordhuis
bnoordhuis approved these changes Sep 23, 2016
View reviewed changes

@bnoordhuisbnoordhuis left a comment

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.

Nicely simplified!

@alexkozy

alexkozy commented Sep 23, 2016

Copy link
Copy Markdown
Member

lgtm!

@eugeneo

eugeneo commented Sep 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Thank you for the review.

Last CI: https://ci.nodejs.org/job/node-test-pull-request/4224/
Failures are coming from parallel/test-tick-processor-unknown, which is a known issue.

@eugeneo

eugeneo commented Sep 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Landed as 5acbeb0

@eugeneoeugeneo closed this Sep 23, 2016
eugeneo pushed a commit that referenced this pull request Sep 23, 2016
Eugene Ostroukhov
inspector: address race conditions
5acbeb0
Stress tests uncovered 2 race conditions, when IO events happened during
V8 entering event loop on pause or during Node.js shutdown.
Fixes: #8669
PR-URL: #8672
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
@bnoordhuis

bnoordhuis commented Sep 23, 2016

Copy link
Copy Markdown
Member

@eugeneo Can you use a full URL in the Fixes: tag next time?

@eugeneo

eugeneo commented Sep 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Will do.

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
Eugene OstroukhovMyles Borins
inspector: address race conditions
7e33afa
Stress tests uncovered 2 race conditions, when IO events happened during
V8 entering event loop on pause or during Node.js shutdown.
Fixes: nodejs#8669
PR-URL: nodejs#8672
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Eugene Ostroukhov@jasnell
inspector: address race conditions
7f80ce4
Stress tests uncovered 2 race conditions, when IO events happened during
V8 entering event loop on pause or during Node.js shutdown.
Fixes: #8669
PR-URL: #8672
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Eugene OstroukhovMyles Borins
inspector: address race conditions
e80ae13
Stress tests uncovered 2 race conditions, when IO events happened during
V8 entering event loop on pause or during Node.js shutdown.
Fixes: #8669
PR-URL: #8672
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky inspector/test-inspector

5 participants

@eugeneo@alexkozy@bnoordhuis@MylesBorins@nodejs-github-bot