From 755c9e445bf88f05c8893579ce838161ff13be6c Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Mon, 10 Jun 2024 11:34:36 -0700 Subject: [PATCH] Add timeout/retry handling for fetch cache (#66652) As discussed this adds handling to timeout at a max of 500ms for fetch cache request and retries a max of 3 times due to network instability. This also adds cache service tests and fixes a case we've been trying to track down where we were seeing `undefined` cache URL values which made debugging fetches tricky. --- packages/next/src/server/base-server.ts | 1 - .../lib/incremental-cache/fetch-cache.ts | 72 ++++-- .../web/spec-extension/unstable-cache.ts | 10 +- test/ppr-tests-manifest.json | 11 + .../fetch-cache/app/api/revalidate/route.ts | 9 + .../app-dir/fetch-cache/app/layout.tsx | 8 + .../app-dir/fetch-cache/app/page.tsx | 32 +++ .../app-dir/fetch-cache/fetch-cache.test.ts | 240 ++++++++++++++++++ test/turbopack-build-tests-manifest.json | 11 + 9 files changed, 371 insertions(+), 23 deletions(-) create mode 100644 test/production/app-dir/fetch-cache/app/api/revalidate/route.ts create mode 100644 test/production/app-dir/fetch-cache/app/layout.tsx create mode 100644 test/production/app-dir/fetch-cache/app/page.tsx create mode 100644 test/production/app-dir/fetch-cache/fetch-cache.test.ts diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 96e25f3ff8ebd..714b27837f5d3 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -2792,7 +2792,6 @@ export default abstract class Server< kind: 'PAGE', html: RenderResult.fromStatic(''), pageData: {}, - postponed: undefined, headers: undefined, status: undefined, }, diff --git a/packages/next/src/server/lib/incremental-cache/fetch-cache.ts b/packages/next/src/server/lib/incremental-cache/fetch-cache.ts index ee0431a4f218d..909a3ad955a1a 100644 --- a/packages/next/src/server/lib/incremental-cache/fetch-cache.ts +++ b/packages/next/src/server/lib/incremental-cache/fetch-cache.ts @@ -24,10 +24,40 @@ const CACHE_REVALIDATE_HEADER = 'x-vercel-revalidate' as const const CACHE_FETCH_URL_HEADER = 'x-vercel-cache-item-name' as const const CACHE_CONTROL_VALUE_HEADER = 'x-vercel-cache-control' as const +const DEBUG = Boolean(process.env.NEXT_PRIVATE_DEBUG_CACHE) + +async function fetchRetryWithTimeout( + url: Parameters[0], + init: Parameters[1], + retryIndex = 0 +): Promise { + const controller = new AbortController() + const timeout = setTimeout(() => { + controller.abort() + }, 500) + + return fetch(url, { + ...(init || {}), + signal: controller.signal, + }) + .catch((err) => { + if (retryIndex === 3) { + throw err + } else { + if (DEBUG) { + console.log(`Fetch failed for ${url} retry ${retryIndex}`) + } + return fetchRetryWithTimeout(url, init, retryIndex + 1) + } + }) + .finally(() => { + clearTimeout(timeout) + }) +} + export default class FetchCache implements CacheHandler { private headers: Record private cacheEndpoint?: string - private debug: boolean private hasMatchingTags(arr1: string[], arr2: string[]) { if (arr1.length !== arr2.length) return false @@ -53,7 +83,6 @@ export default class FetchCache implements CacheHandler { } constructor(ctx: CacheHandlerContext) { - this.debug = !!process.env.NEXT_PRIVATE_DEBUG_CACHE this.headers = {} this.headers['Content-Type'] = 'application/json' @@ -79,17 +108,18 @@ export default class FetchCache implements CacheHandler { } if (scHost) { - this.cacheEndpoint = `https://${scHost}${scBasePath || ''}` - if (this.debug) { + const scProto = process.env.SUSPENSE_CACHE_PROTO || 'https' + this.cacheEndpoint = `${scProto}://${scHost}${scBasePath || ''}` + if (DEBUG) { console.log('using cache endpoint', this.cacheEndpoint) } - } else if (this.debug) { + } else if (DEBUG) { console.log('no cache endpoint available') } if (ctx.maxMemoryCacheSize) { if (!memoryCache) { - if (this.debug) { + if (DEBUG) { console.log('using memory store for fetch cache') } @@ -118,7 +148,7 @@ export default class FetchCache implements CacheHandler { }) } } else { - if (this.debug) { + if (DEBUG) { console.log('not using memory store for fetch cache') } } @@ -133,21 +163,21 @@ export default class FetchCache implements CacheHandler { ) { let [tags] = args tags = typeof tags === 'string' ? [tags] : tags - if (this.debug) { + if (DEBUG) { console.log('revalidateTag', tags) } if (!tags.length) return if (Date.now() < rateLimitedUntil) { - if (this.debug) { + if (DEBUG) { console.log('rate limited ', rateLimitedUntil) } return } try { - const res = await fetch( + const res = await fetchRetryWithTimeout( `${this.cacheEndpoint}/v1/suspense-cache/revalidate?tags=${tags .map((tag) => encodeURIComponent(tag)) .join(',')}`, @@ -181,7 +211,7 @@ export default class FetchCache implements CacheHandler { } if (Date.now() < rateLimitedUntil) { - if (this.debug) { + if (DEBUG) { console.log('rate limited') } return null @@ -227,7 +257,7 @@ export default class FetchCache implements CacheHandler { } if (res.status === 404) { - if (this.debug) { + if (DEBUG) { console.log( `no fetch cache entry for ${key}, duration: ${ Date.now() - start @@ -245,7 +275,7 @@ export default class FetchCache implements CacheHandler { const cached: IncrementalCacheValue = await res.json() if (!cached || cached.kind !== 'FETCH') { - this.debug && console.log({ cached }) + DEBUG && console.log({ cached }) throw new Error('invalid cache value') } @@ -272,7 +302,7 @@ export default class FetchCache implements CacheHandler { : Date.now() - parseInt(age || '0', 10) * 1000, } - if (this.debug) { + if (DEBUG) { console.log( `got fetch cache entry for ${key}, duration: ${ Date.now() - start @@ -289,7 +319,7 @@ export default class FetchCache implements CacheHandler { } } catch (err) { // unable to get data from fetch-cache - if (this.debug) { + if (DEBUG) { console.error(`Failed to get from fetch-cache`, err) } } @@ -314,7 +344,7 @@ export default class FetchCache implements CacheHandler { JSON.stringify((newValue as Record)[field]) ) ) { - if (this.debug) { + if (DEBUG) { console.log(`skipping cache set for ${key} as not modified`) } return @@ -324,7 +354,7 @@ export default class FetchCache implements CacheHandler { if (!fetchCache) return if (Date.now() < rateLimitedUntil) { - if (this.debug) { + if (DEBUG) { console.log('rate limited') } return @@ -356,7 +386,7 @@ export default class FetchCache implements CacheHandler { tags: undefined, }) - if (this.debug) { + if (DEBUG) { console.log('set cache', key) } const fetchParams: NextFetchCacheParams = { @@ -385,11 +415,11 @@ export default class FetchCache implements CacheHandler { } if (!res.ok) { - this.debug && console.log(await res.text()) + DEBUG && console.log(await res.text()) throw new Error(`invalid response ${res.status}`) } - if (this.debug) { + if (DEBUG) { console.log( `successfully set to fetch-cache for ${key}, duration: ${ Date.now() - start @@ -398,7 +428,7 @@ export default class FetchCache implements CacheHandler { } } catch (err) { // unable to set to fetch-cache - if (this.debug) { + if (DEBUG) { console.error(`Failed to update fetch cache`, err) } } diff --git a/packages/next/src/server/web/spec-extension/unstable-cache.ts b/packages/next/src/server/web/spec-extension/unstable-cache.ts index 56e0801492f18..e132279e0a94e 100644 --- a/packages/next/src/server/web/spec-extension/unstable-cache.ts +++ b/packages/next/src/server/web/spec-extension/unstable-cache.ts @@ -113,7 +113,7 @@ export function unstable_cache( return a.localeCompare(b) }) const sortedSearch = sortedSearchKeys - .map((key) => searchParams.get(key)) + .map((key) => `${key}=${searchParams.get(key)}`) .join('&') // Construct the complete cache key for this function invocation @@ -180,6 +180,7 @@ export function unstable_cache( tags, softTags: implicitTags, fetchIdx, + fetchUrl, }) if (cacheEntry && cacheEntry.value) { @@ -276,10 +277,17 @@ export function unstable_cache( if (!incrementalCache.isOnDemandRevalidate) { // We aren't doing an on demand revalidation so we check use the cache if valid + // @TODO check on this API. addImplicitTags mutates the store and returns the implicit tags. The naming + // of this function is potentially a little confusing + const implicitTags = store && addImplicitTags(store) + const cacheEntry = await incrementalCache.get(cacheKey, { kindHint: 'fetch', revalidate: options.revalidate, tags, + fetchIdx, + fetchUrl, + softTags: implicitTags, }) if (cacheEntry && cacheEntry.value) { diff --git a/test/ppr-tests-manifest.json b/test/ppr-tests-manifest.json index d3f170e5f5ea3..19db3c4177518 100644 --- a/test/ppr-tests-manifest.json +++ b/test/ppr-tests-manifest.json @@ -1,6 +1,17 @@ { "version": 2, "suites": { + "test/production/app-dir/fetch-cache/fetch-cache.test.ts": { + "passed": [], + "failed": [ + "fetch-cache should have correct fetchUrl field for fetches and unstable_cache", + "fetch-cache should retry 3 times when revalidate times out", + "fetch-cache should not retry for failed fetch-cache GET" + ], + "pending": [], + "flakey": [], + "runtimeError": false + }, "test/e2e/app-dir/app-static/app-static.test.ts": { "failed": [ "app-dir static/dynamic handling usePathname should have values from canonical url on rewrite", diff --git a/test/production/app-dir/fetch-cache/app/api/revalidate/route.ts b/test/production/app-dir/fetch-cache/app/api/revalidate/route.ts new file mode 100644 index 0000000000000..3f8985e20754e --- /dev/null +++ b/test/production/app-dir/fetch-cache/app/api/revalidate/route.ts @@ -0,0 +1,9 @@ +import { revalidateTag } from 'next/cache' +import { NextRequest, NextResponse } from 'next/server' + +export const dynamic = 'force-dynamic' + +export function GET(req: NextRequest) { + revalidateTag('thankyounext') + return NextResponse.json({ done: true }) +} diff --git a/test/production/app-dir/fetch-cache/app/layout.tsx b/test/production/app-dir/fetch-cache/app/layout.tsx new file mode 100644 index 0000000000000..888614deda3ba --- /dev/null +++ b/test/production/app-dir/fetch-cache/app/layout.tsx @@ -0,0 +1,8 @@ +import { ReactNode } from 'react' +export default function Root({ children }: { children: ReactNode }) { + return ( + + {children} + + ) +} diff --git a/test/production/app-dir/fetch-cache/app/page.tsx b/test/production/app-dir/fetch-cache/app/page.tsx new file mode 100644 index 0000000000000..40db1a33422ff --- /dev/null +++ b/test/production/app-dir/fetch-cache/app/page.tsx @@ -0,0 +1,32 @@ +import { unstable_cache } from 'next/cache' + +export const dynamic = 'force-dynamic' +export const fetchCache = 'default-cache' + +const getCachedRandom = unstable_cache( + async () => { + return Math.random() + }, + [], + { + revalidate: 3, + tags: ['thankyounext'], + } +) + +export default async function Page() { + const data = await fetch( + 'https://next-data-api-endpoint.vercel.app/api/random?a=b', + { next: { tags: ['thankyounext'], revalidate: 3 } } + ).then((res) => res.text()) + + const cachedRandom = getCachedRandom() + + return ( + <> +

hello world

+

{data}

+

{cachedRandom}

+ + ) +} diff --git a/test/production/app-dir/fetch-cache/fetch-cache.test.ts b/test/production/app-dir/fetch-cache/fetch-cache.test.ts new file mode 100644 index 0000000000000..972e8ef280076 --- /dev/null +++ b/test/production/app-dir/fetch-cache/fetch-cache.test.ts @@ -0,0 +1,240 @@ +import glob from 'glob' +import http from 'http' +import fs from 'fs-extra' +import { join } from 'path' +import { FileRef, NextInstance, createNext } from 'e2e-utils' +import { + retry, + killApp, + findPort, + fetchViaHTTP, + initNextServerScript, +} from 'next-test-utils' + +describe('fetch-cache', () => { + let next: NextInstance + let appPort: any + let cliOuptut = '' + let nextInstance: any + let fetchGetReqIndex = 0 + let revalidateReqIndex = 0 + let fetchGetShouldError = false + let fetchCacheServer: http.Server + let fetchCacheRequests: Array<{ + url: string + method: string + headers: Record + }> = [] + let fetchCacheEnv: Record = { + SUSPENSE_CACHE_PROTO: 'http', + } + + const setupNext = async ({ + nextEnv, + minimalMode, + }: { + nextEnv?: boolean + minimalMode?: boolean + }) => { + // test build against environment with next support + process.env.NOW_BUILDER = nextEnv ? '1' : '' + + next = await createNext({ + files: { + app: new FileRef(join(__dirname, 'app')), + }, + nextConfig: { + eslint: { + ignoreDuringBuilds: true, + }, + output: 'standalone', + }, + }) + await next.stop() + + await fs.move( + join(next.testDir, '.next/standalone'), + join(next.testDir, 'standalone') + ) + for (const file of await fs.readdir(next.testDir)) { + if (file !== 'standalone') { + await fs.remove(join(next.testDir, file)) + console.log('removed', file) + } + } + const files = glob.sync('**/*', { + cwd: join(next.testDir, 'standalone/.next/server/pages'), + dot: true, + }) + + for (const file of files) { + if (file.endsWith('.json') || file.endsWith('.html')) { + await fs.remove(join(next.testDir, '.next/server', file)) + } + } + + const testServer = join(next.testDir, 'standalone/server.js') + await fs.writeFile( + testServer, + (await fs.readFile(testServer, 'utf8')).replace( + 'port:', + `minimalMode: ${minimalMode},port:` + ) + ) + appPort = await findPort() + nextInstance = await initNextServerScript( + testServer, + /- Local:/, + { + ...process.env, + ...fetchCacheEnv, + PORT: appPort, + }, + undefined, + { + cwd: next.testDir, + onStderr(data) { + cliOuptut += data + }, + onStdout(data) { + cliOuptut += data + }, + } + ) + } + + beforeAll(async () => { + fetchGetReqIndex = 0 + revalidateReqIndex = 0 + fetchCacheRequests = [] + fetchGetShouldError = false + fetchCacheServer = http.createServer((req, res) => { + console.log(`fetch cache request ${req.url} ${req.method}`, req.headers) + const parsedUrl = new URL(req.url || '/', 'http://n') + + fetchCacheRequests.push({ + url: req.url, + method: req.method?.toLowerCase(), + headers: req.headers, + }) + + if (parsedUrl.pathname === '/v1/suspense-cache/revalidate') { + revalidateReqIndex += 1 + // timeout unless it's 3rd retry + const shouldTimeout = revalidateReqIndex % 3 !== 0 + + if (shouldTimeout) { + console.log('not responding for', req.url, { revalidateReqIndex }) + return + } + res.statusCode = 200 + res.end(`revalidated ${parsedUrl.searchParams.get('tags')}`) + return + } + const keyMatches = parsedUrl.pathname.match( + /\/v1\/suspense-cache\/(.*?)\/?$/ + ) + const key = keyMatches?.[0] + + if (key) { + const type = req.method?.toLowerCase() + console.log(`got ${type} for ${key}`) + + if (type === 'get') { + fetchGetReqIndex += 1 + + if (fetchGetShouldError) { + res.statusCode = 500 + res.end('internal server error') + return + } + } + res.statusCode = type === 'post' ? 200 : 404 + res.end(`${type} for ${key}`) + return + } + res.statusCode = 404 + res.end('not found') + }) + await new Promise(async (resolve) => { + let fetchCachePort = await findPort() + fetchCacheServer.listen(fetchCachePort, () => { + fetchCacheEnv['SUSPENSE_CACHE_URL'] = `[::]:${fetchCachePort}` + console.log( + `Started fetch cache server at http://${fetchCacheEnv['SUSPENSE_CACHE_URL']}` + ) + resolve() + }) + }) + await setupNext({ nextEnv: true, minimalMode: true }) + }) + afterAll(async () => { + await next.destroy() + if (fetchCacheServer) fetchCacheServer.close() + if (nextInstance) await killApp(nextInstance) + }) + + it('should have correct fetchUrl field for fetches and unstable_cache', async () => { + const res = await fetchViaHTTP(appPort, '/?myKey=myValue') + const html = await res.text() + + expect(res.status).toBe(200) + expect(html).toContain('hello world') + + const fetchUrlHeader = 'x-vercel-cache-item-name' + const fetchTagsHeader = 'x-vercel-cache-tags' + const fetchSoftTagsHeader = 'x-next-cache-soft-tags' + const unstableCacheSet = fetchCacheRequests.find((item) => { + return ( + item.method === 'get' && + item.headers[fetchUrlHeader]?.includes('unstable_cache') + ) + }) + const fetchSet = fetchCacheRequests.find((item) => { + return ( + item.method === 'get' && + item.headers[fetchUrlHeader]?.includes('next-data-api-endpoint') + ) + }) + + expect(unstableCacheSet.headers[fetchUrlHeader]).toMatch( + /unstable_cache \/\?myKey=myValue .*?/ + ) + expect(unstableCacheSet.headers[fetchTagsHeader]).toBe('thankyounext') + expect(unstableCacheSet.headers[fetchSoftTagsHeader]).toBe( + '_N_T_/layout,_N_T_/page,_N_T_/' + ) + expect(fetchSet.headers[fetchUrlHeader]).toBe( + 'https://next-data-api-endpoint.vercel.app/api/random?a=b' + ) + expect(fetchSet.headers[fetchSoftTagsHeader]).toBe( + '_N_T_/layout,_N_T_/page,_N_T_/' + ) + expect(fetchSet.headers[fetchTagsHeader]).toBe('thankyounext') + }) + + it('should retry 3 times when revalidate times out', async () => { + await fetchViaHTTP(appPort, '/api/revalidate') + + await retry(() => { + expect(revalidateReqIndex).toBe(3) + }) + expect(cliOuptut).not.toContain('Failed to revalidate') + expect(cliOuptut).not.toContain('Error') + }) + + it('should not retry for failed fetch-cache GET', async () => { + fetchGetShouldError = true + const fetchGetReqIndexStart = fetchGetReqIndex + + try { + await fetchViaHTTP(appPort, '/api/revalidate') + const res = await fetchViaHTTP(appPort, '/') + expect(res.status).toBe(200) + expect(await res.text()).toContain('hello world') + expect(fetchGetReqIndex).toBe(fetchGetReqIndexStart + 2) + } finally { + fetchGetShouldError = false + } + }) +}) diff --git a/test/turbopack-build-tests-manifest.json b/test/turbopack-build-tests-manifest.json index edc3c0dac2500..6ff821b17121a 100644 --- a/test/turbopack-build-tests-manifest.json +++ b/test/turbopack-build-tests-manifest.json @@ -1,6 +1,17 @@ { "version": 2, "suites": { + "test/production/app-dir/fetch-cache/fetch-cache.test.ts": { + "passed": [], + "failed": [ + "fetch-cache should have correct fetchUrl field for fetches and unstable_cache", + "fetch-cache should retry 3 times when revalidate times out", + "fetch-cache should not retry for failed fetch-cache GET" + ], + "pending": [], + "flakey": [], + "runtimeError": false + }, "test/e2e/404-page-router/index.test.ts": { "passed": [ "404-page-router 404-page-router with basePath of false and i18n of false and middleware false for /error should have the correct router parameters after it is ready",