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

fix: worker being killed after being spawned and other worker bugs #13107

Merged
merged 53 commits into from
Aug 11, 2022

Conversation

phawxby
Copy link
Contributor

@phawxby phawxby commented Aug 8, 2022

#13106 (comment)

This adds a test case for the issue described above and can be demonstrated with.

cd e2e/worker-restarting 
node ../../packages/jest-cli/bin/jest.js

@phawxby
Copy link
Contributor Author

phawxby commented Aug 9, 2022

@backmask @SimenB I've found the problem and now I've seen it 🤦

I'm writing some more/better tests to try and catch this in future and should have the PR ready to go in an hour or so

@phawxby phawxby changed the title test: ad integration test case for worker not executing test test: fix worker being killed after being spawned Aug 9, 2022
@phawxby
Copy link
Contributor Author

phawxby commented Aug 10, 2022

PR is mostly done and I solved a few other bugs that I found while I was at it. I'm nos just trying to resolve the Windows weirdness that doesn't occur on my Windows machine.

@phawxby
Copy link
Contributor Author

phawxby commented Aug 11, 2022

FINALLY!!!

@phawxby phawxby marked this pull request as ready for review August 11, 2022 07:05
@phawxby phawxby requested a review from SimenB August 11, 2022 07:05
@phawxby phawxby changed the title test: fix worker being killed after being spawned fix: worker being killed after being spawned and other worker bugs Aug 11, 2022
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

yay!

packages/jest-worker/src/types.ts Outdated Show resolved Hide resolved
packages/jest-worker/src/types.ts Outdated Show resolved Hide resolved
const silent = this._options.silent ?? true;

if (!silent) {
console.warn('Unable to detect out of memory event if silent === false');
Copy link
Member

Choose a reason for hiding this comment

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

we should only log this if _childIdleMemoryUsageLimit is passed I think? Then we can probably also make it an error instead of warning

Copy link
Member

Choose a reason for hiding this comment

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

same comment applies to thread worker - shouldn't we only monitor for OOM if the option is passed, otherwise keep old behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should only log this if _childIdleMemoryUsageLimit is passed I think? Then we can probably also make it an error instead of warning

Yes to the first bit, i'm inclined to say no to the second. It can be useful for debugging to see the child output when a memory limit is in place, for example to see the exact thrown error message.

Copy link
Contributor Author

@phawxby phawxby Aug 11, 2022

Choose a reason for hiding this comment

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

same comment applies to thread worker - shouldn't we only monitor for OOM if the option is passed, otherwise keep old behavior?

Actually, ignore my previous comment....

Detecting an out of memory event and the worker crashing is independent of monitoring of the idle memory usage and rebooting as necessary. The _childIdleMemoryUsageLimit applies to the latter case, the former is building and improving on #13054

Do I need better docs to explain that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some comments to explain

const handler = (chunk: any) => {
if (this._state !== WorkerStates.OUT_OF_MEMORY) {
let str: string | undefined = undefined;
this._detectOutOfMemoryCrash();
Copy link
Member

Choose a reason for hiding this comment

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

here as well, only if _childIdleMemoryUsageLimit is passed?


export default abstract class WorkerAbstract
extends EventEmitter
implements Pick<WorkerInterface, 'waitForWorkerReady' | 'state'>
Copy link
Member

Choose a reason for hiding this comment

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

Create a new interface within types.ts instead of Picking here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an extremely limited use case just to prevent the class throwing typing errors, it didn't really seem necessary to create a new class just for that as it would have no utility elsewhere. Someone could create a different worker type in future and create a different abstract with totally different base props.

packages/jest-worker/src/workers/WorkerAbstract.ts Outdated Show resolved Hide resolved
@phawxby
Copy link
Contributor Author

phawxby commented Aug 11, 2022

These test failures don't appear to be related to my changes, are they known to be flaky?

@@ -44,12 +48,14 @@ export default abstract class WorkerAbstract

if (typeof options.on === 'object') {
for (const [event, handlers] of Object.entries(options.on)) {
if (Array.isArray(handlers)) {
// Can't do Array.isArray on a ReadonlyArray<T>.
// https://github.com/microsoft/TypeScript/issues/17002
Copy link
Member

Choose a reason for hiding this comment

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

oh my 😅

@SimenB
Copy link
Member

SimenB commented Aug 11, 2022

These test failures don't appear to be related to my changes, are they known to be flaky?

Yeah, at times 😞 I restarted the failing builds now

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

looks great!

@phawxby phawxby requested a review from SimenB August 11, 2022 12:15
@phawxby
Copy link
Contributor Author

phawxby commented Aug 11, 2022

Hopefully this is the PR done

@SimenB SimenB merged commit d6ad15b into jestjs:main Aug 11, 2022
@SimenB
Copy link
Member

SimenB commented Aug 11, 2022

@phawxby
Copy link
Contributor Author

phawxby commented Aug 11, 2022

@SimenB we're getting closer, tests are running locally in our main project on the alpha release and heap usage is remaining stable. What has me confused is I'm getting a tonne of snapshot failures, not sure if there's beena. chance to how snapshots are encoded between 27 - 29 which is causing it. Investigating now

 PASS   unit  src/common/helpers/context/Site/mapSiteUrlContextData.test.ts (8.602 s, 240 MB heap size)
 PASS   unit  src/common/helpers/support/getFiltersForContent/__tests__/index.test.ts (5.026 s, 228 MB heap size)
 PASS   unit  src/common/helpers/context/Site/SiteContext.test.ts (8.986 s, 253 MB heap size)
 PASS   unit  src/common/helpers/sku/__tests__/getSkuPricing.test.ts (8.824 s, 240 MB heap size)
 PASS   unit  src/common/helpers/tokens/lib/__tests__/traverseTokens.test.ts (7.575 s, 202 MB heap size)
 PASS   unit  src/common/helpers/tokens/xrx/category/__tests__/index.test.ts (7.508 s, 206 MB heap size)
 PASS   unit  src/common/helpers/contentful/__tests__/getContentfulCascade.test.ts (7.459 s, 211 MB heap size)
 PASS   unit  src/common/helpers/support/__tests__/buildSupportDownloadUrl.test.ts (7.689 s, 208 MB heap size)
 PASS   unit  src/common/helpers/sitemap/generators/__tests__/SupportDndProductSitemapGenerator.test.ts (7.701 s, 213 MB heap size)
 PASS   unit  src/common/helpers/product/findRelatedProducts/index.test.ts (16.946 s, 238 MB heap size)
 PASS   unit  src/common/helpers/locale/localeMatch/__tests__/beMatch.test.ts (98 MB heap size)
 PASS   unit  src/common/helpers/locale/localeMatch/__tests__/xeroxMatch.test.ts (109 MB heap size)
 PASS   unit  src/common/helpers/product/__tests__/performProductFooterNavCascade.test.ts (8.692 s, 230 MB heap size)
 PASS   unit  src/common/helpers/sitemap/generators/__tests__/MarketingProductSkuSitemapGenerator.test.ts (233 MB heap size)
 PASS   unit  src/common/helpers/contentful/__tests__/getMasterGlossary.test.ts (8.866 s, 241 MB heap size)
 PASS   unit  src/common/helpers/context/Site/mapSiteHeaderFooterContextData/__tests__/mapSecondaryLink.test.ts (8.78 s, 239 MB heap size)
 PASS   unit  src/common/helpers/url/__tests__/chaseUrl.test.ts (8.204 s, 207 MB heap size)
 PASS   unit  src/common/helpers/support/__tests__/getPagesForProduct.test.ts (8.222 s, 205 MB heap size)
 PASS   unit  src/common/helpers/akamai/__tests__/invalidate.test.ts (105 MB heap size)
 PASS   unit  src/common/helpers/context/Locale/mapEntryToLocaleContext.test.ts (144 MB heap size)
...

Look at that heap usage :D

@phawxby
Copy link
Contributor Author

phawxby commented Aug 11, 2022

Ah, it appears to be this change.
#13036

This is looking good but I'd prefer more external validation

@SimenB
Copy link
Member

SimenB commented Aug 11, 2022

Ah, it appears to be this change. #13036

Yep.

https://jestjs.io/docs/next/upgrading-to-jest29#snapshot-format

@phawxby
Copy link
Contributor Author

phawxby commented Aug 15, 2022

@SimenB I know you were hoping for a stable release last week had I not broken jest, I assume that means a stable release this week instead?

@SimenB
Copy link
Member

SimenB commented Aug 15, 2022

Yep, that's the plan! Mostly waiting for a resolution on #12856

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants