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

Potential memory leak in React SitecoreContextFactory #287

Closed
vtrbowman opened this issue Oct 31, 2019 · 2 comments
Closed

Potential memory leak in React SitecoreContextFactory #287

vtrbowman opened this issue Oct 31, 2019 · 2 comments
Labels

Comments

@vtrbowman
Copy link

Description

When running load testing on our react JSS app, we noticed memory usage steadily increasing over time. Upon investigation we saw that our JSS app when hosted using node SSR was using increasing memory.

We set up an OOTB JSS React setup and ran against that and noticed the same thing. Digging deeper into the memory dumps pre and post load test we were able to isolate a potential cause for this in the SitecoreContextFactory (https://github.com/Sitecore/jss/blob/dev/packages/sitecore-jss-react/src/components/SitecoreContext.tsx).

In this class the SitecoreContext components get subscribed to the factory changes using subscribeToContext. As far as we can tell however, the unsubscribeFromContext method is never called, so the singleton instance of SitecoreContextFactory will be keeping references to the SitecoreContext for each request and never losing it's reference.

I am by no means a node/react expert, so there may be a simple solution to this which I am unaware of, if we can just get confirmation of if this is an legitimate issue or not first, that would be great.

Expected behavior

We would expect all used component resources for a request to be cleared up after request is complete.

Steps To Reproduce

  1. Set up a JSS react app using node SSR rendering
  2. To exaggerate this issue and make it more obvious, have the sitecore layout API serve a large context object
  3. Hit the served site to test it's all working
  4. Using node debugging, force garbage collection and collect a memory dump at this stage, note the size
  5. Run some load against the site (We found this noticable after about ~500 requests)
  6. Using node debugging, force garbage collection and collect a memory dump, note the size
  7. Run a diff between the 2 memory dumps and you should see many new objects being retained by the SitecoreContextFactory

Possible Fix

Potentially can call unsubscribeFromContext when component is disposed (unmount?)

Your Environment

  • Sitecore Version: v9.0.2
  • JSS Version: v12.0.0

Screenshots

image_2019_10_28T13_26_01_021Z

@M1r1k
Copy link

M1r1k commented Dec 15, 2019

Hi @vtrbowman. thanks for great research :)

There is indeed a leak because of no unsubscribeFromContext on componentWillUnmount

This test proves it

import React from 'react';
import { ComponentFactory } from './sharedTypes';
const iterate = require('leakage').iterate;
import { SitecoreContext, SitecoreContextFactory } from './SitecoreContext';
import { withSitecoreContext } from '../enhancers/withSitecoreContext';
const create = require('react-test-renderer').create;

const TestNestedComponent: React.FC = (props: any) => (<div>{props.sitecoreContext && 'test'}</div>);
const TestNestedComponentWithContext: React.FC = withSitecoreContext()(TestNestedComponent as any);
const arrayOfComponents: React.Component[] = [];
const mockComponentFactory: ComponentFactory = (name: string) => arrayOfComponents[name as any];
const singleton = new SitecoreContextFactory();

describe('SitecoreContext', () => {
  it('should not leak memory', () => {
    iterate(() => {
      const wrappedComponent = create(
        <SitecoreContext componentFactory={mockComponentFactory} contextFactory={singleton}>
          <TestNestedComponentWithContext />
        </SitecoreContext>
      );

      singleton.setSitecoreContext({...singleton.context, myCustomProp: "asdfasdfasdfas"});

      wrappedComponent.unmount();
    }, {iterations:100});
  });
});

Here is heap diff for recent code
Screenshot 2019-12-15 at 01 11 25

and here when I add unsubscribe to unmount
Screenshot 2019-12-15 at 01 09 00

there still some problems or it is just my draft test, but clearly there is an improvement :)

@sc-illiakovalenko
Copy link
Contributor

@vtrbowman Fixed in commit and released in v13.0.3. Feel free to reopen issue, if you will have this problem.

sc-illiakovalenko pushed a commit that referenced this issue May 15, 2020
sc-illiakovalenko pushed a commit that referenced this issue May 15, 2020
sc-illiakovalenko pushed a commit that referenced this issue May 15, 2020
sc-illiakovalenko pushed a commit that referenced this issue May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants