Improved "memory leak" warning

[Aprillion]

Dear React Maintainers,

My proposal to improve the "memory leak" warning has quite a long background, so let me first thank you for keeping React alive and well. You are all fabulous 🎉

TL;DR

A Promise is not cancellable, so there is 90% chance that no memory leaks will be fixed when a user applies a "solution" to this warning from the internet. This warning encourages a pit of failure (more complex code without removing actual memory leaks) and I argue the detection of memory leaks should be changed.

Table of contents: The good (Intended Solutions) - The bad - The uncanny - Proposal

The good

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 a useEffect cleanup function.

There were, obviously, good reasons to introduce this warning in the first place. React can't detect memory leaks directly, so this was the next best thing to detect forgotten imperatively attached DOM event handlers or uncancelled WebAPIs. The warning itself does not list any examples, so let me illustrate with my own bad example:

const [tick, setTick] = useState(0)
useEffect(() => {
  setInterval(() => setTick(t => t+1), 1000)
}, [])

And the intention of the warning is to gently guide the developer to figure out, on their own, what cleaning up means.

Intended Solutions

Let me list a few ideas how I think the memory leaks should be solved.

For the setInterval (or setTimeout):

const [tick, setTick] = useState(0);
useEffect(() => {
  const interval = setInterval(() => setTick(t => t + 1), 1000)
  return () => clearInterval(interval)
}, []);

Aborting a fetch:

const [data, setData] = useState(null);
useEffect(() => {
    const {signal, abort} = new AbortController()
    fetch('...', {signal}).then(r => r.json()).then(setData)
    return abort
}, []);

Replacing closure references to the dispatch function inside .then(...) when dealing with an uncancellable Promise:

const [data, setData] = useStateIfMounted(null)
useEffect(() => {
  myGenericPromise.then(setData)
}, [])

...

function useStateIfMounted(initialValue) {
  const [state, setState] = useState(initialValue)
  const setterRef = useRef(setState)
  const setterIfMounted = useCallback((...args) => setterRef.current(...args), [])
  useEffect(() => {
    return () => {
      setterRef.current = () => undefined
    }
  }, [])

  return [state, setterIfMounted]
}

Note the last one is basically recreating the no-op done internally by React (if my understanding is in the right ballpark).

The bad

Now, the same buggy code causes a completely different warning in tests (at least in most cases I checked, not sure if always):

Warning: An update to MyComponent inside a test was not wrapped in act(...).

Let's ignore that for a second and try to analyze the solutions that developers can find for the "memory leak" problem on the internet.

1.) isMounted variants, like in most of the top answers on StackOverflow or various learning materials such as Kent's useSafeDispatch

=>❌ These solutions only address the warning, not solving any memory leaks (presumably, they focus on making the warning go away if it's a false positive). The code is more complex without any benefit. An example of shutting up the warning without solving the leak:

const [tick, setTick] = useState(0)
const isMounted = useRef(true)
useEffect(() => {
  setInterval(() => isMounted && setTick(t => t+1), 1000) // ❌ please don't run infinite intervals at home
  return () => isMounted.current = false
}, [])

2.) fake cancellation like at the end of isMounted is an Antipattern or the cancel method from the react-async library

=>❌ These look like valid solutions on a first look, but they actually leave the closure references untouched, so not solving any memory leaks either. JS Promise is simply not cancellable (yet?), and no amount of sophistication will allow garbage collection of closures of the function used for the .then(...) callback while the Promise is still pending - it is not possible to modify the original Promise, only to create a new Promise. The code is much more complex without any benefit. (see The uncanny section for details)

3.) real cancellation like https://stackoverflow.com/a/54964237/1176601

=>✔ Aborting a fetch request and other operations that enable garbage collection.
=>❌ But it's very hard to figure it out from the available advice - "memory leaks" is not the best documented topic for JavaScript...

The uncanny

My statement about fake cancellations (2. above) could be controversial, so let me elaborate:

As far as I can tell, the makeCancellable utility from the end of https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html as well as all proposed solutions in #5465 , reactjs/react.dev#1082 and #15006 suffer from the same memory leaks as a naive solution would:

// naive
const [value, setValue] = useState()
useEffect(() => {
  myPromise.then(setValue)
}, [])

This naive solution can trigger the warning (as a race condition during unmount), while the complicated solutions won't. Both leak the exact same amount of memory references - functions that we know will never be used but the JS engine cannot garbage collect - because a closure reference to them still exists.

Let me illustrate on a modified example that can be executed in the console => the cancelablePromise will be rejected after 2 seconds, so the reference from then to the function that was a value of the setValue variable will exist for exactly as long as in the naive solution:

const cancelablePromise = makeCancelable(
  new Promise(r => setTimeout(r, 2000))
);
const setValue = () => undefined

cancelablePromise
  .promise
  .then(setValue)
  .catch((reason) => console.log('isCanceled', reason.isCanceled));

cancelablePromise.cancel(); // Cancel the promise

function makeCancelable(promise) {
  let hasCanceled_ = false;

  const wrappedPromise = new Promise((resolve, reject) => {
    promise.then(
      val => hasCanceled_ ? reject({isCanceled: true}) : resolve(val),
      error => hasCanceled_ ? reject({isCanceled: true}) : reject(error)
    );
  });

  return {
    promise: wrappedPromise,
    cancel() {
      hasCanceled_ = true;
    },
  };
};

As for the cancel method in https://github.com/async-library/react-async/blob/129385c7477c9c6b5ad9c4ea96220779478a1ff6/packages/react-async/src/useAsync.tsx#L230-L231, that is even more complicated, but my intuition says that a reference to dispatch will continue to exist after cancel() as long as the promiseFn is pending - because the cancel function does not clear lastPromise.current...

Proposal

Option 1: small tweak

How about flipping this condition if (didWarnStateUpdateForUnmountedComponent.has(componentName)) return; the other way round? If some code tries to update the state of an unmounted component just once, ignore it.

Sweep the little insignificant memory leak under the carpet (remember, I argue that it is more likely that an attempt to fix the warning will NOT fix any memory leaks, so remaining quiet is not such a bad option and the memory will be freed at the same time with or without this warning - but the user code will be much simpler if they don't attempt to fix this warning).

On the other hand, if the state update happens repeatedly after the unmounting (e.g. setInterval or observable stream), that is a very clear signal that the memory leak is more serious => the warning is much more useful in this situation. It might require some re-wording + a link to examples how to fix it correctly.

However, I am not sure about the related not wrapped in act(...) warning in tests. Changes to one warning might require synchronization of both warnings for consistency...

Option 2: detect the memory leaks in Dev Tools

In an ideal world, it should be possible to trigger browser garbage collection from inside React Dev Tools code in all supported browsers. That might not be the case in the real world, but please stay with me for 1 more minute: both supported browsers are open source and they both expose the tools to trigger garbage collection in GUI => some API must exist and it should be possible to expose it in a browser extension.

Now, if React Profiler could trigger GC reliably after pressing the stop profiling button, then we can use an array of WeakRefs to find all references to the dispatchers (or other objects) of unmounted components that leaked in some closure somewhere. Pseudocode:

const zombies = []

function unmountComponent(node) {
  dispatchZombies.push(new WeakRef(node))
}

function stopProfiling() {
  gc()
  warningAboutZombies.nodes = zombies.map((ref => ref.deref()).filter(Boolean)
}

Option 3: React.getZombies()

Expose some method to get a list of "undead zombie references" in tests, that would be available when running in Node with the --expose-gc flag...

[Aprillion]

The warning was removed in #22114 => closing this issue.