-
Notifications
You must be signed in to change notification settings - Fork 29.3k
fix: memory leak from cloneResponse #82678
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
Conversation
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Tests Passed |
Hey @snyamathi! Do you think you can address the failing tests in this PR? Feel free to ask any questions. |
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.
Does this code work in edge runtimes?
What about older Node.js versions?
@@ -1,3 +1,22 @@ | |||
// @ts-ignore https://nodejs.org/api/stream.html#streamreadableisdisturbedstream | |||
import { isDisturbed, isErrored } from 'node:stream' |
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.
If I read the docs correctly, these are statics on Readable
. Can't use node:
either since it needs to work in Edge runtimes.
import { isDisturbed, isErrored } from 'node:stream' | |
import { Readable } from 'stream' |
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.
Thank you, let me update this
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.
I'm still not able to get the isDisturbed
or isErrored
checks working type-wise. However, I'm also not certain they're needed at all.
The stream you are trying to cancel is not a ReadableStream, or it is locked.
https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/cancel#exceptions
Additionally, we're also catching the error, if one were to occur.
!isDisturbed(stream) && | ||
!isErrored(stream as any as NodeJS.ReadableStream) |
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.
!isDisturbed(stream) && | |
!isErrored(stream as any as NodeJS.ReadableStream) | |
!Readable.isDisturbed(stream) && | |
!Readable.isErrored(stream) |
I've added a check for I'm trying to get tests and to run locally - even the current |
repro info here: https://github.com/snyamathi/nextjs-mem-leak
Background
Undici added support for this in nodejs/undici#3199 and later for the body of cloned responses in nodejs/undici#3458 which first landed in Undici version 6.19.8 in August 2024. In September 2024, this issue #69635 was raised for the error,
TypeError: Response.clone: Body has already been consumed
In NextJS's dedupe-fetch the cloned response is returned to userland, while the original response is stored in a react cache
Undici Bug
I'm omitting some details in the code snippet below (see full changes at https://github.com/nodejs/undici/pull/3458/files), but the Undici change to
use FinalizationRegistry for cloned response body
seems to have mixed up the response pointer and stream bodies when registering with the finalization registry, resulting in the wrong stream being cancelled.The original response, (the
match
above) stored in the react cache is cloned, and then its stream is registered with the finalization registry when the cloned responsenewRequest
is reclaimed.I believe this is the true underlying cause of the errors:
Body has already been consumed
https://github.com/snyamathi/nextjs-mem-leak/blob/main/index.mjs has a simple reproduction of this issue which shows that when the clone is reclaimed, the original stream is cancelled, leading to issues
Memory Leak
This lead to #73274 which fixed the problem by adding a custom
cloneResponse
function.https://github.com/vercel/next.js/blob/7ef0a2b2767b4159f8db8e1884bac98370528a25/packages/next/src/server/lib/clone-response.ts
However, this in turn has lead to a memory leak because now there is no one to garbage collect the tee'd stream.
https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/tee
Undici was incorrectly cancelling the stream (leading to a bug), and Next is simply not cancelling the stream (leading to a memory leak). This can be observed with a Docker setup which shows a current version of NextJS, the last version prior to the custom
cloneResponse
function, as well as the effects of the proposed fix here.I have a reproduction available here https://github.com/snyamathi/nextjs-mem-leak/tree/main - you'll notice the memory for NextJS climb until it runs out of memory and crashes, whereas the version prior to this change (
15.0.4-canary
) is unaffected.Similarly, if we forego the dedupe cache by passing in a signal (
next.js/packages/next/src/server/lib/dedupe-fetch.ts
Line 45 in 102f233
The PR changes here are applied as a patch and also fix the memory leak :)
Each container outputs the request number and current memory usage which are then plotted in order to observe the memory leak due to the custom
cloneResponse
.Because the fix associates the correct response and stream, the previous regression does not happen again. We can confirm this by making requests to the page for each of the docker containers. The container using the version prior to the custom
cloneResponse
will error out, while the rest will notcc @wyattjoh @gnoff
Reproduction
https://github.com/snyamathi/nextjs-mem-leak/tree/main contains a number of reproductions
Body has already been consumed
errorcloneResponse
functionThe rest of the files are supporting infrastructure to start docker containers of various versions and various patches showing how versions prior to the
cloneResponse
do not have the same memory leak, and how the patch (PR'd here) fixes the issue without regressing on the Undici issue.