From 94313366990b760e7401855d542c8dbc6b78635a Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Fri, 17 Jan 2025 12:28:16 +0100 Subject: [PATCH] fix fetch lock not being consistently released (#75028) --- packages/next/src/server/lib/patch-fetch.ts | 237 +++++++++--------- .../app-fetch-deduping-errors.test.ts | 13 + .../app/[id]/page.tsx | 45 ++++ .../app-fetch-deduping-errors/app/layout.tsx | 10 + 4 files changed, 190 insertions(+), 115 deletions(-) create mode 100644 test/e2e/app-dir/app-fetch-deduping-errors/app-fetch-deduping-errors.test.ts create mode 100644 test/e2e/app-dir/app-fetch-deduping-errors/app/[id]/page.tsx create mode 100644 test/e2e/app-dir/app-fetch-deduping-errors/app/layout.tsx diff --git a/packages/next/src/server/lib/patch-fetch.ts b/packages/next/src/server/lib/patch-fetch.ts index 60fca42908ba3..9b1853fedc7cd 100644 --- a/packages/next/src/server/lib/patch-fetch.ts +++ b/packages/next/src/server/lib/patch-fetch.ts @@ -610,132 +610,139 @@ export function createPatchedFetcher( next: { ...init?.next, fetchType: 'origin', fetchIdx }, } - return originFetch(input, clonedInit).then(async (res) => { - if (!isStale && fetchStart) { - trackFetchMetric(workStore, { - start: fetchStart, - url: fetchUrl, - cacheReason: cacheReasonOverride || cacheReason, - cacheStatus: - finalRevalidate === 0 || cacheReasonOverride - ? 'skip' - : 'miss', - cacheWarning, - status: res.status, - method: clonedInit.method || 'GET', - }) - } - if ( - res.status === 200 && - incrementalCache && - cacheKey && - (isCacheableRevalidate || requestStore?.serverComponentsHmrCache) - ) { - const normalizedRevalidate = - finalRevalidate >= INFINITE_CACHE - ? CACHE_ONE_YEAR - : finalRevalidate - const externalRevalidate = - finalRevalidate >= INFINITE_CACHE ? false : finalRevalidate - - if (workUnitStore && workUnitStore.type === 'prerender') { - // We are prerendering at build time or revalidate time with dynamicIO so we need to - // buffer the response so we can guarantee it can be read in a microtask - const bodyBuffer = await res.arrayBuffer() - - const fetchedData = { - headers: Object.fromEntries(res.headers.entries()), - body: Buffer.from(bodyBuffer).toString('base64'), + return originFetch(input, clonedInit) + .then(async (res) => { + if (!isStale && fetchStart) { + trackFetchMetric(workStore, { + start: fetchStart, + url: fetchUrl, + cacheReason: cacheReasonOverride || cacheReason, + cacheStatus: + finalRevalidate === 0 || cacheReasonOverride + ? 'skip' + : 'miss', + cacheWarning, status: res.status, - url: res.url, - } - - // We can skip checking the serverComponentsHmrCache because we aren't in - // dev mode. - - await incrementalCache.set( - cacheKey, - { - kind: CachedRouteKind.FETCH, - data: fetchedData, - revalidate: normalizedRevalidate, - }, - { - fetchCache: true, - revalidate: externalRevalidate, - fetchUrl, - fetchIdx, - tags, + method: clonedInit.method || 'GET', + }) + } + if ( + res.status === 200 && + incrementalCache && + cacheKey && + (isCacheableRevalidate || + requestStore?.serverComponentsHmrCache) + ) { + const normalizedRevalidate = + finalRevalidate >= INFINITE_CACHE + ? CACHE_ONE_YEAR + : finalRevalidate + const externalRevalidate = + finalRevalidate >= INFINITE_CACHE ? false : finalRevalidate + + if (workUnitStore && workUnitStore.type === 'prerender') { + // We are prerendering at build time or revalidate time with dynamicIO so we need to + // buffer the response so we can guarantee it can be read in a microtask + const bodyBuffer = await res.arrayBuffer() + + const fetchedData = { + headers: Object.fromEntries(res.headers.entries()), + body: Buffer.from(bodyBuffer).toString('base64'), + status: res.status, + url: res.url, } - ) - await handleUnlock() - // We we return a new Response to the caller. - return new Response(bodyBuffer, { - headers: res.headers, - status: res.status, - 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. - cloned1 - .arrayBuffer() - .then(async (arrayBuffer) => { - const bodyBuffer = Buffer.from(arrayBuffer) - - const fetchedData = { - headers: Object.fromEntries(cloned1.headers.entries()), - body: bodyBuffer.toString('base64'), - status: cloned1.status, - url: cloned1.url, + // We can skip checking the serverComponentsHmrCache because we aren't in + // dev mode. + + await incrementalCache.set( + cacheKey, + { + kind: CachedRouteKind.FETCH, + data: fetchedData, + revalidate: normalizedRevalidate, + }, + { + fetchCache: true, + revalidate: externalRevalidate, + fetchUrl, + fetchIdx, + tags, } + ) + await handleUnlock() - requestStore?.serverComponentsHmrCache?.set( - cacheKey, - fetchedData - ) - - if (isCacheableRevalidate) { - await incrementalCache.set( + // We return a new Response to the caller. + return new Response(bodyBuffer, { + headers: res.headers, + status: res.status, + 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. + cloned1 + .arrayBuffer() + .then(async (arrayBuffer) => { + const bodyBuffer = Buffer.from(arrayBuffer) + + const fetchedData = { + headers: Object.fromEntries(cloned1.headers.entries()), + body: bodyBuffer.toString('base64'), + status: cloned1.status, + url: cloned1.url, + } + + requestStore?.serverComponentsHmrCache?.set( cacheKey, - { - kind: CachedRouteKind.FETCH, - data: fetchedData, - revalidate: normalizedRevalidate, - }, - { - fetchCache: true, - revalidate: externalRevalidate, - fetchUrl, - fetchIdx, - tags, - } + fetchedData ) - } - }) - .catch((error) => - console.warn(`Failed to set fetch cache`, input, error) - ) - .finally(handleUnlock) - return cloned2 + if (isCacheableRevalidate) { + await incrementalCache.set( + cacheKey, + { + kind: CachedRouteKind.FETCH, + data: fetchedData, + revalidate: normalizedRevalidate, + }, + { + fetchCache: true, + revalidate: externalRevalidate, + fetchUrl, + fetchIdx, + tags, + } + ) + } + }) + .catch((error) => + console.warn(`Failed to set fetch cache`, input, error) + ) + .finally(handleUnlock) + + return cloned2 + } } - } - // we had response that we determined shouldn't be cached so we return it - // and don't cache it. This also needs to unlock the cache lock we acquired. - await handleUnlock() + // we had response that we determined shouldn't be cached so we return it + // and don't cache it. This also needs to unlock the cache lock we acquired. + await handleUnlock() - return res - }) + return res + }) + .catch((error) => { + handleUnlock() + throw error + }) } let cacheReasonOverride diff --git a/test/e2e/app-dir/app-fetch-deduping-errors/app-fetch-deduping-errors.test.ts b/test/e2e/app-dir/app-fetch-deduping-errors/app-fetch-deduping-errors.test.ts new file mode 100644 index 0000000000000..c4333ef2167d8 --- /dev/null +++ b/test/e2e/app-dir/app-fetch-deduping-errors/app-fetch-deduping-errors.test.ts @@ -0,0 +1,13 @@ +import { nextTestSetup } from 'e2e-utils' + +describe('app-fetch-errors', () => { + const { next } = nextTestSetup({ + files: __dirname, + }) + + it('should still successfully render when a fetch request that acquires a cache lock errors', async () => { + const browser = await next.browser('/1') + + expect(await browser.elementByCss('body').text()).toBe('Hello World 1') + }) +}) diff --git a/test/e2e/app-dir/app-fetch-deduping-errors/app/[id]/page.tsx b/test/e2e/app-dir/app-fetch-deduping-errors/app/[id]/page.tsx new file mode 100644 index 0000000000000..d46c6fd7dc8fd --- /dev/null +++ b/test/e2e/app-dir/app-fetch-deduping-errors/app/[id]/page.tsx @@ -0,0 +1,45 @@ +'use server' + +import { Metadata } from 'next' + +export async function generateMetadata({ + params, +}: { + params: Promise<{ id: string }> +}): Promise { + const { id } = await params + + try { + // this fetch request will error + await fetch('http://localhost:8000', { + cache: 'force-cache', + next: { tags: ['id'] }, + }) + } catch (err) { + console.log(err) + } + + return { + title: id, + } +} + +export default async function Error({ + params, +}: { + params: Promise<{ id: string }> +}) { + const { id } = await params + + try { + // this fetch request will error + await fetch('http://localhost:8000', { + cache: 'force-cache', + next: { tags: ['id'] }, + }) + } catch (err) { + console.log(err) + } + + return
Hello World {id}
+} diff --git a/test/e2e/app-dir/app-fetch-deduping-errors/app/layout.tsx b/test/e2e/app-dir/app-fetch-deduping-errors/app/layout.tsx new file mode 100644 index 0000000000000..69e348c39ef16 --- /dev/null +++ b/test/e2e/app-dir/app-fetch-deduping-errors/app/layout.tsx @@ -0,0 +1,10 @@ +export default function Layout({ children }) { + return ( + + + my static site + + {children} + + ) +}