diff --git a/src/run/handlers/server.ts b/src/run/handlers/server.ts index 55960b272b..2c6853a655 100644 --- a/src/run/handlers/server.ts +++ b/src/run/handlers/server.ts @@ -113,34 +113,31 @@ export default async (request: Request) => { setVaryHeaders(response.headers, request, nextConfig) setCacheStatusHeader(response.headers) - // Temporary workaround for an issue where sending a response with an empty - // body causes an unhandled error. This doesn't catch everything, but redirects are the - // most common case of sending empty bodies. We can't check it directly because these are streams. - // The side effect is that responses which do contain data will not be streamed to the client, - // but that's fine for redirects. - // TODO: Remove once a fix has been rolled out. - if ((response.status > 300 && response.status < 400) || response.status >= 500) { - const body = await response.text() - return new Response(body || null, response) + async function waitForBackgroundWork() { + // it's important to keep the stream open until the next handler has finished + await nextHandlerPromise + + // Next.js relies on `close` event emitted by response to trigger running callback variant of `next/after` + // however @fastly/http-compute-js never actually emits that event - so we have to emit it ourselves, + // otherwise Next would never run the callback variant of `next/after` + res.emit('close') + + // We have to keep response stream open until tracked background promises that are don't use `context.waitUntil` + // are resolved. If `context.waitUntil` is available, `requestContext.backgroundWorkPromise` will be empty + // resolved promised and so awaiting it is no-op + await requestContext.backgroundWorkPromise } const keepOpenUntilNextFullyRendered = new TransformStream({ async flush() { - // it's important to keep the stream open until the next handler has finished - await nextHandlerPromise - - // Next.js relies on `close` event emitted by response to trigger running callback variant of `next/after` - // however @fastly/http-compute-js never actually emits that event - so we have to emit it ourselves, - // otherwise Next would never run the callback variant of `next/after` - res.emit('close') - - // We have to keep response stream open until tracked background promises that are don't use `context.waitUntil` - // are resolved. If `context.waitUntil` is available, `requestContext.backgroundWorkPromise` will be empty - // resolved promised and so awaiting it is no-op - await requestContext.backgroundWorkPromise + await waitForBackgroundWork() }, }) + if (!response.body) { + await waitForBackgroundWork() + } + return new Response(response.body?.pipeThrough(keepOpenUntilNextFullyRendered), response) }) } diff --git a/tests/e2e/page-router.test.ts b/tests/e2e/page-router.test.ts index a64d02de9f..06f8abc29d 100644 --- a/tests/e2e/page-router.test.ts +++ b/tests/e2e/page-router.test.ts @@ -494,6 +494,45 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => { '.env.production.local': 'defined in .env.production.local', }) }) + + test('ISR pages that are the same after regeneration execute background getStaticProps uninterrupted', async ({ + page, + pageRouter, + }) => { + const slug = Date.now() + + await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href) + + await new Promise((resolve) => setTimeout(resolve, 15_000)) + + await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href) + + await new Promise((resolve) => setTimeout(resolve, 15_000)) + + await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href) + + await new Promise((resolve) => setTimeout(resolve, 15_000)) + + // keep lambda executing to allow for background getStaticProps to finish in case background work execution was suspended + await fetch(new URL(`api/sleep-5`, pageRouter.url).href) + + const response = await fetch(new URL(`read-static-props-blobs/${slug}`, pageRouter.url).href) + expect(response.ok, 'response for stored data status should not fail').toBe(true) + + const data = await response.json() + + expect(typeof data.start, 'timestamp of getStaticProps start should be a number').toEqual( + 'number', + ) + expect(typeof data.end, 'timestamp of getStaticProps end should be a number').toEqual('number') + + // duration should be around 5s overall, due to 5s timeout, but this is not exact so let's be generous and allow 10 seconds + // which is still less than 15 seconds between requests + expect( + data.end - data.start, + 'getStaticProps duration should not be longer than 10 seconds', + ).toBeLessThan(10_000) + }) }) test.describe('Page Router with basePath and i18n', () => { diff --git a/tests/fixtures/page-router/netlify/functions/read-static-props-blobs.ts b/tests/fixtures/page-router/netlify/functions/read-static-props-blobs.ts new file mode 100644 index 0000000000..ad88d87be5 --- /dev/null +++ b/tests/fixtures/page-router/netlify/functions/read-static-props-blobs.ts @@ -0,0 +1,27 @@ +import { getDeployStore } from '@netlify/blobs' +import { Context } from '@netlify/functions' + +function numberOrNull(value: string | null) { + if (!value) { + return null + } + + const maybeNumber = parseInt(value) + return isNaN(maybeNumber) ? null : maybeNumber +} + +// intentionally using Netlify Function to not hit Next.js server handler function instance +// to avoid potentially resuming suspended execution +export default async function handler(_request: Request, context: Context) { + const slug = context.params['slug'] + + const store = getDeployStore({ name: 'get-static-props-tracker', consistency: 'strong' }) + + const [start, end] = await Promise.all([store.get(`${slug}-start`), store.get(`${slug}-end`)]) + + return Response.json({ slug, start: numberOrNull(start), end: numberOrNull(end) }) +} + +export const config = { + path: '/read-static-props-blobs/:slug', +} diff --git a/tests/fixtures/page-router/package.json b/tests/fixtures/page-router/package.json index a7485b1a32..8595183eab 100644 --- a/tests/fixtures/page-router/package.json +++ b/tests/fixtures/page-router/package.json @@ -8,6 +8,7 @@ "build": "next build" }, "dependencies": { + "@netlify/blobs": "^8.1.0", "@netlify/functions": "^2.7.0", "next": "latest", "react": "18.2.0", diff --git a/tests/fixtures/page-router/pages/always-the-same-body/[slug].js b/tests/fixtures/page-router/pages/always-the-same-body/[slug].js new file mode 100644 index 0000000000..3611903021 --- /dev/null +++ b/tests/fixtures/page-router/pages/always-the-same-body/[slug].js @@ -0,0 +1,50 @@ +import { getDeployStore } from '@netlify/blobs' + +const Show = ({ slug }) => { + // ensure that the content is stable to trigger 304 responses + return
{slug}
+} + +/** @type {import('next').getStaticPaths} */ +export async function getStaticPaths() { + return { + paths: [], + fallback: 'blocking', + } +} + +/** @type {import('next').GetStaticProps} */ +export async function getStaticProps({ params }) { + const store = getDeployStore({ name: 'get-static-props-tracker', consistency: 'strong' }) + + const start = Date.now() + + console.log(`[timestamp] ${params.slug} getStaticProps start`) + + const storeStartPromise = store.set(`${params.slug}-start`, start).then(() => { + console.log(`[timestamp] ${params.slug} getStaticProps start stored`) + }) + + // simulate a long running operation + await new Promise((resolve) => setTimeout(resolve, 5000)) + + const storeEndPromise = store.set(`${params.slug}-end`, Date.now()).then(() => { + console.log(`[timestamp] ${params.slug} getStaticProps end stored`) + }) + + console.log( + `[timestamp] ${params.slug} getStaticProps end (duration: ${(Date.now() - start) / 1000}s)`, + ) + + await Promise.all([storeStartPromise, storeEndPromise]) + + // ensure that the data is stable and always the same to trigger 304 responses + return { + props: { + slug: params.slug, + }, + revalidate: 5, + } +} + +export default Show diff --git a/tests/fixtures/page-router/pages/api/sleep-5.js b/tests/fixtures/page-router/pages/api/sleep-5.js new file mode 100644 index 0000000000..c6dca97de2 --- /dev/null +++ b/tests/fixtures/page-router/pages/api/sleep-5.js @@ -0,0 +1,5 @@ +export default async function handler(req, res) { + await new Promise((resolve) => setTimeout(resolve, 5000)) + + res.json({ message: 'ok' }) +}