Bug: Memory leak with setState lambda setting to previous state

Haemoglobin commented 3 years ago

Hi,

It seems that there is a memory leak issue when using the set state hook lambda syntax, and setting the state to the previous state. The set state lambda in this scenario seems to be retained and never garbage collected.

Please ignore the contrived example, it's intentionally whittled down to the exact issue. It's quite a common requirement in our application to subscribe to an rxjs observable in a useEffect hook, then update some state when events are received (but the data received might not always result in a state change). The lambda syntax is required in the set state call because current state would otherwise always be the default state due to the closure around it in the useEffect hook. Even if there was another way to do it to avoid the issue, it's still a surprising behaviour and makes it harder to fall into the 'pit of success'.

Reproduction below:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8" />
    <title>Memory leak repro</title>
    <script src="https://unpkg.com/react@17/umd/react.development.js"></script>
    <script src="https://unpkg.com/react-dom@17/umd/react-dom.development.js"></script>

    <script src="https://unpkg.com/@babel/standalone/babel.min.js"></script>
  </head>
  <body>
    <div id="root"></div>
    <script type="text/babel">
        function App() {
          const [someState, setSomeState] = React.useState(false);
          React.useEffect(() => {
             const interval = setInterval(() => {
               const a = []; 
               for (var i = 0; i < 1000; i++) {
                 a.push({}); //produce lots of garbage objects to make the leak easier to see
               }
               // memory leak when setting state to previous state
               // set state lambda is not garbage collected and retained along with all of its captured closures
               setSomeState(state => a.length > 0 ? state : !state);

               // no memory leak when setting state to something other than previous state
               // set state lambda (and it's closures) are garbage collected as expected
               // setSomeState(state => a.length == 0 ? state : !state);
              }, 100);
             return () => clearInterval(interval);
          }, []);
          return <div>Test Memory Leak</div>;
      }

      ReactDOM.render(
        <App/>,
        document.getElementById('root')
      );

    </script>
  </body>
</html>

The best memory profiler to see this is the 'Allocation instrumentation on timeline' mode in chrome devtools.

I have confirmed this also happens using production / minified bundles so it's not a dev build issue.

Right now as a workaround I need to use 'hacks' like useRef and forceUpdate implementations which is a real shame so keen to see this fixed.

Possibly related to #21692 Many thanks!

zxh19890103 commented 3 years ago

mark

my-illusion commented 3 years ago

that's because when useEffect is triggered when your setSomeState return false, it can't make component rendered, but in react's alternate Fiber Tree, it keeps references to variables a function dispatchAction<S, A>( fiber: Fiber, queue: UpdateQueue<S, A>, action: A, ) { if (DEV) { if (typeof arguments[3] === 'function') { console.error( "State updates from the useState() and useReducer() Hooks don't support the " + 'second callback argument. To execute a side effect after ' + 'rendering, declare it in the component body with useEffect().', ); } }

const eventTime = requestEventTime(); const lane = requestUpdateLane(fiber);

const update: Update<S, A> = { lane, action, eagerReducer: null, eagerState: null, next: (null: any), };

stale[bot] commented 2 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] commented 6 months ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

facebook / react

Bug: Memory leak with setState lambda setting to previous state #21706