Skip to content

Commit

Permalink
[backport] Fix/dedupe fetch clone (#73532)
Browse files Browse the repository at this point in the history
<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->

This is a backport of #73274 which
resolves a bug with response cloning.
  • Loading branch information
wyattjoh authored Dec 4, 2024
1 parent cbc62ad commit 530421d
Show file tree
Hide file tree
Showing 3 changed files with 222 additions and 17 deletions.
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]
}
125 changes: 125 additions & 0 deletions packages/next/src/server/lib/dedupe-fetch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
/**
* Based on https://github.com/facebook/react/blob/d4e78c42a94be027b4dc7ed2659a5fddfbf9bd4e/packages/react/src/ReactFetch.js
*/
import * as React from 'react'
import { cloneResponse } from './clone-response'

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

function generateCacheKey(request: Request): string {
// We pick the fields that goes into the key used to dedupe requests.
// We don't include the `cache` field, because we end up using whatever
// caching resulted from the first request.
// Notably we currently don't consider non-standard (or future) options.
// This might not be safe. TODO: warn for non-standard extensions differing.
// IF YOU CHANGE THIS UPDATE THE simpleCacheKey ABOVE.
return JSON.stringify([
request.method,
Array.from(request.headers.entries()),
request.mode,
request.redirect,
request.credentials,
request.referrer,
request.referrerPolicy,
request.integrity,
])
}

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

export function createDedupeFetch(originalFetch: typeof fetch) {
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
// caching. It's effectively the opt-out mechanism.
// Ideally we should be able to check this on the Request but
// it always gets initialized with its own signal so we don't
// know if it's supposed to override - unless we also override the
// Request constructor.
return originalFetch(resource, options)
}
// Normalize the Request
let url: string
let cacheKey: string
if (typeof resource === 'string' && !options) {
// Fast path.
cacheKey = simpleCacheKey
url = resource
} else {
// Normalize the request.
// if resource is not a string or a URL (its an instance of Request)
// then do not instantiate a new Request but instead
// reuse the request as to not disturb the body in the event it's a ReadableStream.
const request =
typeof resource === 'string' || resource instanceof URL
? new Request(resource, options)
: resource
if (
(request.method !== 'GET' && request.method !== 'HEAD') ||
request.keepalive
) {
// We currently don't dedupe requests that might have side-effects. Those
// have to be explicitly cached. We assume that the request doesn't have a
// body if it's GET or HEAD.
// keepalive gets treated the same as if you passed a custom cache signal.
return originalFetch(resource, options)
}
cacheKey = generateCacheKey(request)
url = request.url
}

const cacheEntries = getCacheEntries(url)
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 Error('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
})
}
}

// We pass the original arguments here in case normalizing the Request
// doesn't include all the options in this environment. We also pass a
// signal down to the original fetch as to bypass the underlying React fetch
// cache.
const controller = new AbortController()
const promise = originalFetch(resource, {
...options,
signal: controller.signal,
})
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
})
}
}
71 changes: 54 additions & 17 deletions packages/next/src/server/lib/patch-fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
import * as Log from '../../build/output/log'
import { trackDynamicFetch } from '../app-render/dynamic-rendering'
import type { FetchMetric } from '../base-http'
import { createDedupeFetch } from './dedupe-fetch'
import { cloneResponse } from './clone-response'

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

Expand Down Expand Up @@ -623,15 +625,26 @@ function createPatchedFetcher(
if (entry.isStale) {
staticGenerationStore.pendingRevalidates ??= {}
if (!staticGenerationStore.pendingRevalidates[cacheKey]) {
const pendingRevalidate = doOriginalFetch(true)
.then(async (response) => ({
body: await response.arrayBuffer(),
headers: response.headers,
status: response.status,
statusText: response.statusText,
}))
.finally(() => {
staticGenerationStore.pendingRevalidates ??= {}
delete staticGenerationStore.pendingRevalidates[
cacheKey || ''
]
})

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

staticGenerationStore.pendingRevalidates[cacheKey] =
doOriginalFetch(true)
.catch(console.error)
.finally(() => {
staticGenerationStore.pendingRevalidates ??= {}
delete staticGenerationStore.pendingRevalidates[
cacheKey || ''
]
})
pendingRevalidate
}
}
const resData = entry.value.data
Expand Down Expand Up @@ -730,16 +743,40 @@ function createPatchedFetcher(
// origin hit if it's a cache-able entry
if (cacheKey && isForegroundRevalidate) {
staticGenerationStore.pendingRevalidates ??= {}
const pendingRevalidate =
let pendingRevalidate =
staticGenerationStore.pendingRevalidates[cacheKey]

if (pendingRevalidate) {
const res: Response = await pendingRevalidate
return res.clone()
const revalidatedResult: {
body: ArrayBuffer
headers: Headers
status: number
statusText: string
} = await pendingRevalidate
return new Response(revalidatedResult.body, {
headers: revalidatedResult.headers,
status: revalidatedResult.status,
statusText: revalidatedResult.statusText,
})
}

const pendingResponse = doOriginalFetch(true, cacheReasonOverride)
const nextRevalidate = pendingResponse
.then((res) => res.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 response.arrayBuffer(),
headers: response.headers,
status: response.status,
statusText: response.statusText,
}
})
.finally(() => {
if (cacheKey) {
// If the pending revalidate is not present in the store, then
Expand All @@ -754,11 +791,11 @@ function createPatchedFetcher(

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

staticGenerationStore.pendingRevalidates[cacheKey] = nextRevalidate
staticGenerationStore.pendingRevalidates[cacheKey] = pendingRevalidate

return pendingResponse
return pendingResponse.then((responses) => responses[1])
} else {
return doOriginalFetch(false, cacheReasonOverride).finally(
handleUnlock
Expand All @@ -784,7 +821,7 @@ export function patchFetch(options: PatchableModule) {

// Grab the original fetch function. We'll attach this so we can use it in
// the patched fetch function.
const original = globalThis.fetch
const original = createDedupeFetch(globalThis.fetch)

// Set the global fetch to the patched fetch.
globalThis.fetch = createPatchedFetcher(original, options)
Expand Down

0 comments on commit 530421d

Please sign in to comment.