Skip to content

Commit ae2bebe

Browse files
larrylin28timdorr
larrylin28
and
timdorr
authored
Fix memory leak issue with UseEffect (reduxjs#1506)
* 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]>
1 parent 512a318 commit ae2bebe

File tree

1 file changed

+150
-96
lines changed

1 file changed

+150
-96
lines changed

‎src/components/connectAdvanced.js

+150-96
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,131 @@ function storeStateUpdatesReducer(state, action) {
2323
return [action.payload, updateCount + 1]
2424
}
2525

26+
function useIsomorphicLayoutEffectWithArgs(
27+
effectFunc,
28+
effectArgs,
29+
dependencies
30+
) {
31+
useIsomorphicLayoutEffect(() => effectFunc(...effectArgs), dependencies)
32+
}
33+
34+
function captureWrapperProps(
35+
lastWrapperProps,
36+
lastChildProps,
37+
renderIsScheduled,
38+
wrapperProps,
39+
actualChildProps,
40+
childPropsFromStoreUpdate,
41+
notifyNestedSubs
42+
) {
43+
// We want to capture the wrapper props and child props we used for later comparisons
44+
lastWrapperProps.current = wrapperProps
45+
lastChildProps.current = actualChildProps
46+
renderIsScheduled.current = false
47+
48+
// If the render was from a store update, clear out that reference and cascade the subscriber update
49+
if (childPropsFromStoreUpdate.current) {
50+
childPropsFromStoreUpdate.current = null
51+
notifyNestedSubs()
52+
}
53+
}
54+
55+
function subscribeUpdates(
56+
shouldHandleStateChanges,
57+
store,
58+
subscription,
59+
childPropsSelector,
60+
lastWrapperProps,
61+
lastChildProps,
62+
renderIsScheduled,
63+
childPropsFromStoreUpdate,
64+
notifyNestedSubs,
65+
forceComponentUpdateDispatch
66+
) {
67+
// If we're not subscribed to the store, nothing to do here
68+
if (!shouldHandleStateChanges) return
69+
70+
// Capture values for checking if and when this component unmounts
71+
let didUnsubscribe = false
72+
let lastThrownError = null
73+
74+
// We'll run this callback every time a store subscription update propagates to this component
75+
const checkForUpdates = () => {
76+
if (didUnsubscribe) {
77+
// Don't run stale listeners.
78+
// Redux doesn't guarantee unsubscriptions happen until next dispatch.
79+
return
80+
}
81+
82+
const latestStoreState = store.getState()
83+
84+
let newChildProps, error
85+
try {
86+
// Actually run the selector with the most recent store state and wrapper props
87+
// to determine what the child props should be
88+
newChildProps = childPropsSelector(
89+
latestStoreState,
90+
lastWrapperProps.current
91+
)
92+
} catch (e) {
93+
error = e
94+
lastThrownError = e
95+
}
96+
97+
if (!error) {
98+
lastThrownError = null
99+
}
100+
101+
// If the child props haven't changed, nothing to do here - cascade the subscription update
102+
if (newChildProps === lastChildProps.current) {
103+
if (!renderIsScheduled.current) {
104+
notifyNestedSubs()
105+
}
106+
} else {
107+
// Save references to the new child props. Note that we track the "child props from store update"
108+
// as a ref instead of a useState/useReducer because we need a way to determine if that value has
109+
// been processed. If this went into useState/useReducer, we couldn't clear out the value without
110+
// forcing another re-render, which we don't want.
111+
lastChildProps.current = newChildProps
112+
childPropsFromStoreUpdate.current = newChildProps
113+
renderIsScheduled.current = true
114+
115+
// If the child props _did_ change (or we caught an error), this wrapper component needs to re-render
116+
forceComponentUpdateDispatch({
117+
type: 'STORE_UPDATED',
118+
payload: {
119+
error
120+
}
121+
})
122+
}
123+
}
124+
125+
// Actually subscribe to the nearest connected ancestor (or store)
126+
subscription.onStateChange = checkForUpdates
127+
subscription.trySubscribe()
128+
129+
// Pull data from the store after first render in case the store has
130+
// changed since we began.
131+
checkForUpdates()
132+
133+
const unsubscribeWrapper = () => {
134+
didUnsubscribe = true
135+
subscription.tryUnsubscribe()
136+
subscription.onStateChange = null
137+
138+
if (lastThrownError) {
139+
// It's possible that we caught an error due to a bad mapState function, but the
140+
// parent re-rendered without this component and we're about to unmount.
141+
// This shouldn't happen as long as we do top-down subscriptions correctly, but
142+
// if we ever do those wrong, this throw will surface the error in our tests.
143+
// In that case, throw the error from here so it doesn't get lost.
144+
throw lastThrownError
145+
}
146+
}
147+
148+
return unsubscribeWrapper
149+
}
150+
26151
const initStateUpdates = () => [null, 0]
27152

28153
export default function connectAdvanced(
@@ -281,104 +406,33 @@ export default function connectAdvanced(
281406
// We need this to execute synchronously every time we re-render. However, React warns
282407
// about useLayoutEffect in SSR, so we try to detect environment and fall back to
283408
// just useEffect instead to avoid the warning, since neither will run anyway.
284-
useIsomorphicLayoutEffect(() => {
285-
// We want to capture the wrapper props and child props we used for later comparisons
286-
lastWrapperProps.current = wrapperProps
287-
lastChildProps.current = actualChildProps
288-
renderIsScheduled.current = false
289-
290-
// If the render was from a store update, clear out that reference and cascade the subscriber update
291-
if (childPropsFromStoreUpdate.current) {
292-
childPropsFromStoreUpdate.current = null
293-
notifyNestedSubs()
294-
}
295-
})
409+
useIsomorphicLayoutEffectWithArgs(captureWrapperProps, [
410+
lastWrapperProps,
411+
lastChildProps,
412+
renderIsScheduled,
413+
wrapperProps,
414+
actualChildProps,
415+
childPropsFromStoreUpdate,
416+
notifyNestedSubs
417+
])
296418

297419
// Our re-subscribe logic only runs when the store/subscription setup changes
298-
useIsomorphicLayoutEffect(() => {
299-
// If we're not subscribed to the store, nothing to do here
300-
if (!shouldHandleStateChanges) return
301-
302-
// Capture values for checking if and when this component unmounts
303-
let didUnsubscribe = false
304-
let lastThrownError = null
305-
306-
// We'll run this callback every time a store subscription update propagates to this component
307-
const checkForUpdates = () => {
308-
if (didUnsubscribe) {
309-
// Don't run stale listeners.
310-
// Redux doesn't guarantee unsubscriptions happen until next dispatch.
311-
return
312-
}
313-
314-
const latestStoreState = store.getState()
315-
316-
let newChildProps, error
317-
try {
318-
// Actually run the selector with the most recent store state and wrapper props
319-
// to determine what the child props should be
320-
newChildProps = childPropsSelector(
321-
latestStoreState,
322-
lastWrapperProps.current
323-
)
324-
} catch (e) {
325-
error = e
326-
lastThrownError = e
327-
}
328-
329-
if (!error) {
330-
lastThrownError = null
331-
}
332-
333-
// If the child props haven't changed, nothing to do here - cascade the subscription update
334-
if (newChildProps === lastChildProps.current) {
335-
if (!renderIsScheduled.current) {
336-
notifyNestedSubs()
337-
}
338-
} else {
339-
// Save references to the new child props. Note that we track the "child props from store update"
340-
// as a ref instead of a useState/useReducer because we need a way to determine if that value has
341-
// been processed. If this went into useState/useReducer, we couldn't clear out the value without
342-
// forcing another re-render, which we don't want.
343-
lastChildProps.current = newChildProps
344-
childPropsFromStoreUpdate.current = newChildProps
345-
renderIsScheduled.current = true
346-
347-
// If the child props _did_ change (or we caught an error), this wrapper component needs to re-render
348-
forceComponentUpdateDispatch({
349-
type: 'STORE_UPDATED',
350-
payload: {
351-
error
352-
}
353-
})
354-
}
355-
}
356-
357-
// Actually subscribe to the nearest connected ancestor (or store)
358-
subscription.onStateChange = checkForUpdates
359-
subscription.trySubscribe()
360-
361-
// Pull data from the store after first render in case the store has
362-
// changed since we began.
363-
checkForUpdates()
364-
365-
const unsubscribeWrapper = () => {
366-
didUnsubscribe = true
367-
subscription.tryUnsubscribe()
368-
subscription.onStateChange = null
369-
370-
if (lastThrownError) {
371-
// It's possible that we caught an error due to a bad mapState function, but the
372-
// parent re-rendered without this component and we're about to unmount.
373-
// This shouldn't happen as long as we do top-down subscriptions correctly, but
374-
// if we ever do those wrong, this throw will surface the error in our tests.
375-
// In that case, throw the error from here so it doesn't get lost.
376-
throw lastThrownError
377-
}
378-
}
379-
380-
return unsubscribeWrapper
381-
}, [store, subscription, childPropsSelector])
420+
useIsomorphicLayoutEffectWithArgs(
421+
subscribeUpdates,
422+
[
423+
shouldHandleStateChanges,
424+
store,
425+
subscription,
426+
childPropsSelector,
427+
lastWrapperProps,
428+
lastChildProps,
429+
renderIsScheduled,
430+
childPropsFromStoreUpdate,
431+
notifyNestedSubs,
432+
forceComponentUpdateDispatch
433+
],
434+
[store, subscription, childPropsSelector]
435+
)
382436

383437
// Now that all that's done, we can finally try to actually render the child component.
384438
// We memoize the elements for the rendered child component as an optimization.

0 commit comments

Comments
 (0)