Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ff2632f

Browse filesBrowse files
piehpieh
authored
fix: ensure background work is finished when response has 3xx or 5xx status code (#2742)
* test: add test for ISR case that always return same body * fix: ensure background work is finished when response has 3xx or 5xx status code
1 parent e77b98a commit ff2632f
Copy full SHA for ff2632f

File tree

Expand file treeCollapse file tree

6 files changed

+140
-21
lines changed
Filter options
Expand file treeCollapse file tree

6 files changed

+140
-21
lines changed

‎src/run/handlers/server.ts

Copy file name to clipboardExpand all lines: src/run/handlers/server.ts
+18-21Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -113,34 +113,31 @@ export default async (request: Request) => {
113113
setVaryHeaders(response.headers, request, nextConfig)
114114
setCacheStatusHeader(response.headers)
115115

116-
// Temporary workaround for an issue where sending a response with an empty
117-
// body causes an unhandled error. This doesn't catch everything, but redirects are the
118-
// most common case of sending empty bodies. We can't check it directly because these are streams.
119-
// The side effect is that responses which do contain data will not be streamed to the client,
120-
// but that's fine for redirects.
121-
// TODO: Remove once a fix has been rolled out.
122-
if ((response.status > 300 && response.status < 400) || response.status >= 500) {
123-
const body = await response.text()
124-
return new Response(body || null, response)
116+
async function waitForBackgroundWork() {
117+
// it's important to keep the stream open until the next handler has finished
118+
await nextHandlerPromise
119+
120+
// Next.js relies on `close` event emitted by response to trigger running callback variant of `next/after`
121+
// however @fastly/http-compute-js never actually emits that event - so we have to emit it ourselves,
122+
// otherwise Next would never run the callback variant of `next/after`
123+
res.emit('close')
124+
125+
// We have to keep response stream open until tracked background promises that are don't use `context.waitUntil`
126+
// are resolved. If `context.waitUntil` is available, `requestContext.backgroundWorkPromise` will be empty
127+
// resolved promised and so awaiting it is no-op
128+
await requestContext.backgroundWorkPromise
125129
}
126130

127131
const keepOpenUntilNextFullyRendered = new TransformStream({
128132
async flush() {
129-
// it's important to keep the stream open until the next handler has finished
130-
await nextHandlerPromise
131-
132-
// Next.js relies on `close` event emitted by response to trigger running callback variant of `next/after`
133-
// however @fastly/http-compute-js never actually emits that event - so we have to emit it ourselves,
134-
// otherwise Next would never run the callback variant of `next/after`
135-
res.emit('close')
136-
137-
// We have to keep response stream open until tracked background promises that are don't use `context.waitUntil`
138-
// are resolved. If `context.waitUntil` is available, `requestContext.backgroundWorkPromise` will be empty
139-
// resolved promised and so awaiting it is no-op
140-
await requestContext.backgroundWorkPromise
133+
await waitForBackgroundWork()
141134
},
142135
})
143136

137+
if (!response.body) {
138+
await waitForBackgroundWork()
139+
}
140+
144141
return new Response(response.body?.pipeThrough(keepOpenUntilNextFullyRendered), response)
145142
})
146143
}

‎tests/e2e/page-router.test.ts

Copy file name to clipboardExpand all lines: tests/e2e/page-router.test.ts
+39Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,45 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
494494
'.env.production.local': 'defined in .env.production.local',
495495
})
496496
})
497+
498+
test('ISR pages that are the same after regeneration execute background getStaticProps uninterrupted', async ({
499+
page,
500+
pageRouter,
501+
}) => {
502+
const slug = Date.now()
503+
504+
await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href)
505+
506+
await new Promise((resolve) => setTimeout(resolve, 15_000))
507+
508+
await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href)
509+
510+
await new Promise((resolve) => setTimeout(resolve, 15_000))
511+
512+
await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href)
513+
514+
await new Promise((resolve) => setTimeout(resolve, 15_000))
515+
516+
// keep lambda executing to allow for background getStaticProps to finish in case background work execution was suspended
517+
await fetch(new URL(`api/sleep-5`, pageRouter.url).href)
518+
519+
const response = await fetch(new URL(`read-static-props-blobs/${slug}`, pageRouter.url).href)
520+
expect(response.ok, 'response for stored data status should not fail').toBe(true)
521+
522+
const data = await response.json()
523+
524+
expect(typeof data.start, 'timestamp of getStaticProps start should be a number').toEqual(
525+
'number',
526+
)
527+
expect(typeof data.end, 'timestamp of getStaticProps end should be a number').toEqual('number')
528+
529+
// duration should be around 5s overall, due to 5s timeout, but this is not exact so let's be generous and allow 10 seconds
530+
// which is still less than 15 seconds between requests
531+
expect(
532+
data.end - data.start,
533+
'getStaticProps duration should not be longer than 10 seconds',
534+
).toBeLessThan(10_000)
535+
})
497536
})
498537

499538
test.describe('Page Router with basePath and i18n', () => {

‎tests/fixtures/page-router/netlify/functions/read-static-props-blobs.ts

Copy file name to clipboard
+27Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { getDeployStore } from '@netlify/blobs'
2+
import { Context } from '@netlify/functions'
3+
4+
function numberOrNull(value: string | null) {
5+
if (!value) {
6+
return null
7+
}
8+
9+
const maybeNumber = parseInt(value)
10+
return isNaN(maybeNumber) ? null : maybeNumber
11+
}
12+
13+
// intentionally using Netlify Function to not hit Next.js server handler function instance
14+
// to avoid potentially resuming suspended execution
15+
export default async function handler(_request: Request, context: Context) {
16+
const slug = context.params['slug']
17+
18+
const store = getDeployStore({ name: 'get-static-props-tracker', consistency: 'strong' })
19+
20+
const [start, end] = await Promise.all([store.get(`${slug}-start`), store.get(`${slug}-end`)])
21+
22+
return Response.json({ slug, start: numberOrNull(start), end: numberOrNull(end) })
23+
}
24+
25+
export const config = {
26+
path: '/read-static-props-blobs/:slug',
27+
}

‎tests/fixtures/page-router/package.json

Copy file name to clipboardExpand all lines: tests/fixtures/page-router/package.json
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
"build": "next build"
99
},
1010
"dependencies": {
11+
"@netlify/blobs": "^8.1.0",
1112
"@netlify/functions": "^2.7.0",
1213
"next": "latest",
1314
"react": "18.2.0",

‎tests/fixtures/page-router/pages/always-the-same-body/[slug].js

Copy file name to clipboard
+50Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import { getDeployStore } from '@netlify/blobs'
2+
3+
const Show = ({ slug }) => {
4+
// ensure that the content is stable to trigger 304 responses
5+
return <pre>{slug}</pre>
6+
}
7+
8+
/** @type {import('next').getStaticPaths} */
9+
export async function getStaticPaths() {
10+
return {
11+
paths: [],
12+
fallback: 'blocking',
13+
}
14+
}
15+
16+
/** @type {import('next').GetStaticProps} */
17+
export async function getStaticProps({ params }) {
18+
const store = getDeployStore({ name: 'get-static-props-tracker', consistency: 'strong' })
19+
20+
const start = Date.now()
21+
22+
console.log(`[timestamp] ${params.slug} getStaticProps start`)
23+
24+
const storeStartPromise = store.set(`${params.slug}-start`, start).then(() => {
25+
console.log(`[timestamp] ${params.slug} getStaticProps start stored`)
26+
})
27+
28+
// simulate a long running operation
29+
await new Promise((resolve) => setTimeout(resolve, 5000))
30+
31+
const storeEndPromise = store.set(`${params.slug}-end`, Date.now()).then(() => {
32+
console.log(`[timestamp] ${params.slug} getStaticProps end stored`)
33+
})
34+
35+
console.log(
36+
`[timestamp] ${params.slug} getStaticProps end (duration: ${(Date.now() - start) / 1000}s)`,
37+
)
38+
39+
await Promise.all([storeStartPromise, storeEndPromise])
40+
41+
// ensure that the data is stable and always the same to trigger 304 responses
42+
return {
43+
props: {
44+
slug: params.slug,
45+
},
46+
revalidate: 5,
47+
}
48+
}
49+
50+
export default Show

‎tests/fixtures/page-router/pages/api/sleep-5.js

Copy file name to clipboard
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
export default async function handler(req, res) {
2+
await new Promise((resolve) => setTimeout(resolve, 5000))
3+
4+
res.json({ message: 'ok' })
5+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.