From b58f4c7d1e563e5fc2cd42cc80a3f0e0eafc6d2a Mon Sep 17 00:00:00 2001 From: Larry Lin Date: Tue, 21 Jan 2020 10:42:13 +0000 Subject: [PATCH 1/3] Fix memory leak issue with `UseEffect` There's a memory leak in `react-redux`'s usage of UseEffect, here's the detail: In the last useIsomorphicLayoutEffect usage in connectAdvanced.js, it returns a funcion, `unsubscribeWrapper`, which will be retained by React as `destroy` function of the effect. Since this `useEffect` only subscribes to `store`, `subscription`, `childPropsSelector`, which in most case won't change when store state updates. So this `useEffect` is never called again in following updates of `connected` component. So the effect in the `connected` component will always keep a reference to the `unsubscribeWrapper` created in first call. But this will lead to a memory leak in modern JS VM. In modern JS VM such as V8(Chrome), the instance of function `unsubscribeWrapper` will retain a closure for context when it's created. Which means, all local variables in first call of each `connected` component will be retained by this instance of `unsubscribeWrapper`, even though they are not used by it at all. In this case, the context includes `actualChildProps`. Which means, every connected component, will in the end retain 2 copy of its props in the memory, one as it's current prop, another is the first version of its props. It can be huge impact of memory if a connected component has props retaining a reference to big chunk of data in store state that can be fully updated to another version(e.g. data parsed from cache, network repsonse, etc). It will end up always retaining 2 copy of that data in memory, or more if there're more other `connected` compoents. A better JS VM should optimize to not include unused variable in the closure, but as I tested in V8 and Hermes, they both doesn't have such optimisation to avoid this case. This can be easy reproduced: 1. Have a connected component, reference one object in the store state. 2. Update the store state(add a version maker in the object to help identify the issue) 3. Use Chrome dev tools to take a heap snapshot. 4. Search for the object in the heap snapshot, you will find 2 version of it in the heap, one retained by connected wrapped component's props, one retained by a `destroy` in lastEffect of conneted compoents. By communicating with React community, a good solution suggested is to lift `useEffect` outside of the hook component in such kind of case. And this is how this PR solve the problem. --- src/components/connectAdvanced.js | 244 ++++++++++++++++++------------ 1 file changed, 148 insertions(+), 96 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index ec9deefc9..82edaecee 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -23,6 +23,127 @@ function storeStateUpdatesReducer(state, action) { return [action.payload, updateCount + 1] } +function useIsomorphicLayoutEffectWithArgs(create, deps, args) { + useIsomorphicLayoutEffect(() => create(...args), deps) +} + +function captureWrapperProps( + lastWrapperProps, + lastChildProps, + renderIsScheduled, + wrapperProps, + actualChildProps, + childPropsFromStoreUpdate, + notifyNestedSubs +) { + // We want to capture the wrapper props and child props we used for later comparisons + lastWrapperProps.current = wrapperProps + lastChildProps.current = actualChildProps + renderIsScheduled.current = false + + // If the render was from a store update, clear out that reference and cascade the subscriber update + if (childPropsFromStoreUpdate.current) { + childPropsFromStoreUpdate.current = null + notifyNestedSubs() + } +} + +function subscribeUpdates( + shouldHandleStateChanges, + store, + subscription, + childPropsSelector, + lastWrapperProps, + lastChildProps, + renderIsScheduled, + childPropsFromStoreUpdate, + notifyNestedSubs, + forceComponentUpdateDispatch +) { + // If we're not subscribed to the store, nothing to do here + if (!shouldHandleStateChanges) return + + // Capture values for checking if and when this component unmounts + let didUnsubscribe = false + let lastThrownError = null + + // We'll run this callback every time a store subscription update propagates to this component + const checkForUpdates = () => { + if (didUnsubscribe) { + // Don't run stale listeners. + // Redux doesn't guarantee unsubscriptions happen until next dispatch. + return + } + + const latestStoreState = store.getState() + + let newChildProps, error + try { + // Actually run the selector with the most recent store state and wrapper props + // to determine what the child props should be + newChildProps = childPropsSelector( + latestStoreState, + lastWrapperProps.current + ) + } catch (e) { + error = e + lastThrownError = e + } + + if (!error) { + lastThrownError = null + } + + // If the child props haven't changed, nothing to do here - cascade the subscription update + if (newChildProps === lastChildProps.current) { + if (!renderIsScheduled.current) { + notifyNestedSubs() + } + } else { + // Save references to the new child props. Note that we track the "child props from store update" + // as a ref instead of a useState/useReducer because we need a way to determine if that value has + // been processed. If this went into useState/useReducer, we couldn't clear out the value without + // forcing another re-render, which we don't want. + lastChildProps.current = newChildProps + childPropsFromStoreUpdate.current = newChildProps + renderIsScheduled.current = true + + // If the child props _did_ change (or we caught an error), this wrapper component needs to re-render + forceComponentUpdateDispatch({ + type: 'STORE_UPDATED', + payload: { + error + } + }) + } + } + + // Actually subscribe to the nearest connected ancestor (or store) + subscription.onStateChange = checkForUpdates + subscription.trySubscribe() + + // Pull data from the store after first render in case the store has + // changed since we began. + checkForUpdates() + + const unsubscribeWrapper = () => { + didUnsubscribe = true + subscription.tryUnsubscribe() + subscription.onStateChange = null + + if (lastThrownError) { + // It's possible that we caught an error due to a bad mapState function, but the + // parent re-rendered without this component and we're about to unmount. + // This shouldn't happen as long as we do top-down subscriptions correctly, but + // if we ever do those wrong, this throw will surface the error in our tests. + // In that case, throw the error from here so it doesn't get lost. + throw lastThrownError + } + } + + return unsubscribeWrapper +} + const initStateUpdates = () => [null, 0] export default function connectAdvanced( @@ -278,107 +399,38 @@ export default function connectAdvanced( return childPropsSelector(store.getState(), wrapperProps) }, [store, previousStateUpdateResult, wrapperProps]) + // To avoid memory leak issue of function closure in useEffect, move useEffect functions out of component scope. + // We need this to execute synchronously every time we re-render. However, React warns // about useLayoutEffect in SSR, so we try to detect environment and fall back to // just useEffect instead to avoid the warning, since neither will run anyway. - useIsomorphicLayoutEffect(() => { - // We want to capture the wrapper props and child props we used for later comparisons - lastWrapperProps.current = wrapperProps - lastChildProps.current = actualChildProps - renderIsScheduled.current = false - - // If the render was from a store update, clear out that reference and cascade the subscriber update - if (childPropsFromStoreUpdate.current) { - childPropsFromStoreUpdate.current = null - notifyNestedSubs() - } - }) + useIsomorphicLayoutEffectWithArgs(captureWrapperProps, undefined, [ + lastWrapperProps, + lastChildProps, + renderIsScheduled, + wrapperProps, + actualChildProps, + childPropsFromStoreUpdate, + notifyNestedSubs + ]) // Our re-subscribe logic only runs when the store/subscription setup changes - useIsomorphicLayoutEffect(() => { - // If we're not subscribed to the store, nothing to do here - if (!shouldHandleStateChanges) return - - // Capture values for checking if and when this component unmounts - let didUnsubscribe = false - let lastThrownError = null - - // We'll run this callback every time a store subscription update propagates to this component - const checkForUpdates = () => { - if (didUnsubscribe) { - // Don't run stale listeners. - // Redux doesn't guarantee unsubscriptions happen until next dispatch. - return - } - - const latestStoreState = store.getState() - - let newChildProps, error - try { - // Actually run the selector with the most recent store state and wrapper props - // to determine what the child props should be - newChildProps = childPropsSelector( - latestStoreState, - lastWrapperProps.current - ) - } catch (e) { - error = e - lastThrownError = e - } - - if (!error) { - lastThrownError = null - } - - // If the child props haven't changed, nothing to do here - cascade the subscription update - if (newChildProps === lastChildProps.current) { - if (!renderIsScheduled.current) { - notifyNestedSubs() - } - } else { - // Save references to the new child props. Note that we track the "child props from store update" - // as a ref instead of a useState/useReducer because we need a way to determine if that value has - // been processed. If this went into useState/useReducer, we couldn't clear out the value without - // forcing another re-render, which we don't want. - lastChildProps.current = newChildProps - childPropsFromStoreUpdate.current = newChildProps - renderIsScheduled.current = true - - // If the child props _did_ change (or we caught an error), this wrapper component needs to re-render - forceComponentUpdateDispatch({ - type: 'STORE_UPDATED', - payload: { - error - } - }) - } - } - - // Actually subscribe to the nearest connected ancestor (or store) - subscription.onStateChange = checkForUpdates - subscription.trySubscribe() - - // Pull data from the store after first render in case the store has - // changed since we began. - checkForUpdates() - - const unsubscribeWrapper = () => { - didUnsubscribe = true - subscription.tryUnsubscribe() - subscription.onStateChange = null - - if (lastThrownError) { - // It's possible that we caught an error due to a bad mapState function, but the - // parent re-rendered without this component and we're about to unmount. - // This shouldn't happen as long as we do top-down subscriptions correctly, but - // if we ever do those wrong, this throw will surface the error in our tests. - // In that case, throw the error from here so it doesn't get lost. - throw lastThrownError - } - } - - return unsubscribeWrapper - }, [store, subscription, childPropsSelector]) + useIsomorphicLayoutEffectWithArgs( + subscribeUpdates, + [store, subscription, childPropsSelector], + [ + shouldHandleStateChanges, + store, + subscription, + childPropsSelector, + lastWrapperProps, + lastChildProps, + renderIsScheduled, + childPropsFromStoreUpdate, + notifyNestedSubs, + forceComponentUpdateDispatch + ] + ) // Now that all that's done, we can finally try to actually render the child component. // We memoize the elements for the rendered child component as an optimization. From eb79ce31e629537e24db77d64a6a0334d0b45279 Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Tue, 21 Jan 2020 09:51:18 -0500 Subject: [PATCH 2/3] Drop this comment for now. It's historic, not related to the code as-is. That can be represented in the git history. --- src/components/connectAdvanced.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 82edaecee..264e2df7a 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -399,8 +399,6 @@ export default function connectAdvanced( return childPropsSelector(store.getState(), wrapperProps) }, [store, previousStateUpdateResult, wrapperProps]) - // To avoid memory leak issue of function closure in useEffect, move useEffect functions out of component scope. - // We need this to execute synchronously every time we re-render. However, React warns // about useLayoutEffect in SSR, so we try to detect environment and fall back to // just useEffect instead to avoid the warning, since neither will run anyway. From 54c65dffcaf8a988e1f266335d422b97f06ba5c1 Mon Sep 17 00:00:00 2001 From: Larry Lin Date: Wed, 22 Jan 2020 16:48:26 +0000 Subject: [PATCH 3/3] Apply suggestions on parameters naming --- src/components/connectAdvanced.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 264e2df7a..f6feb738f 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -23,8 +23,12 @@ function storeStateUpdatesReducer(state, action) { return [action.payload, updateCount + 1] } -function useIsomorphicLayoutEffectWithArgs(create, deps, args) { - useIsomorphicLayoutEffect(() => create(...args), deps) +function useIsomorphicLayoutEffectWithArgs( + effectFunc, + effectArgs, + dependencies +) { + useIsomorphicLayoutEffect(() => effectFunc(...effectArgs), dependencies) } function captureWrapperProps( @@ -402,7 +406,7 @@ export default function connectAdvanced( // We need this to execute synchronously every time we re-render. However, React warns // about useLayoutEffect in SSR, so we try to detect environment and fall back to // just useEffect instead to avoid the warning, since neither will run anyway. - useIsomorphicLayoutEffectWithArgs(captureWrapperProps, undefined, [ + useIsomorphicLayoutEffectWithArgs(captureWrapperProps, [ lastWrapperProps, lastChildProps, renderIsScheduled, @@ -415,7 +419,6 @@ export default function connectAdvanced( // Our re-subscribe logic only runs when the store/subscription setup changes useIsomorphicLayoutEffectWithArgs( subscribeUpdates, - [store, subscription, childPropsSelector], [ shouldHandleStateChanges, store, @@ -427,7 +430,8 @@ export default function connectAdvanced( childPropsFromStoreUpdate, notifyNestedSubs, forceComponentUpdateDispatch - ] + ], + [store, subscription, childPropsSelector] ) // Now that all that's done, we can finally try to actually render the child component.