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(jest-leak-detector): gc() even harder #10994

Closed
wants to merge 1 commit into from

Conversation

FauxFaux
Copy link
Contributor

@FauxFaux FauxFaux commented Jan 1, 2021

Summary

jest-leak-detector declares a leak in some of our large projects,
a leak which does not exist. I have bisected it down to there
being too deep require() trees, and/or too many files loaded.

It is definitely not a timing issue; replacing the extra gc() with
a multi-second sleep does not fix it. Calling gc() from the app code
surprisingly does fix it, at least, for us. My working theory is that
the app's gc() takes out some of the actual garbage, leaving
leak-detector's gc() time to actually collect all of the code,
resulting, eventually, in no leak.

I realise this is hot garbage. I'm absolutely not going to try and
write a test for this, and I understand if you close the PR with
nelson.gif, and I will continue running with hacks, or try and
raise a PR which allows the leak-detector implementation to be
configurable.

Tested on node 12 and 14, same behaviour.

Test plan

I really advise that you don't.

There are tests which cover the leak detector's behaviour, and they are not broken.

@FauxFaux FauxFaux marked this pull request as ready for review January 1, 2021 20:33
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Sep 8, 2022
jest-leak-detector declares a leak in some of our large projects,
a leak which does not exist. I have bisected it down to there
being too deep require() trees, and/or too many files loaded.

It is definitely not a timing issue; replacing the extra gc() with
a multi-second sleep does not fix it. Calling gc() from the app code
surprisingly does fix it, at least, for us. My working theory is that
the app's gc() takes out some of the actual garbage, leaving
leak-detector's gc() time to actually collect all of the code,
resulting, eventually, in no leak.

I realise this is hot garbage. I'm absolutely not going to try and
write a test for this, and I understand if you close the PR with
nelson.gif, and I will continue running with hacks, or try and
raise a PR which allows the leak-detector implementation to be
configurable.
@FauxFaux
Copy link
Contributor Author

FauxFaux commented Sep 9, 2022

I suspect this is still useful, given other people have raised it. I haven't tested it against the new leak detector code, with FinalisationRegistry, though.

@github-actions github-actions bot removed the Stale label Sep 9, 2022
@SimenB
Copy link
Member

SimenB commented Sep 10, 2022

If a test can be added that shows that it works properly with more gc calls, I'm happy to merge this 🙂

@github-actions
Copy link

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Sep 10, 2023
@github-actions
Copy link

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Oct 10, 2023
@SimenB
Copy link
Member

SimenB commented Oct 11, 2023

FWIW, GC run was made more aggressive in #14526

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 Nov 11, 2023
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