Skip to content

Mark revalidate: 0 on fetch or use cache as never resolving promise in Dynamic I/O #71285

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 15, 2024
Merged

Mark revalidate: 0 on fetch or use cache as never resolving promise in Dynamic I/O #71285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d84cb05
Mark revalidate: 0 on fetch or use cache as never resolving promise
sebmarkbage Oct 15, 2024
e91326b
Don't issue dynamic fetches in Dynamic I/O prerendering
sebmarkbage Oct 15, 2024
361f86c
Update tests
sebmarkbage Oct 15, 2024
aa2d268
Abort cacheSignal early if we make a hanging promise
sebmarkbage Oct 15, 2024
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
14 changes: 2 additions & 12 deletions packages/next/src/server/app-render/dynamic-rendering.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export function getFirstDynamicReason(
*/
export function markCurrentScopeAsDynamic(
store: WorkStore,
workUnitStore: undefined | WorkUnitStore,
workUnitStore: undefined | Exclude<WorkUnitStore, PrerenderStoreModern>,
expression: string
): void {
if (workUnitStore) {
Expand All @@ -143,17 +143,7 @@ export function markCurrentScopeAsDynamic(
}

if (workUnitStore) {
if (workUnitStore.type === 'prerender') {
// We're prerendering the RSC stream with dynamicIO enabled and we need to abort the
// current render because something dynamic is being used.
// This won't throw so we still need to fall through to determine if/how we handle
// this specific dynamic request.
abortAndThrowOnSynchronousDynamicDataAccess(
store.route,
expression,
workUnitStore
)
} else if (workUnitStore.type === 'prerender-ppr') {
if (workUnitStore.type === 'prerender-ppr') {
postponeWithTracking(
store.route,
expression,
Expand Down
103 changes: 76 additions & 27 deletions packages/next/src/server/lib/patch-fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
NEXT_CACHE_TAG_MAX_LENGTH,
} from '../../lib/constants'
import { markCurrentScopeAsDynamic } from '../app-render/dynamic-rendering'
import { makeHangingPromise } from '../dynamic-rendering-utils'
import type { FetchMetric } from '../base-http'
import { createDedupeFetch } from './dedupe-fetch'
import type {
Expand Down Expand Up @@ -185,6 +186,15 @@ export function createPatchedFetcher(
const workStore = workAsyncStorage.getStore()
const workUnitStore = workUnitAsyncStorage.getStore()

// During static generation we track cache reads so we can reason about when they fill
let cacheSignal =
workUnitStore && workUnitStore.type === 'prerender'
? workUnitStore.cacheSignal
: null
if (cacheSignal) {
cacheSignal.beginRead()
}

const result = getTracer().trace(
isInternal ? NextNodeServerSpan.internalFetch : AppRenderSpan.fetch,
{
Expand Down Expand Up @@ -373,6 +383,23 @@ export function createPatchedFetcher(
revalidateStore &&
revalidateStore.revalidate === 0)

if (
hasNoExplicitCacheConfig &&
workUnitStore !== undefined &&
workUnitStore.type === 'prerender'
) {
// If we have no cache config, and we're in Dynamic I/O prerendering, it'll be a dynamic call.
// We don't have to issue that dynamic call.
if (cacheSignal) {
cacheSignal.endRead()
cacheSignal = null
}
return makeHangingPromise<Response>(
workUnitStore.renderSignal,
'fetch()'
)
}

switch (pageFetchCacheMode) {
case 'force-no-store': {
cacheReason = 'fetchCache = force-no-store'
Expand Down Expand Up @@ -454,11 +481,22 @@ export function createPatchedFetcher(
// If we were setting the revalidate value to 0, we should try to
// postpone instead first.
if (finalRevalidate === 0) {
markCurrentScopeAsDynamic(
workStore,
workUnitStore,
`revalidate: 0 fetch ${input} ${workStore.route}`
)
if (workUnitStore && workUnitStore.type === 'prerender') {
if (cacheSignal) {
cacheSignal.endRead()
cacheSignal = null
}
return makeHangingPromise<Response>(
workUnitStore.renderSignal,
'fetch()'
)
} else {
markCurrentScopeAsDynamic(
workStore,
workUnitStore,
`revalidate: 0 fetch ${input} ${workStore.route}`
)
}
}

if (revalidateStore) {
Expand Down Expand Up @@ -576,7 +614,6 @@ export function createPatchedFetcher(
if (workUnitStore && workUnitStore.type === 'prerender') {
// We are prerendering at build time or revalidate time with dynamicIO so we need to
// buffer the response so we can guarantee it can be read in a microtask

const bodyBuffer = await res.arrayBuffer()

const fetchedData = {
Expand Down Expand Up @@ -778,11 +815,22 @@ export function createPatchedFetcher(

if (cache === 'no-store') {
// If enabled, we should bail out of static generation.
markCurrentScopeAsDynamic(
workStore,
workUnitStore,
`no-store fetch ${input} ${workStore.route}`
)
if (workUnitStore && workUnitStore.type === 'prerender') {
if (cacheSignal) {
cacheSignal.endRead()
cacheSignal = null
}
return makeHangingPromise<Response>(
workUnitStore.renderSignal,
'fetch()'
)
} else {
markCurrentScopeAsDynamic(
workStore,
workUnitStore,
`no-store fetch ${input} ${workStore.route}`
)
}
}

const hasNextConfig = 'next' in init
Expand All @@ -794,11 +842,18 @@ export function createPatchedFetcher(
) {
if (next.revalidate === 0) {
// If enabled, we should bail out of static generation.
markCurrentScopeAsDynamic(
workStore,
workUnitStore,
`revalidate: 0 fetch ${input} ${workStore.route}`
)
if (workUnitStore && workUnitStore.type === 'prerender') {
return makeHangingPromise<Response>(
workUnitStore.renderSignal,
'fetch()'
)
} else {
markCurrentScopeAsDynamic(
workStore,
workUnitStore,
`revalidate: 0 fetch ${input} ${workStore.route}`
)
}
}

if (!workStore.forceStatic || next.revalidate !== 0) {
Expand Down Expand Up @@ -879,22 +934,16 @@ export function createPatchedFetcher(
}
)

if (
workUnitStore &&
workUnitStore.type === 'prerender' &&
workUnitStore.cacheSignal
) {
// During static generation we track cache reads so we can reason about when they fill
const cacheSignal = workUnitStore.cacheSignal
cacheSignal.beginRead()
if (cacheSignal) {
try {
return await result
} finally {
cacheSignal.endRead()
if (cacheSignal) {
cacheSignal.endRead()
}
}
} else {
return result
}
return result
}

// Attach the necessary properties to the patched fetch function.
Expand Down
19 changes: 19 additions & 0 deletions packages/next/src/server/use-cache/use-cache-wrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import type {
import { workUnitAsyncStorage } from '../app-render/work-unit-async-storage.external'
import { runInCleanSnapshot } from '../app-render/clean-async-snapshot.external'

import { makeHangingPromise } from '../dynamic-rendering-utils'

import { cacheScopeAsyncLocalStorage } from '../async-storage/cache-scope.external'

import type { ClientReferenceManifest } from '../../build/webpack/plugins/flight-manifest-plugin'
Expand All @@ -33,6 +35,9 @@ import type { CacheScopeStore } from '../async-storage/cache-scope.external'

const isEdgeRuntime = process.env.NEXT_RUNTIME === 'edge'

// If the expire time is less than .
const DYNAMIC_EXPIRE = 300

type CacheEntry = {
value: ReadableStream
timestamp: number
Expand Down Expand Up @@ -427,6 +432,20 @@ async function loadCacheEntry(

const currentTime = performance.timeOrigin + performance.now()
if (
workUnitStore !== undefined &&
workUnitStore.type === 'prerender' &&
entry !== undefined &&
Copy link
Contributor Author

@sebmarkbage sebmarkbage Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One thing to note here is that we can't mark this if we don't have an existing cache entry. That's because for a miss we start streaming immediately.

In practice, because we drop in memory entries from the default Cache Handler using the revalidate time. We'll actually never hit this for revalidate: 0 in the default cache.

We really need to handle this in the cacheScope but that's currently disabled pending another refactor to be able to store cache entries in the cacheScope.

(entry.revalidate === 0 || entry.expire < DYNAMIC_EXPIRE)
) {
// In a Dynamic I/O prerender, if the cache entry has revalidate: 0 or if the
// expire time is under 5 minutes, then we consider this cache entry dynamic
// as it's not worth generating static pages for such data. It's better to leave
// a PPR hole that can be filled in dynamically with a potentially cached entry.
if (cacheSignal) {
cacheSignal.endRead()
}
return makeHangingPromise(workUnitStore.renderSignal, 'dynamic "use cache"')
} else if (
entry === undefined ||
currentTime > entry.timestamp + entry.expire * 1000 ||
(workStore.isStaticGeneration &&
Expand Down
6 changes: 5 additions & 1 deletion packages/next/src/server/web/spec-extension/unstable-no-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ export function unstable_noStore() {
return
} else {
store.isUnstableNoStore = true
markCurrentScopeAsDynamic(store, workUnitStore, callingExpression)
if (workUnitStore && workUnitStore.type === 'prerender') {
// unstable_noStore() is a noop in Dynamic I/O.
} else {
markCurrentScopeAsDynamic(store, workUnitStore, callingExpression)
}
}
}
51 changes: 15 additions & 36 deletions test/e2e/app-dir/dynamic-io/dynamic-io.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,33 +288,18 @@ describe('dynamic-io', () => {
})
}

if (WITH_PPR) {
it('should partially prerender pages that use `unstable_noStore()`', async () => {
let $ = await next.render$('/cases/dynamic_api_no_store', {})
if (isNextDev) {
expect($('#layout').text()).toBe('at runtime')
expect($('#page').text()).toBe('at runtime')
expect($('#inner').text()).toBe('at runtime')
} else {
expect($('#layout').text()).toBe('at buildtime')
expect($('#page').text()).toBe('at buildtime')
expect($('#inner').text()).toBe('at buildtime')
}
})
} else {
it('should not prerender pages that use `unstable_noStore()`', async () => {
let $ = await next.render$('/cases/dynamic_api_no_store', {})
if (isNextDev) {
expect($('#layout').text()).toBe('at runtime')
expect($('#page').text()).toBe('at runtime')
expect($('#inner').text()).toBe('at runtime')
} else {
expect($('#layout').text()).toBe('at runtime')
expect($('#page').text()).toBe('at runtime')
expect($('#inner').text()).toBe('at runtime')
}
})
}
it('should fully prerender pages that use `unstable_noStore()`', async () => {
let $ = await next.render$('/cases/dynamic_api_no_store', {})
if (isNextDev) {
expect($('#layout').text()).toBe('at runtime')
expect($('#page').text()).toBe('at runtime')
expect($('#inner').text()).toBe('at runtime')
} else {
expect($('#layout').text()).toBe('at buildtime')
expect($('#page').text()).toBe('at buildtime')
expect($('#inner').text()).toBe('at buildtime')
}
})

if (WITH_PPR) {
it('should partially prerender pages that use `searchParams` in Server Components', async () => {
Expand Down Expand Up @@ -450,15 +435,9 @@ describe('dynamic-io', () => {
expect($('#page-slot').text()).toBe('at runtime')
expect($('#page-children').text()).toBe('at runtime')
} else {
if (WITH_PPR) {
expect($('#layout').text()).toBe('at buildtime')
expect($('#page-slot').text()).toBe('at runtime')
expect($('#page-children').text()).toBe('at buildtime')
} else {
expect($('#layout').text()).toBe('at runtime')
expect($('#page-slot').text()).toBe('at runtime')
expect($('#page-children').text()).toBe('at runtime')
}
expect($('#layout').text()).toBe('at buildtime')
expect($('#page-slot').text()).toBe('at buildtime')
expect($('#page-children').text()).toBe('at buildtime')
}

$ = await next.render$('/cases/parallel/cookies', {})
Expand Down
Loading