Fix potential subscription leakage in SSR environments #5111
+123
−73
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
isRunningvalue inanySubscriptionsRemainingwas alwaysfalse, because it tried to access therunningQueriesmap too early and there was no per-store lookup table of promises yetFixes #5110
Background
In #5064 , I rewrote RTKQ's internals to improve perf. As part of that, I hoisted the
InternalMiddlewareStateobject out ofbuildMiddlewareand intocore/module.ts(part ofcreateApi), so that both the middleware and the thunks could access the same subscription data directly.Redux middleware are factory functions for closures. When
createStoreis called, it executes the outermost middleware function, which creates a closure scoped to just that store instance. When theInternalMiddlewareStateobject was being created there, this was fine - every store would get its ownInternalMiddlewareState.When I moved that instantiation up to
createApi, though, now that value is outside the scope of the middleware, and thus outside the lifetime of a given store. If multiple stores use the same RTKQ middleware definition, they each create their own unique middleware instance, but those middleware instances are all sharing the sameInternalMiddlewareStatereference.To be clear, this doesn't appear to have leaked any actual cache data - just that there were subscriptions of some kind, with RTKQ's internal cache entry IDs.
This PR resolves that by using the same technique we did for thunk promises - a
WeakMap<Dispatch, Thing>that ensures we cache the value on a per-store basis. In fact, since therunningQueries/Mutationsmaps were already on theInternalMiddlewareState, we're really just moving theDispatchkeying up one level, which simplifies a few other checks.I found I'd caused another issue in #5064 in the process. I had tried to save
const runningQueries = internalState.runningQueries.get(dispatch)at the top ofcacheCollection.ts, and then use it inanySubscriptionsRemaining. When I made my fixes for this issue, a single test inbuildHooks.test.tsxfailed, indicating some subscriptions weren't getting cleaned up. After a lot of investigation, I finally realized that the problem was theisRunningcheck itself. I added logging to the pre-fix logic and found thatrunningQueriesitself was alwaysundefined... because we only inserted the per-store lookup table whenever we had active running promises. Since we were trying to read that at the start of the middleware, there was no lookup table for this store yet. My fix accidentally uncovered that broken logic.It turns out that with the various other changes we've had, the
isRunningcheck inanySubscriptionsRemainingisn't even needed! I removed it and all the tests passed, with both the pre- and post-fix subscription handling. I actually don't have a full explanation for that right now, but also it's late and my mind's kinda fried :)