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-repl-sigint-nested-eval #45354

Merged
merged 2 commits into from Nov 13, 2022
Merged

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

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

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Nov 8, 2022

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

@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

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.

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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@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
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
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
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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 Dec 30, 2022
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
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
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
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
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-repl-sigint-nested-eval
6 participants