Skip to content

test: fix flaky test-repl-sigint-nested-eval#45354

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 2 commits into from
Nov 13, 2022
Merged

test: fix flaky test-repl-sigint-nested-eval #45354

nodejs-github-bot merged 2 commits into from
Nov 13, 2022

Conversation

@Trott
Copy link
Member

@TrottTrott commented Nov 7, 2022

There is a race condition where process.kill can be sent before the target is ready to receive the signal. Or at least that's what I think is going on. Regardless, objectively, using setTimeout() to slightly delay the invocation causes the test to not fail anymore using: tools/test.py --repeat=1000 test/parallel/test-repl-sigint-nested-eval

Fixes: #41123

@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2022
@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 7, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 7, 2022

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

@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 7, 2022

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

@github-actionsgithub-actionsbot mentioned this pull request Nov 8, 2022
Open
17 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 8, 2022

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

@Trott
Copy link
MemberAuthor

Trott commented Nov 8, 2022

@nodejs/process @nodejs/child_process Is my conjecture of process.kill() plausible?

@github-actionsgithub-actionsbot mentioned this pull request Nov 9, 2022
Open
14 tasks
@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2022

Did you try with setImmediate or queueMicrotask? 10 looks like a magic number we wouldn't want to keep in the test.

@Trott
Copy link
MemberAuthor

Trott commented Nov 9, 2022

Did you try with setImmediate or queueMicrotask? 10 looks like a magic number we wouldn't want to keep in the test.

I did not test with setImmediate() and queueMicrotask() but I did try setTimeout() with 1 instead of 10 and it greatly reduced the flakiness but did not eliminate it.

@Trott
Copy link
MemberAuthor

Trott commented Nov 9, 2022

Did you try with setImmediate or queueMicrotask? 10 looks like a magic number we wouldn't want to keep in the test.

I did not test with setImmediate() and queueMicrotask() but I did try setTimeout() with 1 instead of 10 and it greatly reduced the flakiness but did not eliminate it.

Maybe there is an event that we can/should wait for before process.kill() is invoked.

@Trott
Copy link
MemberAuthor

Trott commented Nov 9, 2022

(All that said, I'd rather have the magic number and a working test than a forever-skipped-on-all-platforms test. We might as well remove the test in that case.)

@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2022

Maybe let's add a TODO find which event should be listened to instead of arbitrary wait 10 ms, and land it as is. wdyt?

@lpinca
Copy link
Member

lpinca commented Nov 9, 2022

I would keep it flaky until the cause of the crash is found. A flaky test has an implicit TODO, the flakiness. We can add a comment specifying that delaying process.kill() fixes the issue, but we should figure out why.

@Trott
Copy link
MemberAuthor

Trott commented Nov 9, 2022•
edited
Loading

I think the issue is that the signal sent from the child process sometimes get processed by the parent process before the child process has its vm running. The solution I've come up with is to switch from signal SIGUSR2 to IPC (since we're testing SIGINT and the SIGUSR2 was just a way to let the parent process know that it was OK to send a SIGINT now) so that the vm itself can send the message, resulting in a completely reliable test. 🎉

PTAL.

@Trott
test: fix flaky test-repl-sigint-nested-eval
80ade7f
There is a race condition where process.kill can be sent before the target is ready to receive the signal. Fixes: nodejs#41123
@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2022
@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 9, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsbot mentioned this pull request Nov 10, 2022
Open
20 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsbot mentioned this pull request Nov 11, 2022
Open
19 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsbot mentioned this pull request Nov 12, 2022
Open
19 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 13, 2022

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

@github-actionsgithub-actionsbot mentioned this pull request Nov 13, 2022
Open
25 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 13, 2022

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

@TrottTrott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
@nodejs-github-botnodejs-github-bot merged commit 6a22b77 into nodejs:mainNov 13, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 13, 2022

Landed in 6a22b77

@TrottTrott deleted the flaky branch November 13, 2022 03:17
This was referenced Nov 14, 2022
Open
Open
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
@Trott@ruyadorno
test: fix flaky test-repl-sigint-nested-eval
4fe5c4e
There is a race condition where process.kill can be sent before the target is ready to receive the signal. Fixes: #41123 PR-URL: #45354 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@ruyadornoruyadorno mentioned this pull request Nov 24, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@Trott@danielleadams
test: fix flaky test-repl-sigint-nested-eval
6a03ac9
There is a race condition where process.kill can be sent before the target is ready to receive the signal. Fixes: #41123 PR-URL: #45354 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@Trott@danielleadams
test: fix flaky test-repl-sigint-nested-eval
76ee666
There is a race condition where process.kill can be sent before the target is ready to receive the signal. Fixes: #41123 PR-URL: #45354 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@Trott@danielleadams
test: fix flaky test-repl-sigint-nested-eval
4283af4
There is a race condition where process.kill can be sent before the target is ready to receive the signal. Fixes: #41123 PR-URL: #45354 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@Trott@danielleadams
test: fix flaky test-repl-sigint-nested-eval
6710b31
There is a race condition where process.kill can be sent before the target is ready to receive the signal. Fixes: #41123 PR-URL: #45354 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes

@lpincalpincalpinca approved these changes

@tniessentniessentniessen approved these changes

@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-repl-sigint-nested-eval

6 participants

@Trott@nodejs-github-bot@aduh95@lpinca@benjamingr@tniessen