React 16.x causes exception with setState on unmounted component

[idibidiart]

React 16.x causes exception with setState on unmounted component

[idibidiart]

Wow. Then both theories could be true: rFA tasks queuing up and InteractionManager runs in a separate thread. This is nuts. Putting back line 21 and republishing. Thanks.

[idibidiart]

yeah np, but to clarify, interactionManager doesn't run in a separate thread but rather has a callback that can run after component is unmounted, thus scheduling an rAF that is missed by the cAF in componentWillUnmount. Async flow control is tricky.

[idibidiart]

something like this:

export default class Grid extends React.Component {
    constructor(props) {
        super(props)
        this.state = {...props.state, layout: {}}
        this.animFrame
        this.unmounting = false
    }

    componentWillUnmount = () => {
        this.unmounting = true
        cancelAnimationFrame(this.animFrame)
    }

    callback = (e) => {
        const layout = {
          screen: ScreenInfo(), 
          grid: e.nativeEvent.layout
        }
        this.setState((state) => {
            return this.unmounting ? null : {...state, layout}
        })
    }

    render() {
        return (
            <View
                style={[
                    {
                        flex: 1
                    },
                    this.props.style
                ]}
                
                onLayout={(e) => {
                    e.persist()
                    InteractionManager.runAfterInteractions(() => {
                        this.animFrame = requestAnimationFrame(() => {
                        this.callback(e)
                        })
                    })
                }}
            >
                {this.props.children({
                    state: this.state,
                    setState: (...args) => this.setState(...args),
                })}
            </View>)
    }
}

[idibidiart]

or try calling this.callback without the rFA deferment and see... no idea what React 16 is doing under the hood but worth a try

[peacechen]

React [Native] may have ignored this error or didn't detect it previously. I recall occasionally seeing yellow box warnings about calling setState on unmounted components. RN 0.54 / React 16 now throws exceptions on it.

setState can't be called at all after a component has unmounted. Checking the flag inside setState's callback is too late, as in your snippet. It needs to be checked before any calls to setState.

Created PR #25 to fix this.

[idibidiart]

See my comment on the PR

Race condition can only happen if cancelAnimationFrame is async which I would be appalled if it is (I hope not! Since it would really screw with the programmer to make cancelination of an async task async but can we find out for sure)

Returning null from setState fixes the situation where callback() is called before componentWillUnmount fires but the actual setting of state in the setState callback is async and so it can happen that component is u mounted before it runs which is what should prompt React to throw that error. Logically speaking, preventing the rerender is what React needs to prevent the error and in React 16 returning null from the callback to setState does exactly that.

I would not take the elimination of an error that happens sometimes but not all times as an indication that the fix works. It would be a mirage fix.

Logically the only way your fix could work is if cancelAnimationFrame is async.

Is cAF async? That would be a very bad programming model.

[peacechen]

Yes things are all moving to async in React 16.x. Lots of chatter about it in recent updates.

Perhaps the most exciting area we’re working on is async rendering—a strategy for cooperatively scheduling rendering work by periodically yielding execution to the browser.

https://reactjs.org/blog/2017/09/26/react-v16.0.html

Returning null in its callback doesn't stop setState from attempting to execute state updates. I tested your code and it continues to throw the exception.

[idibidiart]

See this for info on returning null from the callback passed to setState

https://x-team.com/blog/react-render-setstate/

React Async has nothing to do with making cAF async. It's has to do with pausing and resuming render aka Suspense

Not sure what to assume (if your fix works 100% of the time) unless cAF is async which would be a very bad flaw in the JS async programming model.

I'll do a console experiment later today....

Oh wait.... there is a possibility that interaction manager runs in its own thread in the Objective-C implementation. If that's the case, a race condition can happen.

I tweaked it so that we're protected against that (per your fix) as well as against the very real possibility that setState callback may run after component is unmounted, or so I hope! Check it out and please let me know if it does work 100% of the time for you

[peacechen]

Your fix works 👍 It's basically the same as my PR so that's expected.

Do you think it's necessary to also run the check inside the setState callback?

react-native-responsive-grid/src/components/Grid.js

Line 28 in 4385da7

return this.unmounting ? null : {...state, layout}

It doesn't throw an exception without that ternary. And since setState calls can happen quite a bit, reducing any little bit of overhead helps.

[idibidiart]

Well we need to understand the reason things work and don’t work and make educated decisions. Only reason the conditional early return works is if either cAF is async which would be bonkers from a programming model standpoint OR interactionManager runs in a separate thread. Else, we don’t have a logical explanation of why it works while cAF doesn’t.

The null return deals with another edge case:

setState may run in callback() before componetWillUnmount fires BUT it’s callback may run AFTER that in which case you will have another race condition.

[peacechen]

I see now that you're passing a function as setState's first argument. I was reading that as the second argument callback which fires after setState has already updated. You're right that returning null should prevent state updates. However React's error checking routine blindly detects if setState is called, whether or not it ends up mutating state.

Once rendering becomes async, cAF would probably need to as well. Even if cAF remains sync, there could be race conditions due to async rendering.

[idibidiart]

Returning null from the callback to setState only prevents rerender. Obviously, setState has already ran. React throws an error I assume if setState is called after unmount but there had to be a reason why they introduced the ability to cancel the pending state update (nullify the callback) and for now I’m happy to leave it there I’m case React is leaky in its abstraction in this case and relies on the programmer to do this low level task of nullifying the callback once the component has unmourned. I mean why else would they throw an error in the first place for setState after unmount? If they were handling it the user wouldn’t have to jump thru hoops but if we’re jumping thru hoops we might as well jump thru all hoops.

Btw, definition of race condition you’re using is different from the conventional definition which only applies in multi-threaded scenarios. There is no race condition per second in cooperative multitasking in a single thread but I we refer to that in terms of our perception of it, not an actual race condition. AFaIK event loop is made of ordered task queues each coming from a different part of the JS engine, e.g DOM, IndexedDB etc

[idibidiart]

“Per se” not “per second” lol

[idibidiart]

Also, I kindly disagree that cAF should be async when rendering is async. Rendering with rAF is async but cancelling a pending task should not be async or you lose your ability to derive a control flow abstraction.

[peacechen]

I don't fully understand all the async changes that are (will) happening under the hood. Exceptions such as the one for unmounted setState are in preparation of the upcoming React changes. Time slice and suspense are intriguing.
https://react-etc.net/entry/time-slice-and-suspense-features-coming-to-react-js-16-3-or-16-4

You're right that single threaded JS shouldn't cause a race in the traditional multi-threaded sense. In RN land, render gets offloaded to native code which is on a separate thread. Resulting events can fire truly asynchronously.

[idibidiart]

Btw, I have another theory that might be worth testing.

I just pushed a fix for the rAF queue that is created by having rAF calls in onLayout potentially accumulating in the task queue but only clearing one of those tasks on componentWillUnmount!

With this fix, I call cAF before calling rAF each time, so rAF tasks won't accumulate in the event loop.

To test the effect of it, uncomment line 21 in Grid.js "// if (this.unmounting) return" and see if you get the setState error ... if not then we can remove the early return in the callback() (but will keep the null return for the other edge case)

Yeah Suspense is intriguing if only because it's based on an interesting way for async control flow (throwing promises and awaiting on them in catch etc) but the intersection of single threaded concurrency (as in Suspense) and multi-threaded execution in RN is not a simple programming model.

[peacechen]

Nice sleuthing! Your latest fix does indeed work even with line 21 commented out. That does make sense now that you mention it about rAF tasks queueing up. Thanks for digging into this.

[idibidiart]

Great. Pushed fix.

[peacechen]

I just saw the setState exception again with line 21 removed. It wasn't happening yesterday but something changed with the timing today and is hitting it once more.

[peacechen]

Thanks for updating and sorry this has dragged on. Hopefully with these guards in place we won't have to look at this again.

[idibidiart]

Published re-fix with comments

export default class Grid extends React.Component {
    constructor(props) {
        super(props)
        this.state = {...props.state, layout: {}}
        this.animFrame
        this.unmounting = false
    }

    componentWillUnmount = () => {
        this.unmounting = true
        cancelAnimationFrame(this.animFrame)
    }

    callback = (e) => {

        // callback to runAfterInteractions is async
        // so onLayout might be triggered before component is unmounted
        // and it might schedule rAF after component is unmounted 
        // so cAF in componentWillUnmount would then miss that rFA
        if (this.unmounting) return

        const layout = {
          screen: ScreenInfo(), 
          grid: e.nativeEvent.layout
        }
        this.setState((state) => {
            return {...state, layout}
        })
    }

    render() {
        return (
            <View
                style={[
                    {
                        flex: 1
                    },
                    this.props.style
                ]}
                
                onLayout={(e) => {
                    e.persist()
                    InteractionManager.runAfterInteractions(() => {
                        // avoid queuing up rAF tasks
                        cancelAnimationFrame(this.animFrame)
                        this.animFrame = requestAnimationFrame(() => {
                            this.callback(e)
                        })
                    })
                }}
            >
                {this.props.children({
                    state: this.state,
                    setState: (...args) => this.setState(...args),
                })}
            </View>)
    }
}