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.

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

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

Merged
merged 2 commits into from
Nov 13, 2022
Merged

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

merged 2 commits into from
Nov 13, 2022

Conversation

Trott
Copy link
Member

@Trott Trott 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

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2022
@nodejs-github-bot nodejs-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-actions github-actions bot 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

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

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2022
@github-actions github-actions bot 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

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

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

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

@Trott
Copy link
Member Author

Trott commented Nov 8, 2022

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

@github-actions github-actions bot 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
Member Author

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
Member Author

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
Member Author

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
Member Author

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
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2022
@Trott Trott 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-actions github-actions bot 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-actions github-actions bot 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-actions github-actions bot mentioned this pull request Nov 11, 2022
Open
19 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot 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

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

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

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

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

Landed in 6a22b77

@Trott Trott 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 <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ruyadorno ruyadorno 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 <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams 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 <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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 <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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 <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues 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