Skip to content

fix: fixed dedupe response cloning #73274

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

Merged
merged 1 commit into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions packages/next/src/server/lib/clone-response.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* Clones a response by teeing the body so we can return two independent
* ReadableStreams from it. This avoids the bug in the undici library around
* response cloning.
*
* After cloning, the original response's body will be consumed and closed.
*
* @see https://github.com/vercel/next.js/pull/73274
*
* @param original - The original response to clone.
* @returns A tuple containing two independent clones of the original response.
*/
export function cloneResponse(original: Response): [Response, Response] {
// If the response has no body, then we can just return the original response
// twice because it's immutable.
if (!original.body) {
return [original, original]
}

const [body1, body2] = original.body.tee()

const cloned1 = new Response(body1, {
status: original.status,
statusText: original.statusText,
headers: original.headers,
})

Object.defineProperty(cloned1, 'url', {
value: original.url,
})

const cloned2 = new Response(body2, {
status: original.status,
statusText: original.statusText,
headers: original.headers,
})

Object.defineProperty(cloned2, 'url', {
value: original.url,
})

return [cloned1, cloned2]
}
70 changes: 44 additions & 26 deletions packages/next/src/server/lib/dedupe-fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
* Based on https://github.com/facebook/react/blob/d4e78c42a94be027b4dc7ed2659a5fddfbf9bd4e/packages/react/src/ReactFetch.js
*/
import * as React from 'react'
import { cloneResponse } from './clone-response'
import { InvariantError } from '../../shared/lib/invariant-error'

const simpleCacheKey = '["GET",[],null,"follow",null,null,null,null]' // generateCacheKey(new Request('https://blank'));

Expand All @@ -24,14 +26,22 @@ function generateCacheKey(request: Request): string {
])
}

type CacheEntry = [
key: string,
promise: Promise<Response>,
response: Response | null,
]

export function createDedupeFetch(originalFetch: typeof fetch) {
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- url is the cache key
const getCacheEntries = React.cache((url: string): Array<any> => [])
const getCacheEntries = React.cache(
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- url is the cache key
(url: string): CacheEntry[] => []
)

return function dedupeFetch(
resource: URL | RequestInfo,
options?: RequestInit
) {
): Promise<Response> {
if (options && options.signal) {
// If we're passed a signal, then we assume that
// someone else controls the lifetime of this object and opts out of
Expand Down Expand Up @@ -60,7 +70,6 @@ export function createDedupeFetch(originalFetch: typeof fetch) {
: resource
if (
(request.method !== 'GET' && request.method !== 'HEAD') ||
// $FlowFixMe[prop-missing]: keepalive is real
request.keepalive
) {
// We currently don't dedupe requests that might have side-effects. Those
Expand All @@ -74,29 +83,38 @@ export function createDedupeFetch(originalFetch: typeof fetch) {
}

const cacheEntries = getCacheEntries(url)
let match
if (cacheEntries.length === 0) {
// We pass the original arguments here in case normalizing the Request
// doesn't include all the options in this environment.
match = originalFetch(resource, options)
cacheEntries.push(cacheKey, match)
} else {
// We use an array as the inner data structure since it's lighter and
// we typically only expect to see one or two entries here.
for (let i = 0, l = cacheEntries.length; i < l; i += 2) {
const key = cacheEntries[i]
const value = cacheEntries[i + 1]
if (key === cacheKey) {
match = value
// I would've preferred a labelled break but lint says no.
return match.then((response: Response) => response.clone())
}
for (let i = 0, j = cacheEntries.length; i < j; i += 1) {
const [key, promise] = cacheEntries[i]
if (key === cacheKey) {
return promise.then(() => {
const response = cacheEntries[i][2]
if (!response) throw new InvariantError('No cached response')

// We're cloning the response using this utility because there exists
// a bug in the undici library around response cloning. See the
// following pull request for more details:
// https://github.com/vercel/next.js/pull/73274
const [cloned1, cloned2] = cloneResponse(response)
cacheEntries[i][2] = cloned2
return cloned1
})
}
match = originalFetch(resource, options)
cacheEntries.push(cacheKey, match)
Copy link
Contributor

Choose a reason for hiding this comment

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

So it turns out that var args push is slower than non-var-args push in some runtimes so pushing an array appears faster. But the fastest is to call push twice (or after this change, three times)

}
// We clone the response so that each time you call this you get a new read
// of the body so that it can be read multiple times.
return match.then((response) => response.clone())

// We pass the original arguments here in case normalizing the Request
// doesn't include all the options in this environment.
const promise = originalFetch(resource, options)
const entry: CacheEntry = [cacheKey, promise, null]
cacheEntries.push(entry)

return promise.then((response) => {
// We're cloning the response using this utility because there exists
// a bug in the undici library around response cloning. See the
// following pull request for more details:
// https://github.com/vercel/next.js/pull/73274
const [cloned1, cloned2] = cloneResponse(response)
entry[2] = cloned2
return cloned1
})
}
}
73 changes: 44 additions & 29 deletions packages/next/src/server/lib/patch-fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
type CachedFetchData,
} from '../response-cache'
import { waitAtLeastOneReactRenderTask } from '../../lib/scheduler'
import { cloneResponse } from './clone-response'

const isEdgeRuntime = process.env.NEXT_RUNTIME === 'edge'

Expand Down Expand Up @@ -676,20 +677,25 @@ export function createPatchedFetcher(
statusText: res.statusText,
})
} else {
// We're cloning the response using this utility because there
// exists a bug in the undici library around response cloning.
// See the following pull request for more details:
// https://github.com/vercel/next.js/pull/73274
const [cloned1, cloned2] = cloneResponse(res)

// We are dynamically rendering including dev mode. We want to return
// the response to the caller as soon as possible because it might stream
// over a very long time.
res
.clone()
cloned1
.arrayBuffer()
.then(async (arrayBuffer) => {
const bodyBuffer = Buffer.from(arrayBuffer)

const fetchedData = {
headers: Object.fromEntries(res.headers.entries()),
headers: Object.fromEntries(cloned1.headers.entries()),
body: bodyBuffer.toString('base64'),
status: res.status,
url: res.url,
status: cloned1.status,
url: cloned1.url,
}

requestStore?.serverComponentsHmrCache?.set(
Expand Down Expand Up @@ -720,7 +726,7 @@ export function createPatchedFetcher(
)
.finally(handleUnlock)

return res
return cloned2
}
}

Expand Down Expand Up @@ -788,14 +794,23 @@ export function createPatchedFetcher(
if (entry.isStale) {
workStore.pendingRevalidates ??= {}
if (!workStore.pendingRevalidates[cacheKey]) {
workStore.pendingRevalidates[cacheKey] = doOriginalFetch(
true
)
.catch(console.error)
const pendingRevalidate = doOriginalFetch(true)
.then(async (response) => ({
body: await response.arrayBuffer(),
headers: response.headers,
status: response.status,
statusText: response.statusText,
}))
.finally(() => {
workStore.pendingRevalidates ??= {}
delete workStore.pendingRevalidates[cacheKey || '']
})

// Attach the empty catch here so we don't get a "unhandled
// promise rejection" warning.
pendingRevalidate.catch(console.error)

workStore.pendingRevalidates[cacheKey] = pendingRevalidate
}
}

Expand Down Expand Up @@ -895,7 +910,7 @@ export function createPatchedFetcher(
if (cacheKey && isForegroundRevalidate) {
const pendingRevalidateKey = cacheKey
workStore.pendingRevalidates ??= {}
const pendingRevalidate =
let pendingRevalidate =
workStore.pendingRevalidates[pendingRevalidateKey]

if (pendingRevalidate) {
Expand All @@ -914,27 +929,27 @@ export function createPatchedFetcher(

// We used to just resolve the Response and clone it however for
// static generation with dynamicIO we need the response to be able to
// be resolved in a microtask and Response#clone() will never have a
// body that can resolve in a microtask in node (as observed through
// be resolved in a microtask and cloning the response will never have
// a body that can resolve in a microtask in node (as observed through
// experimentation) So instead we await the body and then when it is
// available we construct manually cloned Response objects with the
// body as an ArrayBuffer. This will be resolvable in a microtask
// making it compatible with dynamicIO.
const pendingResponse = doOriginalFetch(true, cacheReasonOverride)

const nextRevalidate = pendingResponse
.then(async (response) => {
// Clone the response here. It'll run first because we attached
// the resolve before we returned below. We have to clone it
// because the original response is going to be consumed by
// at a later point in time.
const clonedResponse = response.clone()

// We're cloning the response using this utility because there
// exists a bug in the undici library around response cloning.
// See the following pull request for more details:
// https://github.com/vercel/next.js/pull/73274
.then(cloneResponse)

pendingRevalidate = pendingResponse
.then(async (responses) => {
const response = responses[0]
return {
body: await clonedResponse.arrayBuffer(),
headers: clonedResponse.headers,
status: clonedResponse.status,
statusText: clonedResponse.statusText,
body: await response.arrayBuffer(),
headers: response.headers,
status: response.status,
statusText: response.statusText,
}
})
.finally(() => {
Expand All @@ -949,11 +964,11 @@ export function createPatchedFetcher(

// Attach the empty catch here so we don't get a "unhandled promise
// rejection" warning
nextRevalidate.catch(() => {})
pendingRevalidate.catch(() => {})

workStore.pendingRevalidates[pendingRevalidateKey] = nextRevalidate
workStore.pendingRevalidates[pendingRevalidateKey] = pendingRevalidate

return pendingResponse
return pendingResponse.then((responses) => responses[1])
} else {
return doOriginalFetch(false, cacheReasonOverride)
}
Expand Down
Loading