Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: fix flaky test-inspector-connect-main-thread #31637

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 4, 2020

Previously, the test waited for a (any) message from the workers,
and then attached another event listener to a specific kind of
message. However, it was possible that the second listener was
attached after the Worker had already exited, thus never receiving
the message it was supposed to receive. (This is the race condition
here – usually, the Worker thread would exit after the second
listener was attached.)

Solve this by keeping a single 'message' event listener attached
to the worker instance during its entire lifetime.

Fixes: #31226

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Previously, the test waited for a (any) message from the workers,
and then attached another event listener to a specific kind of
message. However, it was possible that the second listener was
attached after the Worker had already exited, thus never receiving
the message it was supposed to receive. (This is the race condition
here – usually, the Worker thread would exit *after* the second
listener was attached.)

Solve this by keeping a single `'message'` event listener attached
to the worker instance during its entire lifetime.

Fixes: nodejs#31226
@addaleax addaleax added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Feb 4, 2020
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 4, 2020
@addaleax addaleax added inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support. and removed test Issues and PRs related to the tests. labels Feb 4, 2020
addaleax added a commit to addaleax/node that referenced this pull request Feb 5, 2020
Motivated by the fact that getting this wrong has led to flaky
tests in our test suite.

Refs: nodejs#31637
@addaleax addaleax requested a review from eugeneo February 7, 2020 11:04
@addaleax addaleax added the review wanted PRs that need reviews. label Feb 7, 2020
@addaleax addaleax requested a review from Trott February 7, 2020 11:04
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

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

Thank you!

addaleax added a commit that referenced this pull request Feb 7, 2020
Motivated by the fact that getting this wrong has led to flaky
tests in our test suite.

Refs: #31637

PR-URL: #31642
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Feb 8, 2020

Landed in 0279c2f

Trott pushed a commit that referenced this pull request Feb 8, 2020
Previously, the test waited for a (any) message from the workers,
and then attached another event listener to a specific kind of
message. However, it was possible that the second listener was
attached after the Worker had already exited, thus never receiving
the message it was supposed to receive. (This is the race condition
here – usually, the Worker thread would exit *after* the second
listener was attached.)

Solve this by keeping a single `'message'` event listener attached
to the worker instance during its entire lifetime.

Fixes: #31226

PR-URL: #31637
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott closed this Feb 8, 2020
@addaleax addaleax deleted the fix-flaky-test-inspector-connect-main-thread branch February 8, 2020 14:15
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Motivated by the fact that getting this wrong has led to flaky
tests in our test suite.

Refs: #31637

PR-URL: #31642
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Previously, the test waited for a (any) message from the workers,
and then attached another event listener to a specific kind of
message. However, it was possible that the second listener was
attached after the Worker had already exited, thus never receiving
the message it was supposed to receive. (This is the race condition
here – usually, the Worker thread would exit *after* the second
listener was attached.)

Solve this by keeping a single `'message'` event listener attached
to the worker instance during its entire lifetime.

Fixes: #31226

PR-URL: #31637
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Motivated by the fact that getting this wrong has led to flaky
tests in our test suite.

Refs: #31637

PR-URL: #31642
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Previously, the test waited for a (any) message from the workers,
and then attached another event listener to a specific kind of
message. However, it was possible that the second listener was
attached after the Worker had already exited, thus never receiving
the message it was supposed to receive. (This is the race condition
here – usually, the Worker thread would exit *after* the second
listener was attached.)

Solve this by keeping a single `'message'` event listener attached
to the worker instance during its entire lifetime.

Fixes: #31226

PR-URL: #31637
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Motivated by the fact that getting this wrong has led to flaky
tests in our test suite.

Refs: #31637

PR-URL: #31642
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Previously, the test waited for a (any) message from the workers,
and then attached another event listener to a specific kind of
message. However, it was possible that the second listener was
attached after the Worker had already exited, thus never receiving
the message it was supposed to receive. (This is the race condition
here – usually, the Worker thread would exit *after* the second
listener was attached.)

Solve this by keeping a single `'message'` event listener attached
to the worker instance during its entire lifetime.

Fixes: #31226

PR-URL: #31637
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Motivated by the fact that getting this wrong has led to flaky
tests in our test suite.

Refs: #31637

PR-URL: #31642
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Previously, the test waited for a (any) message from the workers,
and then attached another event listener to a specific kind of
message. However, it was possible that the second listener was
attached after the Worker had already exited, thus never receiving
the message it was supposed to receive. (This is the race condition
here – usually, the Worker thread would exit *after* the second
listener was attached.)

Solve this by keeping a single `'message'` event listener attached
to the worker instance during its entire lifetime.

Fixes: #31226

PR-URL: #31637
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. flaky-test Issues and PRs related to the tests with unstable failures on the CI. inspector Issues and PRs related to the V8 inspector protocol review wanted PRs that need reviews. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flaky test-inspector-connect-main-thread
4 participants