Huge memory leak with Jest/Enzyme after upgrading to react-modal 3.X #593

ryami333 · 2018-01-10T03:14:30Z

Summary:

For a while now, I've been unable to upgrade to react-modal 3.X in my React 16 project with Jest/Enzyme because when I run tests Node itself actually crashes and I usually have to take some fairly nuclear steps such as force-quitting the process in my activity monitor app in order to use it again. The latest 2.X release of react-modal is totally fine.

Steps to reproduce:

Upgrade react-modal from 2.4.1 to 3.X
Run npm run jest
First few tests may run, but eventually the process hangs, and eventually these errors start to spill out:

<--- Last few GCs --->

[46548:0x102802400]    43615 ms: Mark-sweep 1402.9 (1437.6) -> 1402.9 (1436.6) MB, 1477.5 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 1477 ms) last resort GC in old space requested
[46548:0x102802400]    45072 ms: Mark-sweep 1402.9 (1436.6) -> 1402.9 (1436.6) MB, 1456.0 / 0.0 ms  last resort GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x38a049725ec1 <JSObject>
    1: read [/path/to/project/node_modules/react-dom/cjs/react-dom-server.node.development.js:~2206] [pc=0x181429965313](this=0x38a08a383211 <ReactDOMServerRenderer map = 0x38a00fd8cc29>,bytes=0x38a0125029a9 <Number inf>)
    3: renderToStaticMarkup [/path/to/project/node_modules/react-dom/cjs/react-dom-server.node.development.js:2512] [bytecode=0x38a0...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: node::Abort() [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 3: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 4: v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 5: v8::internal::Runtime_AllocateInTargetSpace(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 6: 0x18142970463d

*Note that I've replaced the actual path and username in this snippet.

The text was updated successfully, but these errors were encountered:

diasbruno · 2018-01-10T05:51:45Z

Hi @ryami333, can you give more information about this tests?...or is there anything unusual about how are you using react-modal like keep reference of the modal children?

enapupe · 2018-01-11T19:29:58Z

This seems to be an issue with createPortal.. not sure if enzyme has proper support for it?

ryami333 · 2018-01-11T22:16:39Z

@diasbruno I'm not doing anything like that, no. I'm using enzyme and enzyme-to-json. Most of my tests follow this kind of pattern:

import React from 'react';
import { render } from 'enzyme'; // 3.3.0 (latest)
import toJson from 'enzyme-to-json'; // 3.3.0 (latest)

describe('FooComponent', () => {
    let wrapper;

    it('renders without crashing', () => {
        wrapper = render(
            <FooComponent />,
        );
    });

    it('matches existing snapshot', () => {
        expect(toJson(wrapper)).toMatchSnapshot();
    });
});

Early on, the tests are all appearing to pass, but they just get slower and slower, and eventually the test runner hangs and those node heap errors start spewing.

@enapupe yeah portals seem the most likely candidate don't they?

diasbruno · 2018-01-11T22:34:50Z

@ryami333 That's a place to look at. Thanks, @enapupe.

ryami333 · 2018-02-19T21:20:16Z

@diasbruno have you had any success replicating this bug? I'm still unable to upgrade at all. Using latest versions of enzyme, enzyme-adapter-react-16, react-test-renderer and of course react-modal.

I've managed to find that it works with Enzyme's 'mount' but not with 'render'.

diasbruno · 2018-02-19T22:00:07Z

This can be related to the environment the modal is been rendered src/components/Modal.js#L187.

It will just return null if it can not use the DOM.

ryami333 · 2018-02-19T22:03:24Z

But why would returning null result in a big memory exception? Returning null is a perfectly valid render return value.

diasbruno · 2018-02-19T22:05:54Z

It's possible that the modal is not clean up resources properly. Can you write a small app with test? It would be better to debug.

ryami333 · 2018-02-19T22:56:31Z

Here you go @diasbruno: https://github.com/ryami333/react-modal-memory-exception

This is a 'Create React App' web-app, where I've done the following:

Installed enzyme, react-modal and enzyme-adapter-react-16.
Added a <Modal> to App.js.
Tried to render the 'App' component in App.test.js.

Simply run yarn install && yarn test (or npm install && npm test I suppose) to replicate.

diasbruno · 2018-02-20T00:05:39Z

The modal seems to be rendered. I'll try to see what is going on.

./node_modules/.bin/react-scripts --inspect-brk test --env=jsdom --runInBand

rharriso · 2018-02-20T18:07:27Z

This could be an issue with jsdom I was seeing a similar bug when running integration tests (no enzyme render). This problem went away when I removed jsdom from the configuration.

diasbruno · 2018-02-20T18:16:09Z

@rharriso Thanks for the info. I'll try to dig in.

rharriso · 2018-02-20T18:21:28Z

@diasbruno some more info WRT jsdom.

This issue remained with the latest version 11.6.2.
I didn't see any issues in their backlog that looked related.

diasbruno · 2018-02-20T18:42:12Z

Some resources to help tracking this issue:

https://github.com/felixge/node-memory-leak-tutorial
jsdom/jsdom#1705

diasbruno · 2018-02-21T00:55:13Z

It seems to be stuck in an infinity loop on:

ReactDOMServerRenderer.prototype.read = function read(bytes) { 
  // bytes = Infinity
  ...

  var child = frame.children[frame.childIndex++]; // always $$typeof: Symbol(react.portal)
  ...
}

diasbruno · 2018-02-21T01:24:10Z

Well...your test has passed. :)

 PASS  src/App.test.js (347.669s)
  â renders without crashing (342764ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        348.978s, estimated 1404s
Ran all test suites related to changed files.

Watch Usage
 âº Press a to run all tests.
 âº Press p to filter by a filename regex pattern.
 âº Press t to filter by a test name regex pattern.
 âº Press q to quit watch mode.
 âº Press Enter to trigger a test run.

diasbruno · 2018-02-21T01:48:34Z

Try to apply this patch on your project and tell me your results.

diff --git a/react-dom-server.node.development.js b/dd.js
index 2039700..d4832da 100644
--- a/react-dom-server.node.development.js
+++ b/dd.js
@@ -2227,7 +2227,8 @@ var ReactDOMServerRenderer = function () {
         }
         continue;
       }
-      var child = frame.children[frame.childIndex++];
+      frame.childIndex++;
+      var child = frame.children[frame.childIndex];
       {
         setCurrentDebugStack(this.stack);
       }

patch -p1 < patch.diff

node_modules/react-dom/cjs/react-dom-server.node.development.js

diasbruno · 2018-02-21T01:49:42Z

It's not a proper fix, I still need to investigate more, it's just to have an idea of what is going on...

diasbruno · 2018-02-22T12:39:30Z

cc @ryami333 Please let me know when you get a chance to try this patch.

rharriso · 2018-02-22T15:41:08Z

@diasbruno I tried this out on my test suite. And it works!

diasbruno · 2018-02-22T15:44:57Z

@rharriso sorry, forgot to mention you. Thanks for trying it.

diasbruno · 2018-02-22T16:21:00Z

After some investigation, I reached the same state as facebook/react/issues/11565.

ryami333 · 2018-02-22T23:34:27Z

@diasbruno that patch works for me too! Does this mean you will create a PR for react itself?

diasbruno · 2018-02-22T23:44:51Z

Actually, it's not a fix. The things I wrote here are just for logging. That (patch) is really incorrect since it will skip the frame.children[0]. So, I'll continue debugging this issue.

WARNING: it is not known the side-effects of this patch. :)

diasbruno · 2018-02-22T23:51:38Z

@ryami333 @rharriso Did you applied the patch on a real project?

ryami333 · 2018-02-22T23:54:55Z

Yes I did, one with over 200 render tests.

diasbruno · 2018-02-23T00:05:34Z

Interesting. I'll continue investigating to find the problem (if possible).

ryami333 · 2018-02-23T00:15:19Z

Sorry, I should qualify my response a little bit - the tests don't all pass, but they no longer crash.

rharriso · 2018-02-23T17:43:37Z

@diasbruno, yes. It's a private company project with 600 tests

rharriso · 2018-02-23T17:44:18Z

However, the error was see in our integration test suite, which renders with ReactDOM, not enzyme

diasbruno · 2018-02-23T17:50:00Z

@rharriso so you are using some kind of browser?

rharriso · 2018-02-23T18:22:54Z

No, these tests just make sure the page responds with 200.
we inject a request into the Hapi server, and looking at the response code.

diasbruno · 2018-02-23T18:47:55Z

There is a second patch I made, it should work with react-test-render (I still need to test it).

I'll publish on a fork of react.

rharriso · 2018-02-23T18:59:13Z

@diasbruno Let me know what you want me to test out.

diasbruno · 2018-02-24T00:31:46Z

The potential bug is:

while (React.isValidElement(child)) {. If the child has type react.portal, React.isValidElement(child) will return false. Then it will return {child, context};.

So, it will end up in an infinity loop ({child: nextChild, context} = resolve(child, context));.

Note that in the current revision, it seems to have a fix $$typeof !== REACT_PORTAL_TYPE.

I ended up with this code before I looked the current version of react:

function isPortalElement(object) {
  return typeof object === 'object' && object !== null && object.$$typeof === REACT_PORTAL_TYPE;
}

  if (React.isPortalElement(child)) {
    child = null;
  }
  
  return { child: child, context: context };

@rharriso @ryami333 If you can't upgrade your react version, let me know so I can provide the patch to this version.

rharriso · 2018-02-24T02:32:13Z

@diasbruno I probably can't ~~this~~ until after next week. But probably soon after, I'll get back to you next week though!

ryami333 · 2018-02-24T02:35:38Z

@diasbruno - what do you mean by upgrading React? I've been experiencing this on the latesr version of React (16.2.0)

diasbruno · 2018-02-24T02:43:23Z

You are right. Totally forgot I was on the master branch.

I'm trying to finish the patch to submit to react.

The patch is available here.

diasbruno · 2018-02-25T17:22:24Z

Sorry for dumping a lot of information that might be confusing or not organized (that's because I was starting to get confused by the many packages involved in this issue and I was starting to look into the react-test-render issue).

asborisov · 2018-03-09T09:08:43Z

@diasbruno Can you advice the best way to avoid this problem for now (until fix merged into next version of react)?

rharriso · 2018-03-09T16:05:13Z

@asborisov I avoided this by removing jsdom from our integration tests. That may not be an option for unit tests though.

diasbruno · 2018-03-09T16:20:49Z

@asborisov @rharriso Follow the react issue I've mention here. Many ideas/workaround have been publish there.

asborisov · 2018-03-10T03:24:02Z

I downgrade to 2.4.2 and looks it work now. Will wait until correct fix is merged

diasbruno added bug help wanted needs info testing labels Feb 20, 2018

layershifter mentioned this issue Jul 24, 2018

fix(Popup): default open position Semantic-Org/Semantic-UI-React#3029

Merged

Huge memory leak with Jest/Enzyme after upgrading to react-modal 3.X #593

Huge memory leak with Jest/Enzyme after upgrading to react-modal 3.X #593

Comments

ryami333 commented Jan 10, 2018 • edited Loading

Summary:

Steps to reproduce:

diasbruno commented Jan 10, 2018

enapupe commented Jan 11, 2018

ryami333 commented Jan 11, 2018 • edited Loading

diasbruno commented Jan 11, 2018

ryami333 commented Feb 19, 2018 • edited Loading

diasbruno commented Feb 19, 2018

ryami333 commented Feb 19, 2018

diasbruno commented Feb 19, 2018 • edited Loading

ryami333 commented Feb 19, 2018 • edited Loading

diasbruno commented Feb 20, 2018

rharriso commented Feb 20, 2018

diasbruno commented Feb 20, 2018

rharriso commented Feb 20, 2018

diasbruno commented Feb 20, 2018

diasbruno commented Feb 21, 2018

diasbruno commented Feb 21, 2018 • edited Loading

diasbruno commented Feb 21, 2018

diasbruno commented Feb 21, 2018

diasbruno commented Feb 22, 2018

rharriso commented Feb 22, 2018

diasbruno commented Feb 22, 2018

diasbruno commented Feb 22, 2018

ryami333 commented Feb 22, 2018

diasbruno commented Feb 22, 2018 • edited Loading

diasbruno commented Feb 22, 2018

ryami333 commented Feb 22, 2018

diasbruno commented Feb 23, 2018

ryami333 commented Feb 23, 2018

rharriso commented Feb 23, 2018

rharriso commented Feb 23, 2018

diasbruno commented Feb 23, 2018

rharriso commented Feb 23, 2018 • edited Loading

diasbruno commented Feb 23, 2018

rharriso commented Feb 23, 2018

diasbruno commented Feb 24, 2018

rharriso commented Feb 24, 2018 • edited Loading

ryami333 commented Feb 24, 2018

diasbruno commented Feb 24, 2018

diasbruno commented Feb 25, 2018

asborisov commented Mar 9, 2018

rharriso commented Mar 9, 2018

diasbruno commented Mar 9, 2018

asborisov commented Mar 10, 2018

ryami333 commented Jan 10, 2018 •

edited

Loading

ryami333 commented Jan 11, 2018 •

edited

Loading

ryami333 commented Feb 19, 2018 •

edited

Loading

diasbruno commented Feb 19, 2018 •

edited

Loading

ryami333 commented Feb 19, 2018 •

edited

Loading

diasbruno commented Feb 21, 2018 •

edited

Loading

diasbruno commented Feb 22, 2018 •

edited

Loading

rharriso commented Feb 23, 2018 •

edited

Loading

rharriso commented Feb 24, 2018 •

edited

Loading