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

Use a single global infinite promise #34

Merged
merged 2 commits into from
Feb 24, 2024

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Oct 17, 2023

It's not super clear why the code is written this way, but I assume it's due to not being familiar with the pattern. If you want to wait until the freeze prop changes, it's enough to throw a Promise that never resolves.

This fixes an issue that would occur on initial mount with createRoot and/or Fabric. The issue is that React throws away uncommitted trees if they suspend during mount, and this includes throwing away useRef. So stashing Promises in useRef is wrong. Having one at the top level is both safe and simpler.

@kmagiera
Copy link
Member

Thanks for sending the PR. I don't remember if we'd ever consider doing this in proposed way and as you suggested it is likely due to the fact we weren't familiar with this pattern.

As for the issue with initial mount that you mentioned, is it reported somewhere?

As for the actual change, one thing that I wonder is whether this could potentially result in memory leaks? My understanding is that what React does under the hood, is it calls .then on the promise that gets thrown. On the other hand, the promise object has to retain all the callbacks passed via .then. If there is just one global promise it'll keep all the callbacks forever therefore leaking memory. This problem may have limited impact depending on what objects these callbacks retain, but IMO it is possible that they may capture some React internal data structures (old fiber trees?) that may get very big in some apps.

@kmagiera
Copy link
Member

I wonder if we could just override then to be a noop and if that'd suffice

@gaearon
Copy link
Contributor Author

gaearon commented Oct 25, 2023

As for the issue with initial mount that you mentioned, is it reported somewhere?

No. I don't mean that it's an actual issue that shows up today. I meant that with a Concurrent Root (on the web, this corresponds to createRoot, on RN it should be enabled or at least opt-in with Fabric), useRef does not get preserved between suspended mount attempts. In general useRef or useState are never safe places to keep a Promise cache. Promise cache should always be outside the thing that may suspend.

As for the actual change, one thing that I wonder is whether this could potentially result in memory leaks? My understanding is that what React does under the hood, is it calls .then on the promise that gets thrown. On the other hand, the promise object has to retain all the callbacks passed via .then. If there is just one global promise it'll keep all the callbacks forever therefore leaking memory.

Hm, interesting! Next.js does use this pattern, but I'm not sure if this is a concern.

I wonder if we could just override then to be a noop and if that'd suffice

Yes, that should work fine. (On the React side, it's how a built-in version of this will be implemented.)

@gaearon
Copy link
Contributor Author

gaearon commented Oct 25, 2023

Pushed a fix.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Apologies for a long turnaround here. This looks good after the change. Will merge this in and will make a new release early next week. Thank you!

@kmagiera kmagiera merged commit 2e6ced2 into software-mansion:main Feb 24, 2024
1 check passed
@gaearon gaearon deleted the infinite-promise branch February 25, 2024 01:56
grahammendick added a commit to grahammendick/navigation that referenced this pull request Jun 1, 2024
See the matching PR on react-freeze software-mansion/react-freeze#34 - the ref could get thrown away so not safe, plus not needed. Removed the copyright because don't think it's needed - it's only 1 line of code that is pretty well known now (React supsending on thrown promise)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants