test: reduce flakiness of test-esm-loader-hooks
#49248
Conversation
nodejs-github-bot
added
esm
This comment was marked as outdated.
This comment was marked as outdated.
|
LGTM on the solution but I am a little concerned (as I expressed in #49105) that this is the experience loader authors will have |
There was a problem hiding this comment.
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 🙌)
This comment was marked as outdated.
This comment was marked as outdated.
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. |
|
Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/416/ |
GeoffreyBooth
added
the
fast-track
|
Fast-track has been requested by @GeoffreyBooth. Please 👍 to approve. |
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. |
GeoffreyBooth
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 0daea96 |
PR-URL: #49248 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#49248 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: #49248 Backport-PR-URL: #50669 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs/node#49248 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs/node#49248 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
It seems that #49105 wasn't enough to address the issue on every platforms.