facebook / facebook/relay

`FragmentResource` leaks `temporaryRetain()` calls

Open
#4,384 0 comments 3 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
19k
Forks
1.9k
PR merge metrics
No merged PRs in 30d

Description

# Summary

`FragmentResource`'s usages of `temporaryRetain()` are not paired with either `permanentRetain()` or `dispose()` management, causing the referenced data to cause "transitive" memory leaks with evictable data staying in the store. These leaks are currently not permanent because there is a 5-minute timeout that eventually forces release as the last fallback.

## Deep Dive

Much of this is probably known to maintainers, but documenting it to make sure I didn't overlook something.

### SuspenseResource

`SuspenseResource` has a concept of [`temporaryRetain()`](https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/SuspenseResource.js#L54) to prevent garbage collection during a suspended render. It returns a disposable for imperative release but also includes a [5-minute](https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/SuspenseResource.js#L18) fallback of last resort for automatic release in the event of aborted renders, etc. There is also a [`permanentRetain()`](https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/SuspenseResource.js#L97) function for transitioning from out of this temporary hold into a retain that is coupled to the normal React component lifecycle. Put together there are 3 intended ways out of a temporary retain:

1. Transition into a permanent retain on the normal path
2. Manually dispose in the event of a canceled request
3. Time-based auto-dispose in the event of an aborted render that orphans the request

### QueryResource

`temporaryRetain()` is [used in `QueryResource`](https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/QueryResource.js#L291) where it is paired both with a [transition to `permanentRetain()`](https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/QueryResource.js#L326) during normal execution and an [imperative `dispose()`](https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/QueryResource.js#L277) in the event of cancelation. This logic works without issue.

### FragmentResource

`temporaryRetain()` is also [used in `FragmentResource`](https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/FragmentResource.js#L174-L180). The returned disposable is not captured for cancelation, and there are no corresponding calls to `permanentRetain()` to bind the retention to the component lifecycle. This results in the data related to the fragment staying in cache until the 5-minute auto-release timeout executes.

## Discovery

This is a rather complex edge-case that was found while troubleshooting a custom pagination implementation which we discovered was doing some unsafe things that this bug was somewhat covering for. After an initial page-level query, additional pages were being fetched via `useRefetchableFragment()` and each new page of nodes was filtered and then added to a separate in-memory data structure. For simplicity, this could look like nodes 1-5 being provided by the page-level query, and then 6-10 and 11-15 provided in subsequent refetches, and all of these nodes were then also referenced by a `nodeList` array. Crucially these nodes contained fragment references themselves. This `nodeList` array was then rendered in a virtualized list, meaning that not all components/references were mounted despite still being relevant to the page.

Issues arose when a user would scroll around the list and load at least two pages via refetch. The page/list seems to work initially, rendering the subset of items relevant to the current viewport/window and removing items outside of the virtualization window. But after 5 minutes, scrolling to and thus attempting to render an item from a not-latest refetch hit an undefined object as the data has been evicted by GC. Continuing from the above simplification, items 1-5 are still retained via the page-level query, allowing those items to mount and unmount appropriately as they come into view. Similarly, items 11-15 are retained via the latest refetch query, also allowing proper virtualization. But items 6-10 have their related query released as soon as the second refetch is made, meaning that they continue to exist via their "temporary" retentions until their 5-minute auto-release.

As noted above, this implementation was unsafe and we are fixing it through a proper conversion into Relay patterns. I'm reporting this solely because it causes transitive memory bloat and if other users do wind up experiencing this bug (probably via production crash reports) it's pretty hard to track down.

## Expected Behavior

Fragment resources should properly transition out of temporary retention status.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.