Skip to content

inspector: wait for both sides closing - #8505

Closed
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:fix_is_active
Closed

inspector: wait for both sides closing#8505
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:fix_is_active

Conversation

@eugeneo

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 test, no run-time changes.

Description of change

Removes race condition when test relied on both sides of the socket
to be closed on the same UV event loop iteration.

Fixes: #8498

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 12, 2016
@cjihrig

Copy link
Copy Markdown
Contributor

LGTM

@mscdexmscdex added the inspector Issues and PRs related to the V8 inspector protocol label Sep 12, 2016
@bnoordhuis

Copy link
Copy Markdown
Member

LGTM. Can you s/Wait/wait/ in the status line?

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

Removes race condition when test relied on both sides of the socket
to be closed on the same UV event loop iteration.
Fixes: #8498
@eugeneoeugeneo changed the title inspector: Wait for both sides closinginspector: wait for both sides closingSep 13, 2016
@eugeneo

Copy link
Copy Markdown
ContributorAuthor

Updated the status line. Thank you for the review.

@bnoordhuis

Copy link
Copy Markdown
Member

CI seems pretty happy. Infrastructure failure on one of the arm buildbots ("hudson.remoting.ChannelClosedException: channel is already closed".)

@saghul

Copy link
Copy Markdown
Member

LGTM. Tested locally, works! 👍

@jasnelljasnell 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.

LGTM

@indutnyindutny 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.

LGTM

eugeneo pushed a commit that referenced this pull request Sep 22, 2016
Removes race condition when test relied on both sides of the socket
to be closed on the same UV event loop iteration.
Fixes: #8498
PR-URL: #8505
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-by: Fedor Indutny <fedor@indutny.com>
@eugeneo

Copy link
Copy Markdown
ContributorAuthor

Landed as a67f11d

@eugeneoeugeneo closed this Sep 22, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
Removes race condition when test relied on both sides of the socket
to be closed on the same UV event loop iteration.
Fixes: nodejs#8498
PR-URL: nodejs#8505
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-by: Fedor Indutny <fedor@indutny.com>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Removes race condition when test relied on both sides of the socket
to be closed on the same UV event loop iteration.
Fixes: #8498
PR-URL: #8505
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-by: Fedor Indutny <fedor@indutny.com>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Removes race condition when test relied on both sides of the socket
to be closed on the same UV event loop iteration.
Fixes: #8498
PR-URL: #8505
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-by: Fedor Indutny <fedor@indutny.com>
@eugeneo
eugeneo deleted the fix_is_active branch October 13, 2016 23:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inspectorIssues and PRs related to the V8 inspector protocoltestIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failures on macOS Sierra 10.12 (GM) with Xcode 8.0 (GM)

9 participants

@eugeneo@cjihrig@bnoordhuis@saghul@indutny@jasnell@mscdex@MylesBorins@nodejs-github-bot