fix useSelector race condition with memoized selector when dispatching in child components useLayoutEffect as well as cDM/cDU

[dai-shi]

This is the continuation of #1532.
It can be related with #1508 too.

This only happened if a parent component uses a selector with useCallback, and a child component dispatch an action in useLayouteffect (or componentDidMount / componentDidUpdate).

You can try how it works or fails with the test case by changing one of conditions: Removing useCallback, or changing useLayoutEffect to useEffect, with reverting the useSelector patch.

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit ee80685

https://deploy-preview-1536--react-redux-docs.netlify.app

[dai-shi]

codesandbox reproduction by @laxels :
https://codesandbox.io/s/react-redux-useselector-bug-igp9l

[laxels]

So you're adding another condition for re-running the selector: if the store's state has been updated since the last time the cache refs were updated. I think this works to elegantly solve the problem.

A tiny side effect of this change is that the selector is now invoked an extra time, which I don't think should matter.

[laxels]

@dai-shi There is actually still a tiny edge case bug left here, though it's much harder to trigger:

Check out this test case:

        store = createStore(c => c + 1, -1)
        const selectorA = c => Math.min(c, 1)
        const selectorB = c => c

        const Comp = () => {
          const selectorRef = useRef(selectorA)
          useEffect(() => {
            selectorRef.current = selectorB
          }, [])

          const count = useSelector(selectorRef.current)
          renderedItems.push(count)
          return <Child parentCount={count} />
        }

        const Child = ({ parentCount }) => {
          useLayoutEffect(() => {
            if (parentCount === 1) {
              store.dispatch({ type: '' })
            }
          }, [parentCount])
          return <div>{parentCount}</div>
        }

        rtl.render(
          <ProviderMock store={store}>
            <Comp />
          </ProviderMock>
        )

        // The first render doesn't trigger dispatch
        expect(renderedItems).toEqual([0])

        // This dispatch triggers another dispatch in useLayoutEffect
        rtl.act(() => {
          store.dispatch({ type: '' })
        })

        expect(renderedItems).toEqual([0, 1, 2])

The only difference between this and your test case is that the selector switches from selectorA to selectorB after the first render. Put another way, selectorA is only ever passed into useSelector the first time Comp is rendered.

Since the dispatch in Child's useLayoutEffect actually fires before lastSelector is updated, it uses selectorA instead of selectorB to decide whether or not the new selected state is equal to the old one. Since selectorA caps at 1, it's equal to the old value and does not trigger a re-render even though the store state has incremented from 1 to 2.

It's caused by essentially the same timing issue. Another edge case which may also happen due to the same timing issue is that the selector errors in checkForUpdates, which causes the error to be recorded in latestSubscriptionCallbackError, only to be overwritten as undefined before the next render happens. The error is never surfaced as a result.

While I haven't figured out how to pass in a fresh latestSelector and latestSelectedState to checkForUpdates (this would fix the test case I posted), IMO the updates to the cache refs in the useIsomorphicLayoutEffect should not automatically assume that the values are guaranteed to be fresh. To build further on your solution, maybe we can do something like

useIsomorphicLayoutEffect(() => {
  if (store.getState() === storeState) {
    // update cache refs
  }
})

With this^, checkForUpdates would also need to ensure that the cache refs are all up-to-date to handle the case where updates are skipped in useIsomorphicLayoutEffect. I think the only thing we would need to add there is

latestStoreState.current = store.getState()

[dai-shi]

Nice catch!
Although I understand the edge case, we wouldn't need to take care of mutating selector outside render. We could simply claim it as a wrong usage.

It's like this, which can be seen as a wrong usage at a glance:

let selector = ...;
setTimeout(() => { selector= ...; }, 1000);
const Comp = {
  const selected = useSelector(selector);
  ...;
};

Another edge case which may also happen due to the same timing issue is that the selector errors in checkForUpdates

Can you elaborate this please? This sounds like a realistic case, but I'm not certain.

[laxels]

Although I understand the edge case, we wouldn't need to take care of mutating selector outside render. We could simply claim it as a wrong usage.

It also applies to in-render mutations, though. For example, the super common usage of useCallback:

const Comp = ({maxVal}) => {
  const selector = useCallback(c => Math.min(c, maxVal), [maxVal]);
  const selected = useSelector(selector);
  ...
};

^ The above will also miss required re-renders. The problem is that checkForUpdates is using a stale latestSelector value even though a render with a new selector has already happened.

Can you elaborate this please? This sounds like a realistic case, but I'm not certain.

Yet another contrived example, but it's easy to see how this one could happen randomly in the wild:

const Comp = () => {
  const selected = useSelector(() => {
    // Simulate an operation with a chance to error out
    if (Math.random() < 0.9) {
      return {};
    }
    throw new Error('Something went wrong');
  });
  ...
};

When the child dispatches in useLayoutEffect / cDU, checkForUpdates may encounter an error when running the selector and store it:

latestSubscriptionCallbackError.current = err

It then relies on the next render to throw the error for it. However, the useIsomorphicLayoutEffect code runs after checkForUpdates but before the next render, with the line

latestSubscriptionCallbackError.current = undefined

So the error is silently ignored.

[dai-shi]

const selector = useCallback(c => Math.min(c, maxVal), [maxVal]);
^ The above will also miss required re-renders.

OK, I get it. Thanks for your patience.
We could run checkForUpdates in useIsomorphicLayoutEffect to cover such edge cases.

As I understand, this only happens when child components trigger dispatch in useLayoutEffect instead of useEffect. Class components are troublesome in this case, because there's no useEffect equivalent.

It then relies on the next render to throw the error for it. However, the useIsomorphicLayoutEffect code runs after checkForUpdates but before the next render, with the line

Yeah, similar for this. We could keep the error in a scoped variable in render and check if it's changed in useIsomorphicLayoutEffect (and forceRender if changed.)

[dai-shi]

@laxels If you are interested, please check out #1509. The implementation doesn't use useLayoutEffect, so there's probably no race condition. I'm not pushing to merge it because we are waiting for useMutableSource.

[dai-shi]

@markerikson Can you take a look at this? I think the spec is also useful to evaluate useMutableSource based implementation.

[timdorr]

I think it looks good. I don't have any objections.

[daiky00]

@timdorr why haven't this been merge I need it :(

[timdorr]

I'd just like another approval here and then we can merge this in.

[markerikson]

Seems reasonable.

[timdorr]

Thanks!

[morteza-fsh]

@markerikson
This change is not taken place on npm, when can we expect a new release?

[timdorr]

I'm dealing with a number of TS updates to Redux for 5.0 and working on getting Redux DevTools published and synced up on npm. I haven't had time to work on getting a new React Redux release together, but do have it on my list.

[markerikson]

I'll try to look at putting out a new release this weekend.

[timdorr]

Go right ahead. Doesn't have to wait on me!

[louisscruz]

Just for another datapoint: we hit this problem in production today and confirmed that this PR fixed the problem.