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
Bug: Stale selectors keep old store referenced and in memory #1981
Comments
This was referenced Jan 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What version of React, ReactDOM/React Native, Redux, and React Redux are you using?
What is the current behavior?
I've been cracking down a memory leak together with some colleagues for some time and it seems that there is a problem introduced with the usage of
useSyncExternalStoreWithSelector
.This function accepts the following arguments:
React-redux provides the
getSnapshot
with thestore.getState
function, which makes sense since that is the base of your selector input.Within the
useSyncExternalStoreWithSelector
the selector will get memoized based on thegetSnapshot
,getServerSnapshot
,selector
andisEqual
function.The getSnapshot function doesn't change (the getServerSnapshot neither) and when using the default isEqual that also won't change. The selector is something that could change if that is provided inline, but when not providing this inline, but by importing this from say another file. This function also includes memoizing the result of the
getSnapshot
function, which means the entire state.Now this would be perfectly fine if this would be the last version of the redux-store, but it does not always trigger an update!
Looking at this snippet from the useSyncExternalStoreWithSelector, there are a few checks in place to prevent useless renders from being triggered.
First it will check if the snapshot has changed, since redux is immutable, it will be different if the store has changed applied.
Then it will execute the selector and it will compare the selectors result with the previous result.
if this does not change, it will short circuit and it will not update memoizedSnapshot, keeping the reference to the previous version
Snippet from here: https://github.com/facebook/react/blob/main/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js#L89
So why is this a problem? For any selectors that take a part of the store that don't change at all, this means it will not update the memoizedSnapshot to the latest redux store. This becomes a bigger problem if you have plenty of updates and dynamically add additional components that use a stale selector. In our application with a fairly large store this results in Out of Memory problems. I would expect this could happen for other as well, depending on the size of their store and the amount of stale selectors in combination of dynamically added components.
A working reproduction can be found here, including steps how to get there.
https://github.com/nickbosland/memory-leak-reproduction/tree/reproduction-without-react-window
Why do I report it here? I'm not 100% sure if this because of how react-redux uses the useSyncExternalStoreWithSelector api or that it is actually caused by a bug in that package itself, but it does concern users of react-redux either way.
With my limited knowledge of the external store api, I would say that the update on memoizedSnapshot should happen before the second check where the selector result is compared.
I did find a PR that actually changed the location of the memoizedSnapshot update from what I expect to be logical, but it doesn't contain too much context on why this change was done. I'll make a report within the react repo as well and link the issues. (The PR: facebook/react#22500)
What is the expected behavior?
I would expect that stale selectors (in where the result never changes) shouldn't keep old versions of the redux store alive.
Which browser and OS are affected by this issue?
Not that relevant, but tested in chromium on linux / windows
Did this work in previous versions of React Redux?
The text was updated successfully, but these errors were encountered: