Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Memory leak suspense safe useId #7757

Merged
merged 6 commits into from
Feb 14, 2025
Merged

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Feb 11, 2025

Closes #7738

This PR makes useId safe to use with Suspense. As stated in the issue, using useId inside a Suspense boundary in concurrent mode can result in a memory leak. This is because we write to a Map during render so that we can track values during the render phase.
Because we do it during render though, and mount/unmount are never called on the suspended components, we never have the opportunity to clean up.

React does eventually clean up Suspended components depending on waves hands criteria.

We can take advantage of that by adding an object we know will be stable for the entire lifetime of that component to a FinalizationRegistry. When that component is cleaned up and the gc runs, we will get a callback such that we can remove the key from the Map.

One thing to note in the docs for FinalizationRegistry, is there is a caveat, this function may never be called. However, I believe this applies to the life time of the entire application. I think it doesn't mean that the memory can be reclaimed without calling it. Since we don't care when the values are cleaned up, I think this is an ok caveat.

I've checked our other components. Nowhere else appears to have this behaviour. We do have a couple minor memory leaks with our Formatter caches, however, those should be negligible and we'd need to write our own GC to handle those. I've left them alone for now.

In the future, we should use WeakMap whenever possible for these situations.

For now, to avoid a breaking change, we have to use the FinalizationRegistry.

Tests are omitted because getting node to GC on demand is very fiddly. I've left a story in storybook under useId.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@snowystinger snowystinger changed the title Suspense safe useId fix: Memory leak suspense safe useId Feb 11, 2025
@rspbot
Copy link

rspbot commented Feb 11, 2025

@rspbot
Copy link

rspbot commented Feb 11, 2025

@liaoyinglong
Copy link

liaoyinglong commented Feb 11, 2025

Test Findings:
In my testing, memory still isn't being properly released. The cached useCallback appears to retain fiber references, preventing garbage collection and consequently keeping cleanupRef in memory indefinitely.

Working Solution:
Changing to this structure successfully triggers GC:

let idRefsMap: Map<string, { current: string | null }[]> = new Map();

cache useCallback

1.mp4

cache ref

2.mp4

@snowystinger
Copy link
Member Author

Thanks for double checking that, I'll update that soon.

@rspbot
Copy link

rspbot commented Feb 13, 2025

@rspbot
Copy link

rspbot commented Feb 13, 2025

@snowystinger
Copy link
Member Author

Compared to your videos, looks like this is behaving correctly now. Thanks again for checking me and tracking down the issue, apologies for not picking up on your comment earlier.

@rspbot
Copy link

rspbot commented Feb 13, 2025

// This allows us to clean up the idsUpdaterMap when the id is no longer used.
// Map is a strong reference, so unused ids wouldn't be cleaned up otherwise.
// This can happen in suspended components where mount/unmount is not called.
let registry = new FinalizationRegistry<string>((heldValue) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

@snowystinger snowystinger added this pull request to the merge queue Feb 14, 2025
Merged via the queue into main with commit 3fee0f4 Feb 14, 2025
30 checks passed
@snowystinger snowystinger deleted the demo-suspense-safe-useid-usage branch February 14, 2025 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useId memory leak in Suspense with concurrent mode
5 participants