Skip to content

Commit

Permalink
Fix memory leak issue with UseEffect (#1506)
Browse files Browse the repository at this point in the history 
* 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.

* Drop this comment for now. 

It's historic, not related to the code as-is. That can be represented in the git history.

* Apply suggestions on parameters naming

Co-authored-by: Tim Dorr <[email protected]>
  • Loading branch information
@larrylin28 @timdorr
larrylin28 and timdorr authored Feb 18, 2020
1 parent e649fb6 commit fa5a7fd
Showing 1 changed file with 150 additions and 96 deletions.
246 changes: 150 additions & 96 deletions src/components/connectAdvanced.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,131 @@ function storeStateUpdatesReducer(state, action) {
return [action.payload, updateCount + 1]
}

function useIsomorphicLayoutEffectWithArgs(
effectFunc,
effectArgs,
dependencies
) {
useIsomorphicLayoutEffect(() => effectFunc(...effectArgs), dependencies)
}

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(
Expand Down Expand Up @@ -281,104 +406,33 @@ 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.
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, [
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,
[
shouldHandleStateChanges,
store,
subscription,
childPropsSelector,
lastWrapperProps,
lastChildProps,
renderIsScheduled,
childPropsFromStoreUpdate,
notifyNestedSubs,
forceComponentUpdateDispatch
],
[store, subscription, childPropsSelector]
)

// 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.
Expand Down

0 comments on commit fa5a7fd

Please sign in to comment.