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: reduce flakiness of test-esm-loader-hooks #49248

Merged
merged 3 commits into from
Aug 20, 2023
Merged

test: reduce flakiness of test-esm-loader-hooks #49248

merged 3 commits into from
Aug 20, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 19, 2023

It seems that #49105 wasn't enough to address the issue on every platforms.

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 19, 2023
@aduh95

This comment was marked as outdated.

Sign in to view
@MoLow
Copy link
Member

MoLow commented Aug 19, 2023

LGTM on the solution but I am a little concerned (as I expressed in #49105) that this is the experience loader authors will have

JakobJingleheimer
JakobJingleheimer approved these changes Aug 19, 2023
View reviewed changes
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

I misunderstood the Slack thread—I thought we were suggesting a change to the implementation.

Mm, seconding Moshe: This feels like sweeping a legit problem under the rug.

That said, I don't have a better idea right now, so if we can at least determine that a downstream solution is possible, that's worth ascertaining (and thanks for fielding 🙌)

@aduh95

This comment was marked as outdated.

Sign in to view
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 19, 2023 •
edited
Loading

I am a little concerned (as I expressed in #49105) that this is the experience loader authors will have

That's still very much a Won't Fix, loaders thread is implemented in a way that it cannot make the process live longer than the main thread, for this reason it's expected that a test that relies work happening on the loader thread would be flaky – unless we force the main thread to stay alive, which is what this PR is doing.
This is the experience loader authors will have, and it's documented behavior: do not use the loader thread to do important work, there's no guarantee that the code outside of the actual hooks will run to completion (and even work happening in the hooks can be terminated at any time if the main thread exits).

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 19, 2023

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/416/

@GeoffreyBooth GeoffreyBooth added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 19, 2023
@github-actions
Copy link
Contributor

Fast-track has been requested by @GeoffreyBooth. Please 👍 to approve.

@GeoffreyBooth
Copy link
Member

That’s still very much a Won’t Fix, loaders thread is implemented in a way that it cannot make the process live longer than the main thread

I agree with this. All sorts of loaders could be running long-lived processes that shouldn’t be artificially keeping the main thread alive if the main thread exits (intentionally or not). It’s far more important that the main thread exit in case of crash than that it be kept afloat because of activity in the loaders thread.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 20, 2023
@nodejs-github-bot nodejs-github-bot merged commit 0daea96 into nodejs:main Aug 20, 2023
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0daea96

@aduh95 aduh95 deleted the test-esm-hooks branch August 20, 2023 08:53
This was referenced Aug 21, 2023
Open
Open
Open
@github-actions github-actions bot mentioned this pull request Aug 24, 2023
Open
27 tasks
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@aduh95 @UlisesGascon
test: reduce flakiness of test-esm-loader-hooks
910378f 
PR-URL: #49248
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
Merged
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
@aduh95 @targos
test: reduce flakiness of test-esm-loader-hooks
cf02160 
PR-URL: nodejs#49248
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos targos mentioned this pull request Nov 11, 2023
Closed
@targos targos added the backported-to-v18.x PRs backported to the v18.x-staging branch. label Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
@aduh95 @targos
test: reduce flakiness of test-esm-loader-hooks
e1c6f46 
PR-URL: #49248
Backport-PR-URL: #50669
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: reduce flakiness of test-esm-loader-hooks
6b492da 
PR-URL: nodejs/node#49248
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: reduce flakiness of test-esm-loader-hooks
85cdfb2 
PR-URL: nodejs/node#49248
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@anonrig anonrig anonrig approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@MoLow MoLow MoLow approved these changes

Assignees
No one assigned
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land. 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.
7 participants
@aduh95 @MoLow @GeoffreyBooth @nodejs-github-bot @anonrig @JakobJingleheimer @targos