Fix useSelector race condition when dispatch happens in child's useLayoutEffect or componentDidUpdate

[laxels]

When an action is dispatched inside a child class component's useLayoutEffect or componentDidUpdate, checkForUpdates synchronously executes before the code block in useIsomorphicLayoutEffect:

latestSelector.current = selector
latestSelectedState.current = selectedState
latestSubscriptionCallbackError.current = undefined

This block sets latestSelectedState.current to the selectedState value assigned by the latest render, which may be stale in comparison to the selectedState value gotten in checkForUpdates. In this case, the stale value overwrites the new value. I've added a test case which reproduces the issue.

This PR fixes the issue by moving the ref-mutating lines out of useIsomorphicLayoutEffect, ensuring that they execute in useSelector's render step immediately. Executing during the render phase should be fine since all it's doing is updating refs to their most up-to-date value.

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 11db31a

https://deploy-preview-1532--react-redux-docs.netlify.com

[dai-shi]

Updating refs in the effect is intentional for concurrent mode.
Do you think this case can be solved by setting the initial ref values?

  const latestSubscriptionCallbackError = useRef()
  const latestSelector = useRef(selector)
  const latestSelectedState = useRef(null)
  if (latestSelectedState.current === null) {
    // lazy ref initialization
    latestSelectedState.current = selector(store.getState())
  }

(But, this doesn't work in the case that selectedState can be null. Should we use a special value?)

[laxels]

Updating refs in the effect is intentional for concurrent mode.
Do you think this case can be solved by setting the initial ref values?

  const latestSelector = useRef(selector)
  const latestSelectedState = useRef(selectedState)

Ah, is that so? I'm not yet aware of the interactions with concurrent mode. I'll try to provide a solution which keeps it in the useIsomorphicLayoutEffect then.

Unfortunately, I don't think setting initial ref values would do anything in this context. The problem is that this block, which is logically updating the selectedState cache to its newest value, is using a potentially stale variable. This can happen long after the initial render phase.

[dai-shi]

This can happen long after the initial render phase.

I see. I misunderstood the issue then. 😥

[markerikson]

Can you provide a sandbox that demonstrates the bug before the fix?

[markerikson]

Uh... looking at this, no, moving the assignments into the render logic is not a correct solution. Mutations while rendering are never allowed, especially in Concurrent Mode.

[markerikson]

I'm also not convinced that the test case is demonstrating an actual bug. If you could explain the potentially bad sequence further (with a more realistic use case if possible), I'd appreciate it.

[laxels]

Can you provide a sandbox that demonstrates the bug before the fix?

Yep, here you go:
https://codesandbox.io/s/react-redux-useselector-bug-igp9l

^In that sandbox, I'm simulating loading a user's configured views from a server. Each of those views, once loaded, needs to be initialized by the frontend. The ViewsInitializer component checks for uninitialized views in its componentDidUpdate and dispatches an action to initialize each one.

Its parent, ViewsInitializerParent, is what grabs the uninitialized views using useSelector. I use both a memoized and an un-memoized selector to show that the memoized selector is returning a stale value while the un-memoized one returns the properly updated value, reflecting the actual state object in Redux. This is because the un-memoized selector forces useSelector to always re-run the selector instead of referring to lastSelectedState.current.

If you uncomment the first return in ViewsInitializerParent, it will pass down the stale value to ViewsInitializer, causing it to re-initialize infinitely until the page crashes.

You may argue that this is a bad way to structure the components, which I completely agree with, but this is a real case example which shows that useSelector can get into a state in which it returns something completely different than what is actually in the Redux store.

Uh... looking at this, no, moving the assignments into the render logic is not a correct solution. Mutations while rendering are never allowed, especially in Concurrent Mode.

Sounds good to me. I'll submit a fix soon which keeps the mutations wrapped in the useIsomorphicLayoutEffect.

[timdorr]

That should go in a componentDidMount, not the update lifecycle. You're just creating an infinite loop, as you're not checking if the update is changing the uninitializedViews prop. That isn't a race, that's a loop.

There isn't a bug here, you're just using the lifecycle incorrectly.

[laxels]

That should go in a componentDidMount, not the update lifecycle. You're just creating an infinite loop, as you're not checking if the update is changing the uninitializedViews prop. That isn't a race, that's a loop.

There isn't a bug here, you're just using the lifecycle incorrectly.

As I mentioned in my previous comment, I am aware that the example app I threw in the sandbox is using the lifecycle incorrectly. The point was that it's a very easy step to get useSelector to return a stale value while there is obviously a newer value in the Redux store. The infinite loop can be removed and the bug is still there. The simplest example I could think of is in the test case I added.

If it is seen as intended behavior that useSelector may return incorrect data, well, welp.

P.S. For whoever may read this, the bug is also triggered when the child component is functional and dispatches in a useLayoutEffect. I believe this is because both a child's useLayoutEffect and componentDidUpdate both run before the parent's useLayoutEffect.

[dai-shi]

I believe this is because both a child's useLayoutEffect and componentDidUpdate both run before the parent's useLayoutEffect.

That's what I thought with the first look (without deep look).
So, do you think you can reproduce the issue only with function components?

[laxels]

I believe this is because both a child's useLayoutEffect and componentDidUpdate both run before the parent's useLayoutEffect.

That's what I thought with the first look (without deep look).
So, do you think you can reproduce the issue only with function components?

Yee, here's the exact same sandbox except with a function component:
https://codesandbox.io/s/react-redux-useselector-bug-igp9l

[dai-shi]

Thanks. That helps me understand, and it's something I originally thought. (Though, my oversight was the assumption that it only happens in the initial render.)

@timdorr I think this is a pitfall that can happen with useLayoutEffect (and cDM/cDU) in child components, if dispatching actions in useLayoutEffect is allowed.

@laxels You might want to change the title as this is not only about class components.

[laxels]

@laxels You might want to change the title as this is not only about class components.

Done. Thanks for the reminder!

[dai-shi]

👍 (BTW, I haven't yet come up with a solution.)

[dai-shi]

@laxels
Here's my solution. Would you try if it fixes your case?

diff --git a/src/hooks/useSelector.js b/src/hooks/useSelector.js
index d6e8592..10ff53e 100644
--- a/src/hooks/useSelector.js
+++ b/src/hooks/useSelector.js
@@ -22,15 +22,18 @@ function useSelectorWithStoreAndSubscription(
   const latestSubscriptionCallbackError = useRef()
   const latestSelector = useRef()
   const latestSelectedState = useRef()
+  const latestStoreState = useRef()
 
+  const storeState = store.getState()
   let selectedState
 
   try {
     if (
       selector !== latestSelector.current ||
+      storeState !== latestStoreState.current ||
       latestSubscriptionCallbackError.current
     ) {
-      selectedState = selector(store.getState())
+      selectedState = selector(storeState)
     } else {
       selectedState = latestSelectedState.current
     }
@@ -46,6 +49,7 @@ function useSelectorWithStoreAndSubscription(
     latestSelector.current = selector
     latestSelectedState.current = selectedState
     latestSubscriptionCallbackError.current = undefined
+    latestStoreState.current = storeState
   })
 
   useIsomorphicLayoutEffect(() => {

[dai-shi]

I filed a new PR #1536 as this PR is closed and hardly noticeable.

[laxels]

Ah, just saw your comments @dai-shi. I'll continue this conversation in your new PR.

[markerikson]

Finally live in https://github.com/reduxjs/react-redux/releases/tag/v7.2.1