Discussion: Async cleanups of useEffect

[Andarist]

Hi 👋

I've been asking about this on Twitter but was told that the issues here might be better to discuss this stuff.

My general concern is that async cleanups might lead to weird race conditions. It may be unwarranted but the concept itself sounds quite alarming to me and I'd like to discuss this, if possible.

If you go with the async cleanups then there is no guarantee that a scheduled work (or just any listeners) would get cleaned up before you get rid of a component instance, so for example:

useEffect(() => {
  if (state !== 'foo') return
  const id = setTimeout(() => setShouldAnimate(true), 300)
  return () => clearTimeout(id)
}, [state])

This might not work as intended. There is an off-chance that the scheduled timeout will fire after the containing component unmounts but before the timer gets disposed.

Calling setState after unmounting was always a sign of broken assumptions in the code or some programming error and React has been warning about it. I was told though that this has been accounted for and the warning is being suppressed now - so it won't pop up for users if setState got called in that short timeframe. So at least that's OK.

I'm worried though that a disposed component can still cause an unwanted side-effect in a parent. One can imagine some scenarios where that would matter.

1. orchestrating animation - an unmounted component tells the parent to trigger some sort of animation. The reason why the animation should happen is owned by a child, but it's also based on an additional timer because the reason might become invalid if the user performs some invalidating action quickly enough. It's not obvious here that useLayoutEffect should be used here to achieve instant cleanup.
2. similar case: orchestrating some in-product tour, triggering tooltips, arrows, whatever in the parent. It becomes even less apparent that this should be useLayoutEffect-based to achieve instant clean up as this is not related to layout, even remotely. This is business logic.

I hope my concerns are not warranted and you could clear up them for me, but right now I'm worried a lot that this is such a small difference for most of the people and that's it's hard to spot in the code that this might become a source of many very subtle bugs.

cc @gaearon @bvaughn

[ghost]

Is Async cleanups new feature？ As far as I know, It will be cleaned up before commit update. You should ensure that a clearTimeout callback is returned in useEffect.

[Andarist]

@By-Meteora this is something that got implemented in the upcoming React 17 as an optimization. As you might see in my example I do return () => clearTimeout(id) from the example effect, the problem is that React 17 is introducing a change to the moment in which this cleanup function gets called.

[gaearon]

It's not obvious here that useLayoutEffect should be used here to achieve instant cleanup.

Why is it not? It does seem like the use case for it.

[gaearon]

that this should be useLayoutEffect-based to achieve instant clean up as this is not related to layout, even remotely

I concede that its naming is overly narrow (to discourage people from using it in cases except where not using it would cause visual artifacts). But conceptually it's just a "synchronous useEffect". If synchronous behavior is what you want (because otherwise the user would notice it), then useLayoutEffect is the tool for the job.

[gaearon]

Overall child unmounting causing something to happen in a parent is a somewhat confusing flow. I can totally understand how one might end up with this, but I'd still encourage to avoid it where you can. A child doesn't unmount for no reason — something is causing it to unmount by setting state. That thing should also have called whatever you're calling from the effect. Then you wouldn't need multiple render passes, and both updates could be batched.

[Andarist]

Why is it not? It does seem like the use case for it.

1. Its name - given your comments I believe that changing this to useSynchronousEffect would make more sense
2. Docs don't really mention any other use case for it than "layout"-related stuff
3. There was no difference between cleanup timing for useEffect and useLayoutEffect before, so this makes it difficult to change the mental model around this right now (not impossible, just harder)
4. during SSR it's OK to call useEffect but it's not OK to call useLayoutEffect as React warns about it. This constraint doesn't make sense for me given the new model, because we might need to "deopt" to useLayoutEffect for business logic purposes
5. it's weird for me because there might be times that I don't care about the synchronous setup (so I'd like to useEffect )but I definitely don't want to risk timing bugs regarding async cleanup so I might be "forced" to deopt to the useLayoutEffect.

If synchronous behavior is what you want (because otherwise the user would notice it), then useLayoutEffect is the tool for the job.

Right, I agree with that - "problem" is that previously the use cases for this were limited to visual effects when component mounts, and right now you extend this to potentially non-visual stuff that might affect the logic of the application.

Overall child unmounting causing something to happen in a parent is a somewhat confusing flow. I can totally understand how one might end up with this, but I'd still encourage to avoid it where you can.

I also agree that this is an anti-pattern that should mostly be avoided, but the described issue is not about "child unmounting causing something to happen" but rather "logic bound to the child's lifetime is causing something to happen incorrectly". The logic is on written to change anything when unmounting, it strictly wants to change something when the component is alive - there is only a disconnect between a moment component is disposed by React and the DOM (from what I understand) and a moment in which the component itself is being notified about it and it has a chance to execute code between those 2 points in time

[gaearon]

I struggle to follow what "business logic" has to do with it. The purpose of useLayoutEffect is to do things that must be done before paint. This hasn't changed.

Right, I agree with that - "problem" is that previously the use cases for this were limited to visual effects when component mounts, and right now you extend this to potentially non-visual stuff that might affect the logic of the application.

I don't know where the "non-visual stuff" comes from here. We were just talking about animation, which is definitely visual. By definition, if it's not visual, then it's okay to run with a delay. If you have a different use case, let's discuss this use case directly.

[Andarist]

The purpose of useLayoutEffect is to do things that must be done before paint. This hasn't changed.

Right, I totally agree with this - but it's only really apparent when it comes to mounting because then without this you would end up with visual glitches. So this is a workaround for those.

I believe though that the situation I'm describing is different - it's not really about visual glitches, but rather about some logic being executed incorrectly due to lack of synchronous cancelation. It could be anything - sending an event to WS, displaying a notification, playing sound, whatever.

I don't know where the "non-visual stuff" comes from here. We were just talking about animation, which is definitely visual. By definition, if it's not visual, then it's okay to run with a delay. If you have a different use case, let's discuss this use case directly.

I have also mentioned the use case of a in-product tour. I think I might have come with a better one now though: playing sounds.

We could have a throttling effect that is supposed to play sounds based on some condition, we don't want to spam the user with sound notifications as this is annoying so we introduce a throttling mechanism to the mix. The component responsible for playing those sounds gets unmounted, other screen gets committed to the browser and at this moment a timeout from the throttle gets executed and the sound is played. The user gets confused because there is no visual element (anymore) associated with that sound notification.

[gaearon]

It could be anything - sending an event to WS, displaying a notification, playing sound, whatever.

I do think we need to be specific here. I don't see how either of these examples would be incorrect if performed a few milliseconds after something has been hidden.

The component responsible for playing those sounds gets unmounted, other screen gets committed to the browser and at this moment a timeout from the throttle gets executed and the sound is played. The user gets confused because there is no visual element (anymore) associated with that sound notification.

The delay we're talking about is at most one frame. I don't quite see how it would be distinguishable to the user if it's not directly visual. In fact playing a sound via browser API is likely to be slightly delayed and isn't precise either.

[incepter]

Hi Dan,

What if in the meantime an action that pushes a notification to a global state was done ? The notification will be displayed to the user despite that the component is unmounted (which generally indicates that the user moved on and no longer interested of what the component may produce (aborted)).

[gaearon]

Consider a situation where with React 16, an action fires and you display the notification just before the unmounting — let's say, 10 milliseconds before that. Then 10 ms later component unmounts. What do you do with the notification? Do you delete it from the screen after it has already been shown? This seems like it would cause a flash that is not particularly helpful to the user. I'd argue that most apps would keep the notification even if it's no longer relevant.

We've just established that even without asynchronous behavior, we may already have situation where the user sees a notification that's no longer relevant (because the action fired just before the unmount). By this argument, since the user can't perceive the difference between "10 ms before" and "10 ms after", it's also fine to show it if the action fires a millisecond after unmount. The difference is simply not perceivable.

I would agree to your argument if we were talking about behavior spanning a noticeable interval (e.g. a second) but we're talking about a single frame.

[incepter]

even without asynchronous behavior, we may already have situation where the user sees a notification that's no longer relevant.

I totally agree with you on this.

By pushing a notification I was referring to a somehow orchestrated notification system that will display for several seconds a snackbar (with interactive user actions) or whatever on the screen.

What bothers me about this, is that single frame is a place where side effects can be triggered. And a side effect, can do any sort of things (sockets, navigation, global mutations...). Which means for me that this is a place for bugs that are the hardest to find/debug.

Also, Let's say I have a unit test which is frame-exact in which I unmount a component and I expect a function not to be called. If I understand well the react 17 behavior, some of my tests may fail depending on the machine I am running my tests on. which would involve rewriting some tests and components while upgrading.

On the other side, If something resolves/changes in a single frame while unmounting and before the cleanup, it should be treated like a success operation.

[gaearon]

By pushing a notification I was referring to a somehow orchestrated notification system that will display for several seconds a snackbar (with interactive user actions) or whatever on the screen.

Right, in which case it doesn't strictly matter if it gets cancelled at the last moment or not. I think my argument above shows that, but let me know if I missed something.

What bothers me about this, is that single frame is a place where side effects can be triggered. And a side effect, can do any sort of things (sockets, navigation, global mutations...). Which means for me that this is a place for bugs that are the hardest to find/debug.

This is still too vague to respond to directly. Like I noted above, we need to discuss a specific case.

Also, Let's say I have a unit test which is frame-exact in which I unmount a component and I expect a function not to be called. If I understand well the react 17 behavior, some of my tests may fail depending on the machine I am running my tests on.

I don't see how that would be the case. The behavior is deterministic — the effects are flushed after the component is unmounted. To wait for both to happen in tests, you should use act(), just like a warning tells if you (if you forget to do it).

Again, this fully mirrors the existing behavior of useEffect for mounting and updates.

[davidje13]

I have a similar concern about this with maybe a more concrete example;

const MyComponent = ({ destination }) => {
  const handleMessage = useCallback((e) => {
    fetch(destination); // or some other state changing action, such as mutating a reducer in a context
  }, [destination]);

  useEffect(() => {
    window.addEventListener('message', handleMessage);
    return () => window.removeEventListener('message', handleMessage);
  }, [handleMessage]);

  return (<div>Listening for messages</div>);
}

1. is the teardown still guaranteed to be called before the next setup? (e.g. if destination changes, is there any chance that both the old and new listener could be bound at the same time?)
2. if I have a parent component like below, could 2 listeners be active at once during the layout transition?

const MyParentComponent = () => {
  const narrowView = useIsWindowNarrow();

  if (narrowView) {
    return (<section><MyComponent /></section>);
  } else {
    return (<section><div><MyComponent /></div></section>); // (i.e. something which prevents reusing the existing MyComponent)
  }
}

---

Should effects which use addEventListener be using useLayoutEffect? If so that should definitely be documented! or will this still be safe?

[GabbeV]

I encountered a component like the following in our codebase and tried to understand if it's still correct after the changes to effect cleanup in React 17. It's supposed to act as any other controlled input component but fetch the city from our server instead of the user having to type it.

function ZipCity({ zipCode, city, onChange }) {
  useEffect(() => {
    let isCanceled = false
    getCityForZipCode(zipCode).then(
      city => {
        if (!isCanceled) onChange(city)
      },
      error => {
        if (!isCanceled) onChange("Unknown city")
      }
    )
    return () => { isCanceled = true }
  }, [onChange])

  return <input readOnly value={city} />
}

The code doesn't do anything layout related and it looks like it handles cancellation correctly but what happens if getCityForZipCode resolve just as the component get unmounted?

Here is a a more contrived example that attempts to force it to resolve at a bad time. In React 16 clicking the Hide button always unmounts the component but in React 17 it sometimes immediately gets remounted. https://codesandbox.io/s/react-17-useeffect-race-condition-kn8hd?file=/src/App.js

This is a race condition that can occur any time a callback prop or context callback is called asynchronously in useEffect and that callback props sets state in an ancestor component. This doesn't seem like something that should be that uncommon.

I realized that this race condition isn't limited to React 17 as something similar could happen during a component update instead of a component unmount in earlier versions.

Being a race condition it will most likely be very hard to debug and seem like a foot gun in React. It might only occur once every thousand executions.

The only proper solution i can see would be to always perform cleanup synchronously and instead recommend against expensive cleanup functions. Until then there should at least be a more detailed section in the docs on when you need to use useLayoutEffect instead of useEffect even if the code has nothing to do with layout.