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 memory leak from snapshot printing #5279

Merged
merged 4 commits into from
Jan 11, 2018

Conversation

rickhanlonii
Copy link
Member

@rickhanlonii rickhanlonii commented Jan 11, 2018

Summary

This PR fixes a memory leak introduced in #5020

Fixes #5234
Fixes #5239
Fixes #5157

Details

I initially confirmed that --detectLeaks is broken for all tests. The next step was to figure out if this was a bug in the way we detect leaks or if it's a legit leak. Using this repo, I walked the commits backward to find the last passing test.

That lead me to this commit. Nothing there is related to the leak detection, so I figured it must be a legit leak. From there I commented out lines until I could prove which line would fix the tests, then inspected the line.

It looks like we were keeping around references to the whole snapshot state. The fix is to remove the dependency using Array.from.

I'm not entirely sure how this is failing the environment leak detection, maybe the Snapshot State somehow keeps a reference to the environment?

Test plan

There are a couple of ways to test/demonstrate this.

1. via --detectLeaks

Considering this test repo, running yarn test from master will fail:

With this change they pass:

2. via React

This fix also fixes the memory issue @gaearon found in React for this command:

yarn test --coverage --runInBand --logHeapUsage

Result of Jest master (OOM at 1.5GB):

With this fix (capped at 500MB):

@codecov-io
Copy link

codecov-io commented Jan 11, 2018

Codecov Report

Merging #5279 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5279   +/-   ##
=======================================
  Coverage   60.98%   60.98%           
=======================================
  Files         203      203           
  Lines        6816     6816           
  Branches        3        4    +1     
=======================================
  Hits         4157     4157           
  Misses       2658     2658           
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/index.js 5.63% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5956891...c9ef1a8. Read the comment docs.

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.

This is awesome. thank you so much!

@cpojer cpojer merged commit 4bcfe19 into jestjs:master Jan 11, 2018
@cpojer
Copy link
Member

cpojer commented Jan 11, 2018

This is great work, thanks so much for diving in and triaging this. cc @mjesun the detectLeaks flag was helpful!!

@cpojer
Copy link
Member

cpojer commented Jan 11, 2018

I just published 22.0.6 which includes this fix. cc @gaearon for React.

@rickhanlonii
Copy link
Member Author

Yeah @mjesun the detectLeaks flag did all the work here

@kand
Copy link

kand commented Jan 11, 2018

@rickhanlonii thanks!

@mjesun
Copy link
Contributor

mjesun commented Jan 11, 2018

@rickhanlonii thanks a lot for that! I'm really happy to see the --detectLeaks flag was useful! ☺️

You rock!

@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 May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants