From 00988cca3c558d81414ad58f22b0918bb7086a13 Mon Sep 17 00:00:00 2001 From: gffuma Date: Mon, 30 Oct 2023 10:06:27 +0100 Subject: [PATCH 1/4] Prevent caching page with 304 status --- packages/next/src/server/base-server.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 5613268d505d3..4e915bba0ed2e 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -2493,8 +2493,19 @@ export default abstract class Server { return null } + const resultToCache = + result?.value?.kind === 'PAGE' && result.value.status === 304 + ? { + ...result, + value: { + ...result.value, + status: 200, + }, + } + : result + return { - ...result, + ...resultToCache, revalidate: result.revalidate !== undefined ? result.revalidate From b33c9c615aa0cda33c5928fbfdf531133e79e1fa Mon Sep 17 00:00:00 2001 From: gffuma Date: Tue, 31 Oct 2023 09:55:18 +0100 Subject: [PATCH 2/4] Test for 304 page cache repro --- .../app-dir-prevent-304-caching/app/layout.js | 7 +++ .../app-dir-prevent-304-caching/app/page.js | 9 +++ .../app-dir-prevent-304-caching/index.test.ts | 60 +++++++++++++++++++ .../next.config.js | 4 ++ 4 files changed, 80 insertions(+) create mode 100644 test/production/app-dir-prevent-304-caching/app/layout.js create mode 100644 test/production/app-dir-prevent-304-caching/app/page.js create mode 100644 test/production/app-dir-prevent-304-caching/index.test.ts create mode 100644 test/production/app-dir-prevent-304-caching/next.config.js diff --git a/test/production/app-dir-prevent-304-caching/app/layout.js b/test/production/app-dir-prevent-304-caching/app/layout.js new file mode 100644 index 0000000000000..750eb927b1980 --- /dev/null +++ b/test/production/app-dir-prevent-304-caching/app/layout.js @@ -0,0 +1,7 @@ +export default function Layout({ children }) { + return ( + + {children} + + ) +} diff --git a/test/production/app-dir-prevent-304-caching/app/page.js b/test/production/app-dir-prevent-304-caching/app/page.js new file mode 100644 index 0000000000000..202198743a23a --- /dev/null +++ b/test/production/app-dir-prevent-304-caching/app/page.js @@ -0,0 +1,9 @@ +export default async function Page() { + const time = await new Promise((resolve) => { + setTimeout(500, resolve(new Date().getTime())) + }) + + return
Time: {time}
+} + +export const revalidate = 1 diff --git a/test/production/app-dir-prevent-304-caching/index.test.ts b/test/production/app-dir-prevent-304-caching/index.test.ts new file mode 100644 index 0000000000000..d27ffc4f108c1 --- /dev/null +++ b/test/production/app-dir-prevent-304-caching/index.test.ts @@ -0,0 +1,60 @@ +import { createNext, FileRef } from 'e2e-utils' +import { NextInstance } from 'test/lib/next-modes/base' +import { join } from 'path' +import { fetchViaHTTP, waitFor } from 'next-test-utils' + +describe('app-dir-prevent-304-caching', () => { + let next: NextInstance + + beforeAll(async () => { + next = await createNext({ + files: { + 'next.config.js': new FileRef(join(__dirname, 'next.config.js')), + app: new FileRef(join(__dirname, 'app')), + }, + }) + }) + afterAll(() => next.destroy()) + + // https://github.com/vercel/next.js/issues/56580 + it('should not cache 304 status', async () => { + // Fresh call + const rFresh = await fetchViaHTTP(next.url, '/') + expect(rFresh.status).toBe(200) + + await waitFor(500) + + // Cache HIT but still 200 + const rHit = await fetchViaHTTP( + next.url, + '/', + {}, + { + headers: { + 'If-None-Match': rFresh.headers.get('etag'), + }, + } + ) + expect(rHit.status).toBe(200) + await waitFor(500) + + // Here happens the race condition + const r304 = await fetchViaHTTP( + next.url, + '/', + {}, + { + headers: { + 'If-None-Match': rHit.headers.get('etag'), + }, + } + ) + expect(r304.status).toBe(304) + // ... Postponed but should not save 304 ... + await waitFor(1000) + + // Now without cache headers should 200 + const rStillFresh = await fetchViaHTTP(next.url, '/') + expect(rStillFresh.status).toBe(200) + }) +}) diff --git a/test/production/app-dir-prevent-304-caching/next.config.js b/test/production/app-dir-prevent-304-caching/next.config.js new file mode 100644 index 0000000000000..d09bdc66ec612 --- /dev/null +++ b/test/production/app-dir-prevent-304-caching/next.config.js @@ -0,0 +1,4 @@ +/** @type {import("next").NextConfig} */ +const nextConfig = {} + +module.exports = nextConfig From 77ba424097e3b0be4191024e17ef661dbb0f85e1 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Tue, 31 Oct 2023 15:58:52 -0700 Subject: [PATCH 3/4] rework fix --- packages/next/src/server/base-server.ts | 53 +++++++++++++++---------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 4e915bba0ed2e..482d35a9db9eb 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -2058,10 +2058,13 @@ export default abstract class Server { }) type Renderer = ( - postponed: string | undefined + postponed: string | undefined, + previousCacheEntry: + | import('./response-cache/types').IncrementalCacheItem + | undefined ) => Promise - const doRender: Renderer = async (postponed) => { + const doRender: Renderer = async (postponed, previousCacheEntry) => { // In development, we always want to generate dynamic HTML. const supportsDynamicHTML = (!isDataReq && opts.dev) || !(isSSG || hasStaticPaths) || !!postponed @@ -2126,6 +2129,7 @@ export default abstract class Server { // Legacy render methods will return a render result that needs to be // served by the server. let result: RenderResult + let curStatusCode = res.statusCode if (components.routeModule?.definition.kind === RouteKind.APP_ROUTE) { const routeModule = components.routeModule as AppRouteRouteModule @@ -2250,11 +2254,29 @@ export default abstract class Server { // https://github.com/vercel/next.js/blob/df7cbd904c3bd85f399d1ce90680c0ecf92d2752/packages/next/server/render.tsx#L947-L952 renderOpts.nextFontManifest = this.nextFontManifest + // wrap response in a proxy to prevent mutating original + // response during a revalidation + const wrappedRes = new Proxy( + (res as NodeNextResponse).originalResponse ?? + (res as WebNextResponse), + { + set(target, prop, value) { + if (previousCacheEntry) { + if (prop === 'statusCode') { + curStatusCode = 200 + } + } else { + ;(target as any)[prop] = value + } + return true + }, + } + ) + // Call the built-in render method on the module. result = await module.render( (req as NodeNextRequest).originalRequest ?? (req as WebNextRequest), - (res as NodeNextResponse).originalResponse ?? - (res as WebNextResponse), + wrappedRes, { page: is404Page ? '/404' : pathname, params: opts.params, @@ -2344,7 +2366,7 @@ export default abstract class Server { pageData: metadata.pageData, postponed: metadata.postponed, headers, - status: isAppPath ? res.statusCode : undefined, + status: isAppPath ? curStatusCode : undefined, }, revalidate: metadata.revalidate, } @@ -2477,7 +2499,7 @@ export default abstract class Server { // We pass `undefined` as there cannot be a postponed state in // development. - const result = await doRender(undefined) + const result = await doRender(undefined, previousCacheEntry) if (!result) { return null } @@ -2488,24 +2510,13 @@ export default abstract class Server { } } - const result = await doRender(postponed) + const result = await doRender(postponed, previousCacheEntry) if (!result) { return null } - const resultToCache = - result?.value?.kind === 'PAGE' && result.value.status === 304 - ? { - ...result, - value: { - ...result.value, - status: 200, - }, - } - : result - return { - ...resultToCache, + ...result, revalidate: result.revalidate !== undefined ? result.revalidate @@ -2683,7 +2694,7 @@ export default abstract class Server { // respond with the dynamic flight data. In the case that this is a // resume request the page data will already be dynamic. if (!isAppPrefetch && !resumed) { - const result = await doRender(cachedData.postponed) + const result = await doRender(cachedData.postponed, undefined) if (!result) { return null } @@ -2735,7 +2746,7 @@ export default abstract class Server { // Perform the render again, but this time, provide the postponed state. // We don't await because we want the result to start streaming now, and // we've already chained the transformer's readable to the render result. - doRender(cachedData.postponed) + doRender(cachedData.postponed, undefined) .then(async (result) => { if (!result) { throw new Error('Invariant: expected a result to be returned') From 40371f35d8322a8d2bae8c1f81262068a7f5b950 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Tue, 31 Oct 2023 16:57:18 -0700 Subject: [PATCH 4/4] simplify --- packages/next/src/server/base-server.ts | 44 +++++++------------------ 1 file changed, 12 insertions(+), 32 deletions(-) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 482d35a9db9eb..43e7aaaeede80 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -1491,6 +1491,7 @@ export default abstract class Server { return } const { req, res } = ctx + const originalStatus = res.statusCode const { body, type } = payload let { revalidate } = payload if (!res.sent) { @@ -1502,13 +1503,14 @@ export default abstract class Server { revalidate = undefined } - return this.sendRenderResult(req, res, { + await this.sendRenderResult(req, res, { result: body, type, generateEtags, poweredByHeader, revalidate, }) + res.statusCode = originalStatus } } @@ -2058,13 +2060,10 @@ export default abstract class Server { }) type Renderer = ( - postponed: string | undefined, - previousCacheEntry: - | import('./response-cache/types').IncrementalCacheItem - | undefined + postponed: string | undefined ) => Promise - const doRender: Renderer = async (postponed, previousCacheEntry) => { + const doRender: Renderer = async (postponed) => { // In development, we always want to generate dynamic HTML. const supportsDynamicHTML = (!isDataReq && opts.dev) || !(isSSG || hasStaticPaths) || !!postponed @@ -2129,7 +2128,6 @@ export default abstract class Server { // Legacy render methods will return a render result that needs to be // served by the server. let result: RenderResult - let curStatusCode = res.statusCode if (components.routeModule?.definition.kind === RouteKind.APP_ROUTE) { const routeModule = components.routeModule as AppRouteRouteModule @@ -2254,29 +2252,11 @@ export default abstract class Server { // https://github.com/vercel/next.js/blob/df7cbd904c3bd85f399d1ce90680c0ecf92d2752/packages/next/server/render.tsx#L947-L952 renderOpts.nextFontManifest = this.nextFontManifest - // wrap response in a proxy to prevent mutating original - // response during a revalidation - const wrappedRes = new Proxy( - (res as NodeNextResponse).originalResponse ?? - (res as WebNextResponse), - { - set(target, prop, value) { - if (previousCacheEntry) { - if (prop === 'statusCode') { - curStatusCode = 200 - } - } else { - ;(target as any)[prop] = value - } - return true - }, - } - ) - // Call the built-in render method on the module. result = await module.render( (req as NodeNextRequest).originalRequest ?? (req as WebNextRequest), - wrappedRes, + (res as NodeNextResponse).originalResponse ?? + (res as WebNextResponse), { page: is404Page ? '/404' : pathname, params: opts.params, @@ -2366,7 +2346,7 @@ export default abstract class Server { pageData: metadata.pageData, postponed: metadata.postponed, headers, - status: isAppPath ? curStatusCode : undefined, + status: isAppPath ? res.statusCode : undefined, }, revalidate: metadata.revalidate, } @@ -2499,7 +2479,7 @@ export default abstract class Server { // We pass `undefined` as there cannot be a postponed state in // development. - const result = await doRender(undefined, previousCacheEntry) + const result = await doRender(undefined) if (!result) { return null } @@ -2510,7 +2490,7 @@ export default abstract class Server { } } - const result = await doRender(postponed, previousCacheEntry) + const result = await doRender(postponed) if (!result) { return null } @@ -2694,7 +2674,7 @@ export default abstract class Server { // respond with the dynamic flight data. In the case that this is a // resume request the page data will already be dynamic. if (!isAppPrefetch && !resumed) { - const result = await doRender(cachedData.postponed, undefined) + const result = await doRender(cachedData.postponed) if (!result) { return null } @@ -2746,7 +2726,7 @@ export default abstract class Server { // Perform the render again, but this time, provide the postponed state. // We don't await because we want the result to start streaming now, and // we've already chained the transformer's readable to the render result. - doRender(cachedData.postponed, undefined) + doRender(cachedData.postponed) .then(async (result) => { if (!result) { throw new Error('Invariant: expected a result to be returned')