-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Prevent RenderPromises
memory leak by calling renderPromises.clear()
after getMarkupFromTree
finishes.
#7943
Conversation
Should fix #7942 and the tests I added in my previous commit.
RenderPromises
memory leak by calling renderPromises.clear()
after getMarkupFromTree
finishes.
}).finally(() => { | ||
renderPromises.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We sometimes get complaints about using Promise.prototype.finally
from folks who aren't using a polyfill in older browsers, but I think it's fine to use it here, because this is server-side rendering code, which typically runs in Node.js, which has supported Promise.prototype.finally
since v8.1.4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could always use try/finally and async/await too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great fix - thanks @benjamn!
This test is my best guess at a reproduction of issue #7942, given the heap snapshots provided by @AlexMost.
As with any
FinalizationRegistry
-driven test, the details are tricky, but you can see from the history of this PR that callingrenderPromises.clear()
makes the failing test pass consistently.