Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible race condition with useSelector #1508

Closed
markerikson opened this issue Jan 24, 2020 · 5 comments
Closed

Possible race condition with useSelector #1508

markerikson opened this issue Jan 24, 2020 · 5 comments

Comments

@markerikson
Copy link
Contributor

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

This was reported by @Jessidhia over in Reactiflux, so I'll paste her writeup:

Jessidhia : hmm, no, there is definitely some spooky race condition in react-redux
Jessidhia : likely in how it uses Refs
Jessidhia : this is the state of a useSelector that is returning null:

{
  name: 'Selector',
  subHooks: [    {
      name: 'ReduxContext',
    },
    {
      name: 'SelectorWithStoreAndSubscription',
      subHooks: [        { id: 0, isStateEditable: true, name: 'Reducer', value: 0 },
        {
          id: 1,
          isStateEditable: false,
          name: 'Memo',
          value: {
            ...Subscription
          }
        },
        { id: 2, name: 'Ref', value: undefined },
        {
          id: 3,
          name: 'Ref',
          value: root => cachedUserWorksAllByType(root, userId, WorkType.Illust)[index]
        },
        {
          id: 4,
          name: 'Ref',
          value: {
            illustId: '78095873',
            illustTitle: '?????',
            ...moreDataThatIsNotNull
          }
        },
        { id: 5, name: 'LayoutEffect' },
        { id: 6, name: 'LayoutEffect' }
      ]
    }
  ]
}

Note how the update counter (Reducer) is still at 0 and the value cached in the third Ref is most definitely not null
Jessidhia : it is possible for that selector (cached in the second Ref) to return null, but never after it has already returned an object. undefined -> (object) or undefined -> null -> (object) are the only possible transitions.
Jessidhia : I have no idea how to reduce this or make it reproducible, though
Jessidhia : it's very finicky
Jessidhia : some instances of the component (that do transition null -> object) get the counter incremented to 3, others do call the refEquality function (which I'm spying on) and the counter doesn't get incremented at all despite it returning false
Jessidhia : but since the Ref was updated, unless changes to props or a parent update triggers a rerender of the component, redux updates alone won't cause updates

Jessidhia: what do you think of https://gist.github.com/Jessidhia/afac05ee884c34d588547324ba0d49ec ? I believe this avoids the race condition I'm encountering, though I have no idea how to prove that.

What is the expected behavior?

Presumably that whatever race condition here doesn't happen.

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

I assume React-Redux v7.1.x.

Note that this overlaps with the refactoring we're doing in #1506 , and we should look at the gist to see what changes are there and if we can apply them. I'd also like to figure out if we can replicate the problem being seen, prior to applying the memory leak changes.

@timdorr
Copy link
Member

timdorr commented Jan 25, 2020

The ref that's in question here is latestSubscriptionCallbackError.

Yes, there is likely a problem with re-rendering a previously errored out useSelector call. But the intention of that code is more gracefully handle the stale props/zombie child problem. In that case, the stale component should not receive a second render; it's destined to be unmounted.

I'd like to see the use case where they're running into this. Is there a Codesandbox to demonstrate? It would seem the simpler fix is to just add latestSubscriptionCallbackError.current = undefined to not have it continually re-run the selector on each render after an error (assuming it's recoverable).

@markerikson
Copy link
Contributor Author

I know @Jessidhia works on a pretty complex app (and possibly uses CM/Suspense in it?), so not sure how reproducible it is.

@mlandalv
Copy link

mlandalv commented Mar 4, 2020

We're experiencing a similar problem. Not sure if it's the same as OP, but possibly.

Our app is also pretty complex with a mix of class and hook components. When we rewrote a class component using connect to a functional component with useSelector we got some issues with child class components calling dispatch in their componentDidUpdate. The store is updated correctly but useSelector in the parent component doesn't trigger a rerender so the updated value isn't reflected in the DOM.

I've created a simple repo with both a test page and a failing test: https://github.com/mlandalv/react-redux-useselector-prob

@dai-shi
Copy link
Contributor

dai-shi commented Mar 4, 2020

If it's about child components that dispatch in componentDidUpdate or useLayoutEffect, it's probably the same as #1532.
I'm not sure either if @Jessidhia 's case is the same or not.

@dai-shi
Copy link
Contributor

dai-shi commented Mar 17, 2020

I wonder if #1536 solves @Jessidhia case. Do you have any chance to try it out?

@timdorr timdorr closed this as completed Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants