React: setState in useEffect causing a "React state update on an unmounted component" warning

[KidkArolis]

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

What is the current behavior?
The warning is triggered after an intricate sequence of events. I stumbled upon it by accident, assumed it was an error in my code, but then step by step removed everything in my application until I was left with a fairly small bit of code that doesn't seem to be doing anything illegal from the API point of view, yet is triggering a warning.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem:

https://codesandbox.io/s/q87882qv64

The example is my real application code trimmed down as much as possible to demonstrate the warning. So some of the code might be a bit nonsensical/contrived, but that's because I removed lots of surrounding code leaving only the relevant bits for reproducing the issue.

In other words, there might weird looking uses of useEffect and weird sequencing of things, but that sort of falls out of how I've structured my routes, state, components in the full app, etc.

What is the expected behavior?

I would like to know if

a) is this a React bug that I stumbled upon that should be fixed?
b) is this something I'm doing "wrong" and how I could fix that in my application (i.e. is this a real memory leak being caused because of the way I structured the code)

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.8.4

[KidkArolis]

Just to add a bit about why I think this is a bug.

The recommendation from the warning is to cleanup any subscriptions inside useEffect in the effect destroy function. But in this codesandbox, you should see that useEffect destroy is only called after the setState is called. There is no way to clean that up afaik since we're not yet aware that the component is being unmounted.

[gaearon]

This code is definitely buggy btw. You’re passing [] as effect deps but close over both atom and selector. That’s not legit. I suggest you to adopt the exhaustive-deps rule and fix those bugs first.

Implementing subscriptions correctly is actually somewhat tricky. Check this for inspiration: https://gist.github.com/bvaughn/e25397f70e8c65b0ae0d7c90b731b189

Does that work better?

[KidkArolis]

I'll continue working on this codesandbox to try and minimise the code and isolate the issue better.

I am aware of some of the subscription pitfalls, this code is not what I use in production version. This is just an attempt at finding a minimal set of code that triggers this warning without using the React APIs "incorrectly" (except for the thing you pointed about about effect's deps, but I doubt that's the issue, I'll check).

It has something to do with the sync calls inside useEffect triggering calls to setStates causing components to unrender, etc. It's something along the lines of useEffect triggering a setState in parent component causing to unrender the child, but in the same tick setState getting called in the child component's useEffect's subscription, before the effect had the chance to cleanup. Not sure you're following, I can elaborate..

I guess I'm just inviting you to have a bit closer look to see that this is not bad logic in my code, but a potentially incorrect behaviour in React. I know there's 99% chance the issue is in my code, or something I'm doing that "you're not supposed to be doing", but I was hoping it's valuable to share this in case it is indeed an issue in React or a matter of missing documentation.

I know how to fix this in my application (e.g. defer my router listener callbacks), I ~know how to implement subscriptions correctly (thanks for the link, that's very useful), I'm not interested in how to fix my code, I'm just interested to help reveal a potential bug.. thus the contrived code.

[gaearon]

I doubt that's the issue, I'll check

I pointed this out because I’ve seen similar issues caused by a stale listener running after a fresh listener or something like it. So it’ll be way easier to debug after rules aren’t violated.

[KidkArolis]

Will fix that and reshare a codesandbox.

Just found some new insight (pointing to the fact that the issue _is in my code_). Will post back soon(ish).

[gaearon]

It's something along the lines of useEffect triggering a setState in parent component causing to unrender the child, but in the same tick setState getting called in the child component's useEffect's subscription, before the effect had the chance to cleanup. Not sure you're following, I can elaborate..

Sounds a bit like https://github.com/facebook/react/issues/14750#issuecomment-460409609.

[KidkArolis]

No, not sure it's my code.

Here's an updated, slightly smaller sandbox https://codesandbox.io/s/98lkm3731o (but with console.logs).

If this helps at all, what I can see happening is the following:

Notice how

[Onboarding] useEffect/subscription/1]
... lots of stuff happens synchronously in the middle, including running effects and destroying them
[Onboarding] useEffect/subscription/2]

The code that logs that is:

console.log(`[${name}] useEffect/subscription/1`);
setState(atom.get());
console.log(`[${name}] useEffect/subscription/2`);

Hm, not sure where I'm going with this..

[KidkArolis]

Btw, I've added all effect dependencies to the code now.

[KidkArolis]

The unexpected thing to me that these logs revealed is that my subscription callbacks are "getting incepted".. I get a subscription callback triggered while in the middle of sync handling of a subscription callback.

[KidkArolis]

Reduced the code further, removed the router bit and some effects:
https://codesandbox.io/s/zxp40j6ll4

[KidkArolis]

Oh now, it looks like an issue in my code though, since now the logs indicate subscription hasn't been cleaned up.. duh, this is so annoying.

[KidkArolis]

OK, I think your initial instict was right, Dan. It is a subscription problem.

I thought it's sufficient to clean subscriptions up by removing the subscription, but you _have_ to use the didUnsubscribe variable like Brian did, because in this case even if the subscriber is removed from the listener array, I'm already inside a sync subscription trigger loop which is going off the old reference of the listeners and it does call my subscriber even if it was technically removed.

I can fix this upstream in tiny-atom now.

Again, thanks for your time 🙏 Apologies for raising false alarms.

[KidkArolis]

Oh noes.. In my actual app, the fix is not sufficient, there might be something else going on. I'll have to continue digging a bit.

[KidkArolis]

For what it's worth, here's a further simplified sandbox with subscription fix mentioned above: https://codesandbox.io/s/421mlo34z7 but the React warning still being triggered. I'll continue investigating..

[KidkArolis]

The issue is roughly something like this, all in the same tick (I think?):

1. Child component's subscription calls setState
2. That for some reason (?) triggers effect in Parent
3. That causes an atom.set in Parent
4. It also causes another Child's subscription callback (inception, remember, we're still in the Child effect's subscription callback)
5. The parent now rerenders and Child is unmounted
6. That causes Child's useEffect cleanup
7. React warning is triggered
8. Only now the original subscription callback completed

You can see all that here:

I mean, I get this looks like some dodgy code on my part 😨 But it doesn't look "incorrect", or does it?

[KidkArolis]

I believe step 2 happens because React calls flushPassiveEffects() when Child calls setState which basically leads to this unexpected (to me) outcome.

[KidkArolis]

No, that's not right, I'll shut up now. Update: it is right, flushPassiveEffects is what triggers the "special sequence" in this case leading to the warning.

[gaearon]

Minor note — you might find it easier to debug if you use console.group / console.groupEnd like here: https://codesandbox.io/s/40l158pp29

[gaearon]

Ok so essentially you're saying setState causes previous effect to flush, by itself causing its destroy to set the done flag, but by that point it's already too late to check it (because we already are inside the setState) so we can't avoid the warning.

[gaearon]

Actually wait no. The previous effect can't set this effect's done flag. I'm still confused.

[gaearon]

We're inside a change handler set up by an effect. That change handler sets the state. This flushes the passive effects, leading to unmount of this component. And therefore continuing with the setState call is useless and shows the warning.

In the unmount case, I think this is something we should fix in React. If flushing passive effects led to unmounting of the component with the currently called state setter, it makes no sense to try to apply that update. It'll never happen anyway.

[KidkArolis]

Phew 😅 Yes, that makes sense. I understand why the warning is triggered now.

[gaearon]

https://github.com/facebook/react/pull/15067

[KidkArolis]

What are passive effects actually?
Also, why does a child’s setState trigger some parents useEffect without Parent being rerenderd? That effect has already been executed when parent rendered and was commited the first time, why is effect getting executed again without a new render?

[gaearon]

useEffect is what we call “passive effects”. It executes with a little delay to let the browser paint first.

Effect doesn’t execute before its render. (That would be impossible because effects are defined in render.) Instead, as I mentioned, effects execute with a delay. But if you try to render again before the previous effects had a chance to flush, we flush them before processing the update. This helps ensure that the behavior is deterministic.

So what you’re seeing is that a setState forces the previous render’s effects to flush. If you didn’t setState they would still flush, but a bit later (after paint).

[kaimiyang]

I have the same question when use axios get data, report:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.

[gaearon]

@kaimiyang Please file a new issue with a CodeSandbox. It's likely unrelated to this one.
Here's an Axios demo: https://codesandbox.io/s/k0lm13kwxo

[kaimiyang]

@gaearon Thank you, this is my fault. ( I unmounted the father component, and updated a children component, and then warning report). Now it's okey!

[gaearon]

This appears fixed by https://github.com/facebook/react/pull/15650. I can't repro it anymore with 0.0.0-db3ae32b8 (soon to be a release candidate for 16.9).