Skip to content

inspector: address race conditions#8672

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

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

Conversation

@eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Sep 20, 2016

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-bot 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 Sep 20, 2016
@eugeneo eugeneo mentioned this pull request Sep 20, 2016
Closed
@eugeneo eugeneo changed the title inspector: address race condition inspector: address the race condition Sep 21, 2016
@eugeneo eugeneo changed the title inspector: address the race condition inspector: address race conditions Sep 21, 2016
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 21, 2016

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.

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
Copy link
Contributor Author

eugeneo commented Sep 22, 2016

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
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Nicely simplified!

@alexkozy
Copy link
Member

alexkozy commented Sep 23, 2016

lgtm!

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 23, 2016

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
Copy link
Contributor Author

eugeneo commented Sep 23, 2016

Landed as 5acbeb0

@eugeneo eugeneo closed this Sep 23, 2016
eugeneo pushed a commit that referenced this pull request Sep 23, 2016
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
Copy link
Member

bnoordhuis commented Sep 23, 2016

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

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 23, 2016

Will do.

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
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
@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
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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

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.

Investigate flaky inspector/test-inspector

5 participants

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