Table dynamic collections, useAsyncList, and useMemo/useCallback/etc can easily interact to create memory leaks

[tyleroooo]

Provide a general summary of the issue here

Say we extend the basic Table + useAsyncList examples to force a reload when some prop changes and create a useCallback function which is updating relatively frequently. It might look like this:

function AsyncSortTable({ importantArg }: { importantArg: number }) {
  const getKey = React.useCallback((k: StarWarsChar) => k.name, [importantArg]);

  let list = useAsyncList<StarWarsChar>({
    async load({ signal }) {
      let res = await fetch("https://swapi.py4e.com/api/people/?search", {
        signal,
      });
      let json = await res.json();
      return {
        items: json.results,
      };
    },
    async sort({ items, sortDescriptor }) {
      return {
        items: // do some sorting here
    },
  });

  React.useEffect(() => list.reload(), [importantArg]);

  return (
    <Table
      sortDescriptor={list.sortDescriptor}
      onSortChange={list.sort}
    >
      <TableHeader>
        {...some columns go here}
      </TableHeader>
      <TableBody items={list.items}>
        {(item) => (
          <Row key={getKey(item)}>
            {(columnKey) => <Cell>{item[columnKey]}</Cell>}
          </Row>
        )}
      </TableBody>
    </Table>
  );
}

This actually guarantees (from what I can tell) that every instance of the json.results (or items) array will never be garbage collected so long as the table is being rendered. This is a large memory leak if you are rendering a lot of data in your table or refreshing often.

This happens because the table's inner Collection maintains a WeakMap cache from each item to its node which contains its props (in this case, the TableBody's props) which includes its children (in this case a function that takes the item and returns the row). This function has in its context/scope a reference to the getKey function which is "old" in that it hasn't updated since the list got new data (since it only updates when importantArg changes and the latest list.items change was triggered by its inner reducer) and so it has within its scope a reference to an old list and its associated items. Since we have a reference to those items we guarantee they cannot be garbage collected from the WeakMap cache, which might contain references to previous lists, continuing the chain.

This is difficult to follow, so here's what that chain looks like in a chrome memory allocation timeline taken from within the CodeSandbox:

🤔 Expected Behavior?

Old versions of the list should get garbage collected, ideally.

😯 Current Behavior

They don't get garbage collected.

💁 Possible Solution

Remove use of useCallback fixes this, you can also move data loading elsewhere and not use useAsyncList.

🔦 Context

No response

🖥️ Steps to Reproduce

https://codesandbox.io/p/sandbox/late-platform-3s5vrn

Version

3.11.0, but also latest

What browsers are you seeing the problem on?

Firefox, Chrome

If other, please specify.

No response

What operating system are you using?

Mac

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[devongovett]

Could you move getKey outside the component entirely since it does not use any variables from that scope? Then you wouldn't need useCallback either.

Alternatively you could inline it into the function that renders the row.

Due to the caching, the rows would never re-render when getKey changed anyway. They only update when the row objects themselves change. So implementing it with useCallback like this wouldn't really work even if it did use variables from the surrounding scope.

[tyleroooo]

Yeah that's a valid fix and good things to keep in mind.

Another thing that would probably work is just using list.items.map to render the rows and columns rather than the dynamic syntax. We're not getting much use out of the caching since every new array contains all new items (by reference) even if they are the same data.