Repository: reduxjs/redux-toolkit

Problem statement:
RTK Query: Subscription leak in React using useQuery() with skip parameter
There seems to be a subscription leak in RTK Query (1.9.2) with React (18.2). 

When using useQuery / useQuerySubscription with the skip parameter, some of the subscriptions randomly won't go away when the component/hook unmounts or the args change. This leak is very common in our application and causes issues with some of our code in the onCacheEntryAdded endpoint lifecycle method, as its lifecycle is never ended for the leaked subscriptions.

I managed to reproduce the leak in the pokemon example sandbox by brute forcing different subscription mounts on the same endpoint (with keepUnusedDataFor=1) while toggling the skip parameter on/off at a 10ms interval. By pressing the mount button in the sandbox you can initiate the brute force action and watch the active list of subscriptions. You can see that some of the subscriptions will never go away and have become ghost subscriptions. (Of course I turned off the api calls to not ddos the pokemon database.)

Obviously the sandbox is not a real world use case, but it shows there's something wrong. In our own application it happens by simply having many different endpoints in a complex/busy react environment. However it's worth noting that we can somehow fix it by turning off the autoBatchEnhancer, unsure why...

[![Edit dry-river-ltbu89](https://codesandbox.io/static/img/play-codesandbox.svg)](https://codesandbox.io/s/dry-river-ltbu89?fontsize=14&hidenavigation=1&theme=dark)

![subscription leak](https://user-images.githubusercontent.com/9107835/219405158-ade8cdfa-c8c9-4ecd-8327-93a85ea86cec.gif)



Hints:
Interesting! Don't immediately have an idea why this would be happening, but thanks for the report and the repro!
Thanks for the quick reply! I'm actually a little bit unsure whether this is the actual cause of the subscription leakage in our specific situation, as it is fixed by disabling the autoBatchEnhancer. However the conditions and the result is exactly the same so it is related at least.

Patch:
diff --git a/packages/toolkit/src/query/react/buildHooks.ts b/packages/toolkit/src/query/react/buildHooks.ts
--- a/packages/toolkit/src/query/react/buildHooks.ts
+++ b/packages/toolkit/src/query/react/buildHooks.ts
@@ -626,9 +626,7 @@ export function buildHooks<Definitions extends EndpointDefinitions>({
       )
         lastResult = undefined
     }
-    if (queryArgs === skipToken) {
-      lastResult = undefined
-    }
+
     // data is the last known good request result we have tracked - or if none has been tracked yet the last good result for the current args
     let data = currentState.isSuccess ? currentState.data : lastResult?.data
     if (data === undefined) data = currentState.data
@@ -742,7 +740,9 @@ export function buildHooks<Definitions extends EndpointDefinitions>({
       })
 
       usePossiblyImmediateEffect((): void | undefined => {
-        promiseRef.current = undefined
+        if (subscriptionRemoved) {
+          promiseRef.current = undefined
+        }
       }, [subscriptionRemoved])
 
       usePossiblyImmediateEffect((): void | undefined => {


Test patch:
diff --git a/packages/toolkit/src/query/tests/buildHooks.test.tsx b/packages/toolkit/src/query/tests/buildHooks.test.tsx
--- a/packages/toolkit/src/query/tests/buildHooks.test.tsx
+++ b/packages/toolkit/src/query/tests/buildHooks.test.tsx
@@ -9,7 +9,14 @@ import {
   QueryStatus,
   skipToken,
 } from '@reduxjs/toolkit/query/react'
-import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'
+import {
+  act,
+  fireEvent,
+  render,
+  screen,
+  waitFor,
+  renderHook,
+} from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
 import { rest } from 'msw'
 import {
@@ -27,7 +34,6 @@ import type { AnyAction } from 'redux'
 import type { SubscriptionOptions } from '@reduxjs/toolkit/dist/query/core/apiState'
 import type { SerializedError } from '@reduxjs/toolkit'
 import { createListenerMiddleware, configureStore } from '@reduxjs/toolkit'
-import { renderHook } from '@testing-library/react'
 import { delay } from '../../utils'
 
 // Just setup a temporary in-memory counter for tests that `getIncrementedAmount`.
@@ -714,6 +720,94 @@ describe('hooks tests', () => {
       expect(res.data!.amount).toBeGreaterThan(originalAmount)
     })
 
+    // See https://github.com/reduxjs/redux-toolkit/issues/3182
+    test('Hook subscriptions are properly cleaned up when changing skip back and forth', async () => {
+      const pokemonApi = createApi({
+        baseQuery: fetchBaseQuery({ baseUrl: 'https://pokeapi.co/api/v2/' }),
+        endpoints: (builder) => ({
+          getPokemonByName: builder.query({
+            queryFn: (name: string) => ({ data: null }),
+            keepUnusedDataFor: 1,
+          }),
+        }),
+      })
+
+      const storeRef = setupApiStore(pokemonApi, undefined, {
+        withoutTestLifecycles: true,
+      })
+
+      const getSubscriptions = () => storeRef.store.getState().api.subscriptions
+
+      const checkNumSubscriptions = (arg: string, count: number) => {
+        const subscriptions = getSubscriptions()
+        const cacheKeyEntry = subscriptions[arg]
+
+        if (cacheKeyEntry) {
+          expect(Object.values(cacheKeyEntry).length).toBe(count)
+        }
+      }
+
+      // 1) Initial state: an active subscription
+      const { result, rerender, unmount } = renderHook(
+        ([arg, options]: Parameters<
+          typeof pokemonApi.useGetPokemonByNameQuery
+        >) => pokemonApi.useGetPokemonByNameQuery(arg, options),
+        {
+          wrapper: storeRef.wrapper,
+          initialProps: ['a'],
+        }
+      )
+
+      await act(async () => {
+        await delay(1)
+      })
+
+      // 2) Set the current subscription to `{skip: true}
+      await act(async () => {
+        rerender(['a', { skip: true }])
+      })
+
+      // 3) Change _both_ the cache key _and_ `{skip: false}` at the same time.
+      // This causes the `subscriptionRemoved` check to be `true`.
+      await act(async () => {
+        rerender(['b'])
+      })
+
+      // There should only be one active subscription after changing the arg
+      checkNumSubscriptions('b', 1)
+
+      // 4) Re-render with the same arg.
+      // This causes the `subscriptionRemoved` check to be `false`.
+      // Correct behavior is this does _not_ clear the promise ref,
+      // so
+      await act(async () => {
+        rerender(['b'])
+      })
+
+      // There should only be one active subscription after changing the arg
+      checkNumSubscriptions('b', 1)
+
+      await act(async () => {
+        await delay(1)
+      })
+
+      unmount()
+
+      await act(async () => {
+        await delay(1)
+      })
+
+      // There should be no subscription entries left over after changing
+      // cache key args and swapping `skip` on and off
+      checkNumSubscriptions('b', 0)
+
+      const finalSubscriptions = getSubscriptions()
+
+      for (let cacheKeyEntry of Object.values(finalSubscriptions)) {
+        expect(Object.values(cacheKeyEntry!).length).toBe(0)
+      }
+    })
+
     describe('Hook middleware requirements', () => {
       let mock: jest.SpyInstance
 
@@ -2472,7 +2566,11 @@ describe('skip behaviour', () => {
     await act(async () => {
       rerender([1, { skip: true }])
     })
-    expect(result.current).toEqual(uninitialized)
+    expect(result.current).toEqual({
+      ...uninitialized,
+      currentData: undefined,
+      data: { name: 'Timmy' },
+    })
     await delay(1)
     expect(subscriptionCount('getUser(1)')).toBe(0)
   })
@@ -2489,6 +2587,7 @@ describe('skip behaviour', () => {
 
     expect(result.current).toEqual(uninitialized)
     await delay(1)
+
     expect(subscriptionCount('getUser(1)')).toBe(0)
     // also no subscription on `getUser(skipToken)` or similar:
     expect(storeRef.store.getState().api.subscriptions).toEqual({})
@@ -2504,10 +2603,51 @@ describe('skip behaviour', () => {
     await act(async () => {
       rerender([skipToken])
     })
-    expect(result.current).toEqual(uninitialized)
+    expect(result.current).toEqual({
+      ...uninitialized,
+      currentData: undefined,
+      data: { name: 'Timmy' },
+    })
     await delay(1)
     expect(subscriptionCount('getUser(1)')).toBe(0)
   })
+
+  test('skipping a previously fetched query retains the existing value as `data`, but clears `currentData`', async () => {
+    const { result, rerender } = renderHook(
+      ([arg, options]: Parameters<typeof api.endpoints.getUser.useQuery>) =>
+        api.endpoints.getUser.useQuery(arg, options),
+      {
+        wrapper: storeRef.wrapper,
+        initialProps: [1],
+      }
+    )
+
+    await act(async () => {
+      await delay(1)
+    })
+
+    // Normal fulfilled result, with both `data` and `currentData`
+    expect(result.current).toMatchObject({
+      status: QueryStatus.fulfilled,
+      isSuccess: true,
+      data: { name: 'Timmy' },
+      currentData: { name: 'Timmy' },
+    })
+
+    await act(async () => {
+      rerender([1, { skip: true }])
+      await delay(1)
+    })
+
+    // After skipping, the query is "uninitialized", but still retains the last fetched `data`
+    // even though it's skipped. `currentData` is undefined, since that matches the current arg.
+    expect(result.current).toMatchObject({
+      status: QueryStatus.uninitialized,
+      isSuccess: false,
+      data: { name: 'Timmy' },
+      currentData: undefined,
+    })
+  })
 })
 
 // type tests: