Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Segment Cache] Respond with 204 on cache miss #73649

Merged
merged 1 commit into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions packages/next/src/client/components/segment-cache/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,17 @@ async function fetchRouteOnCacheMiss(
const nextUrl = key.nextUrl
try {
const response = await fetchSegmentPrefetchResponse(href, '/_tree', nextUrl)
if (!response || !response.ok || !response.body) {
// Received an unexpected response.
if (
!response ||
!response.ok ||
// 204 is a Cache miss. Though theoretically this shouldn't happen when
// PPR is enabled, because we always respond to route tree requests, even
// if it needs to be blockingly generated on demand.
response.status === 204 ||
!response.body
) {
// Server responded with an error, or with a miss. We should still cache
// the response, but we can try again after 10 seconds.
rejectRouteCacheEntry(entry, Date.now() + 10 * 1000)
return
}
Expand Down Expand Up @@ -594,9 +603,14 @@ async function fetchSegmentEntryOnCacheMiss(
accessToken === '' ? segmentPath : `${segmentPath}.${accessToken}`,
routeKey.nextUrl
)
if (!response || !response.ok || !response.body) {
// Server responded with an error. We should still cache the response, but
// we can try again after 10 seconds.
if (
!response ||
!response.ok ||
response.status === 204 || // Cache miss
!response.body
) {
// Server responded with an error, or with a miss. We should still cache
// the response, but we can try again after 10 seconds.
rejectSegmentCacheEntry(segmentCacheEntry, Date.now() + 10 * 1000)
return
}
Expand Down
88 changes: 47 additions & 41 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3044,49 +3044,55 @@ export default abstract class Server<
}
)

if (
isRoutePPREnabled &&
isPrefetchRSCRequest &&
typeof segmentPrefetchHeader === 'string'
) {
if (cacheEntry?.value?.kind === CachedRouteKind.APP_PAGE) {
// This is a prefetch request for an individual segment's static data.
// Unless the segment is fully dynamic, the data should have already been
// loaded into the cache, when the page itself was generated. So we should
// always either return the cache entry. If no cache entry is available,
// it's a 404 — either the segment is fully dynamic, or an invalid segment
// path was requested.
if (cacheEntry.value.segmentData) {
const matchedSegment = cacheEntry.value.segmentData.get(
segmentPrefetchHeader
)
if (matchedSegment !== undefined) {
return {
type: 'rsc',
body: RenderResult.fromStatic(matchedSegment),
// TODO: Eventually this should use revalidate time of the
// individual segment, not the whole page.
revalidate: cacheEntry.revalidate,
}
if (isPrefetchRSCRequest && typeof segmentPrefetchHeader === 'string') {
// This is a prefetch request issued by the client Segment Cache. These
// should never reach the application layer (lambda). We should either
// respond from the cache (HIT) or respond with 204 No Content (MISS).
if (
cacheEntry !== null &&
// This is always true at runtime but is needed to refine the type
// of cacheEntry.value to CachedAppPageValue, because the outer
// ResponseCacheEntry is not a discriminated union.
cacheEntry.value?.kind === CachedRouteKind.APP_PAGE &&
cacheEntry.value.segmentData
) {
const matchedSegment = cacheEntry.value.segmentData.get(
segmentPrefetchHeader
)
if (matchedSegment !== undefined) {
// Cache hit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also have been lazily generated in case that matters for context

return {
type: 'rsc',
body: RenderResult.fromStatic(matchedSegment),
// TODO: Eventually this should use revalidate time of the
// individual segment, not the whole page.
revalidate: cacheEntry.revalidate,
}
}
// If the segment is not found, return a 404. Since this is an RSC
// request, there's no reason to render a 404 page; just return an
// empty response.
res.statusCode = 404
return {
type: 'rsc',
body: RenderResult.fromStatic(''),
revalidate: cacheEntry.revalidate,
}
} else {
// Segment prefetches should never reach the application layer. If
// there's no cache entry for this page, it's a 404.
res.statusCode = 404
return {
type: 'rsc',
body: RenderResult.fromStatic(''),
}
}

// Cache miss. Either a cache entry for this route has not been generated,
// or there's no match for the requested segment. Regardless, respond with
// a 204 No Content. We don't bother to respond with 404 in cases where
// the segment does not exist, because these requests are only issued by
// the client cache.
// TODO: If this is a request for the route tree (the special /_tree
// segment), we should *always* respond with a tree, even if PPR
// is disabled.
res.statusCode = 204
if (isRoutePPREnabled) {
// Set a header to indicate that PPR is enabled for this route. This
// lets the client distinguish between a regular cache miss and a cache
// miss due to PPR being disabled.
// NOTE: Theoretically, when PPR is enabled, there should *never* be
// a cache miss because we should generate a fallback route. So this
// is mostly defensive.
res.setHeader(NEXT_DID_POSTPONE_HEADER, '1')
}
return {
type: 'rsc',
body: RenderResult.fromStatic(''),
revalidate: cacheEntry?.revalidate,
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ describe('per segment prefetching', () => {
expect(childResponseText).toInclude('"rsc"')
})

it('respond with 404 if the segment does not have prefetch data', async () => {
it('respond with 204 if the segment does not have prefetch data', async () => {
const response = await prefetch('/en', '/does-not-exist')
expect(response.status).toBe(404)
expect(response.status).toBe(204)
const responseText = await response.text()
expect(responseText.trim()).toBe('')
})
Expand Down
11 changes: 11 additions & 0 deletions test/e2e/app-dir/segment-cache/incremental-opt-in/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export default function RootLayout({
children,
}: {
children: React.ReactNode
}) {
return (
<html lang="en">
<body>{children}</body>
</html>
)
}
19 changes: 19 additions & 0 deletions test/e2e/app-dir/segment-cache/incremental-opt-in/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import Link from 'next/link'

export default function Page() {
return (
<ul>
<li>
<Link href="/ppr-enabled">Page with PPR enabled</Link>
</li>
<li>
<Link href="/ppr-enabled/dynamic-param">
Page with PPR enabled but has dynamic param
</Link>
</li>
<li>
<Link href="/ppr-disabled">Page with PPR disabled</Link>
</li>
</ul>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function PPRDisabled() {
return '(intentionally empty)'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return '(intentionally empty)'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export const experimental_ppr = true

export default function RootLayout({
children,
}: {
children: React.ReactNode
}) {
return children
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function PPREnabled() {
return '(intentionally empty)'
}
12 changes: 12 additions & 0 deletions test/e2e/app-dir/segment-cache/incremental-opt-in/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {
experimental: {
ppr: 'incremental',
dynamicIO: true,
clientSegmentCache: true,
},
}

module.exports = nextConfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { nextTestSetup } from 'e2e-utils'

describe('segment cache (incremental opt in)', () => {
const { next, isNextDev, skipped } = nextTestSetup({
files: __dirname,
skipDeployment: true,
})
if (isNextDev || skipped) {
test('ppr is disabled', () => {})
return
}

// TODO: Replace with e2e test once the client part is implemented
it('prefetch responds with 204 if PPR is disabled for a route', async () => {
await next.browser('/')
const response = await next.fetch('/ppr-disabled', {
headers: {
RSC: '1',
'Next-Router-Prefetch': '1',
'Next-Router-Segment-Prefetch': '/_tree',
},
})
expect(response.status).toBe(204)
})
})
Loading