redux-toolkit icon indicating copy to clipboard operation
redux-toolkit copied to clipboard

Memory leak during SSR RTK request after upgrading from 1.9.5 -> ^1.9.6

Open StopNGo opened this issue 1 year ago • 24 comments

Hello!

So, I'm using this approach for server side prefetching data: https://redux-toolkit.js.org/rtk-query/usage/server-side-rendering#server-side-rendering-elsewhere

const createApi = buildCreateApi(
  coreModule(),
  reactHooksModule({ unstable__sideEffectsInRender: true })
)

....

store.dispatch(api.endpoints.getSomethingById.initiate(1))

await Promise.all(
  store.dispatch(api.util.getRunningQueriesThunk())
)

preloadedState = { ...store.getState() }

Before upgrading to RTK v2 and Redux 5 this fills store with specific query and subscription:

api: {
    queries: {
      'getSomethingById(1)': {
        status: 'fulfilled',
       ...
    },
    mutations: {},
    provided: {},
    subscriptions: {
      'getSomethingById(1)': {
        ...
      }
    },
    config: {
      ...
    }
  }
}

After upgrading it looks like:

api: {
    queries: {
      'getSomethingById(1)': {
        status: 'fulfilled',
       ...
    },
    mutations: {},
    provided: {},
    subscriptions: {},
    config: {
      ...
    }
  }
}

Subscription is empty.

Also, such server side prefetching leads now to memory leak - server memory consumption grows, however before upgrading there was no such problem on server.

Thanks

StopNGo avatar Dec 17 '23 12:12 StopNGo

I think you have a misconception of the term "subscription" here - the term "subscription" means "hey, something is still using that and you should definitely not clear it out of memory". If you were rehydrating subscriptions created on the server in the browser, you would have subscriptions for component that could never signal "hey, I don't need this anymore" since they don't live in the browser - those would be held onto forever, and never be cache-collected. That's why extractRehydrationInfo will never rehydrate subscriptions, but only query contents.

As for a memory leak on the server: you are aware that you should create a new Redux store for every single server-side-render, and throw it away afterwards? No matter how many contents you have in that Redux store: as you dispose of the whole store, that should never cause any rise in memory usage.

With all that added knowledge, can you explain a little bit more what you are doing here and why you think you see an increase of memory usage because of subscriptions?

phryneas avatar Dec 17 '23 15:12 phryneas

Thanks for the answer!

  • As for subscription: so, I just noticed the difference, before upgrading the subscription was filled on servers' side, now - not. You changed something to optimize this moment?
  • As for the memory leak: You can check full code here: https://github.com/StopNGo/react-proto/blob/main/src/server/middlewares/serverRenderer.tsx Before RTK upgrade there was no memory leak on server. After upgrade I noticed the memory leak. If I comment a line with data prefetching (59: await apiRequest(store)) the memory leak problem disappears.

StopNGo avatar Dec 17 '23 16:12 StopNGo

I don't really see you following the advice in our docs:

  • you don't call store.dispatch(api.util.resetApiState()), which will cause lingering promises for a while (which would explain data leaking on the server - it will be cleaned up, but only after 60 seconds.
  • you don't use extractRehydrationInfo, which will lead to memory leaks in the browser if you accidentally move subscriptions over.

phryneas avatar Dec 17 '23 16:12 phryneas

  • It does not change anything. Before upgrade without resetting api state there was no memory leak on server. After upgrade it is even if I reset api state.
  • I do not use NextJS and I have no memory leak in the browser, only on server. How extractRehydrationInfo help me, especially if before upgrade all was ok without this option.

StopNGo avatar Dec 17 '23 16:12 StopNGo

  • extractRehydrationInfo has nothing to do with Next.js, you should use it with persistence or any other kind of (not only SSR) hydration as well, which you have to do with an action, not by using initialState. (Also see persistence and rehydration).

  • It does not change anything. Before upgrade without resetting api state there was no memory leak on server. After upgrade it is even if I reset api state.

If that is the case, it has probably nothing whatsoever to do with subscriptions. From what version are you updating, and if you take a heap snapshot, can you identify what exactly is leaking here?

phryneas avatar Dec 17 '23 16:12 phryneas

  • But I'm not using any hydration for a moment - Server: create store - make api request - render on server' side with the state with released query - send html to client with preloaded state with released query (window.PRELOADED_STATE). Before upgrade all was ok, now it leads to leak.

Here is server snapshot comparisons - before and after 1000 requests to server: pd9SGl

I am not so good in snapshot analyzing, so maybe I can not identify exact source of leaking, but, again:

  • before upgrade there was no such picture on server, whatever number of requests to the server keeps memory around 25MB
  • after upgrade see attached picture
  • if I do no api requests on the server - all is good again

Upgrade: @reduxjs/toolkit: 1.9.3 -> 2.0.1 redux: 4.2.1 -> 5.0.0

StopNGo avatar Dec 17 '23 17:12 StopNGo

Could you maybe switch to 1.9.7 so we see if this problem started occuring within the 1.9 line or with the 2.0 upgrade?

As for the snapshots - up where it says "summary" you should be able to select "compare" and then it shows you a diff between the two snapshots, what grew etc..

phryneas avatar Dec 17 '23 18:12 phryneas

So, yes, I have found, that with 1.9.7 memory also leaks.

If it is not too intrusive, can I send you snapshots for comparison? I afraid, that I can not do proper comparison.

StopNGo avatar Dec 17 '23 19:12 StopNGo

And leaking starts from 1.9.6: 1.9.5 - no memory leak 1.9.6 - memory leaks

StopNGo avatar Dec 17 '23 19:12 StopNGo

I can't guarantee you that I can read anything from the snapshots, but please send them, maybe we get lucky :) [email protected]

phryneas avatar Dec 17 '23 21:12 phryneas

I just went over all code that changed in 1.9.6, and I can't see anything suspcious. I know it's a lot, but could I ask you to git bisect for the change that caused the memory leak?

cd packages/toolkit
yarn install
git checkout v1.9.6
git bisect start
git bisect bad
git bisect good v1.9.5
# will check you out at some revision now -  you will repeat from here later
yarn build
yalc publish
# now in your reproduction:
yalc add@reduxjs/toolkit 
#or later, 
yalc update my-package
# test for reproduction

# if memory leak occured
git bisect bad
# if no memory leak
git bisect good

# repeat from above

phryneas avatar Dec 17 '23 21:12 phryneas

No problem, will do it ASAP. Thanks!

StopNGo avatar Dec 17 '23 22:12 StopNGo

Hi! So, I have found a problematic commit: https://github.com/reduxjs/redux-toolkit/issues/3716 (56ac6133c94fb71f259eaeee912410defc16f24b)

Before this commit - no memory leak After - memory leaks

Change from:

    const request = new Request(url, config)
    const requestClone = request.clone()

to

    const request = new Request(url, config)
    const requestClone = new Request(url, config)

I think, object (or part of it) from the second new Request stays in memory and request.clone() works differently.

I took v2.0.1, changed this line back, built package and tested in my case and there was no memory leak on my server.

This commit, as I understand, fixed some bug in Chromium but leads to memory leak. So, I do not know what is better for you, but I hope you can fix both issues.

Thanks!

StopNGo avatar Dec 20 '23 23:12 StopNGo

Hi again! Just want to check if my last message was clear and I want to know your thoughts about this issue. Thanks!

StopNGo avatar Jan 04 '24 22:01 StopNGo

Yeah, it was clear, sorry for not coming back to you - Christmas happened :)

I'll try to take a look at this, but there's a lot going on right now, so I can't give you a specific timeline on this, I'm sorry. I hope you could patch this locally for now?

phryneas avatar Jan 04 '24 22:01 phryneas

It's ok :) Happy belated new year! Just clarified for myself. Yes, local patching is not a problem for me. May be I will try to offer some global solution, but I'm not sure in my skills :) Thanks for your afforts!

StopNGo avatar Jan 04 '24 22:01 StopNGo

Thanks for looking into this. I just wanted to notify you that we went through the same process and also identified https://github.com/reduxjs/redux-toolkit/issues/3716 as the root-cause of a memory leak. Patching it back to the original code resulted in the disappearance of the leak.

Caspervw avatar Jan 11 '24 13:01 Caspervw

@phryneas Could I ask where I should call store.dispatch(api.util.resetApiState()) in a NextJS page? FYI, I am using RTK Query and next-redux-wrapper for SSR. Here is what in my getServerSideProps:

export const getServerSideProps = wrapper.getServerSideProps(
  (store) => async (context) => {

    const res = await store.dispatch(
      getData.initiate({
        ...params,
      })
    );

    if (res.status === QueryStatus.fulfilled) {
      await Promise.all(store.dispatch(getRunningQueriesThunk()));
      return {
        props: {},
      };
    }

    return {
      notFound: true,
    };
  }
);

Hoping for a quick response :D Many thanks!

truongle-teq avatar Feb 06 '24 13:02 truongle-teq

I have an unrelated question @StopNGo. In this example (or in your current implementation), are you fetching data and already using in in the ss rendering? I am trying to figure out if I am able to fetch data in the RSCs of nextjs and then pass it to a redux client component and rehydrate the store.

Rodrigo-JM avatar Mar 20 '24 12:03 Rodrigo-JM