Skip to content

Commit e33bd93

Browse files
piehpieh
authoredFeb 15, 2024
perf: reuse lastModified captured in cache handler to calculate date header (#260)
* perf: reuse lastModified captured in cache handler to calculate date header * chore: record exception and set warning and severity attributes * test: spy on blobs.get usage and assert we only do it once, compare produced date headers * test: add integration test for request-context not leaking for concurrent requests * chore: remove some commented out code * fix: there is no requestContext on CacheHandler anymore
·
v5.10.5v5.0.0-beta.9
1 parent 3bd9893 commit e33bd93

File tree

10 files changed

+639
-52
lines changed

10 files changed

+639
-52
lines changed
 

‎package-lock.json

+59
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json

+1
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
"get-port": "^7.0.0",
7070
"lambda-local": "^2.1.2",
7171
"memfs": "^4.6.0",
72+
"mock-require": "^3.0.3",
7273
"msw": "^2.0.7",
7374
"next": "^14.0.4",
7475
"os": "^0.1.2",

‎src/run/handlers/cache.cts

+53-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { Buffer } from 'node:buffer'
55

66
import { getDeployStore, Store } from '@netlify/blobs'
77
import { purgeCache } from '@netlify/functions'
8-
import { trace } from '@opentelemetry/api'
8+
import { trace, type Span } from '@opentelemetry/api'
99
import { NEXT_CACHE_TAGS_HEADER } from 'next/dist/lib/constants.js'
1010
import type {
1111
CacheHandler,
@@ -14,6 +14,8 @@ import type {
1414
IncrementalCache,
1515
} from 'next/dist/server/lib/incremental-cache/index.js'
1616

17+
import { getRequestContext } from './request-context.cjs'
18+
1719
type TagManifest = { revalidatedAt: number }
1820

1921
const fetchBeforeNextPatchedIt = globalThis.fetch
@@ -35,6 +37,54 @@ export class NetlifyCacheHandler implements CacheHandler {
3537
return await encodeBlobKey(key)
3638
}
3739

40+
private captureResponseCacheLastModified(
41+
cacheValue: CacheHandlerValue,
42+
key: string,
43+
getCacheKeySpan: Span,
44+
) {
45+
if (cacheValue.value?.kind === 'FETCH') {
46+
return
47+
}
48+
49+
const requestContext = getRequestContext()
50+
51+
if (!requestContext) {
52+
// we will not be able to use request context for date header calculation
53+
// we will fallback to using blobs
54+
getCacheKeySpan.recordException(
55+
new Error('CacheHandler was called without a request context'),
56+
)
57+
getCacheKeySpan.setAttributes({
58+
severity: 'alert',
59+
warning: true,
60+
})
61+
return
62+
}
63+
64+
if (requestContext.responseCacheKey && requestContext.responseCacheKey !== key) {
65+
// if there are multiple response-cache keys, we don't know which one we should use
66+
// so as a safety measure we will not use any of them and let blobs be used
67+
// to calculate the date header
68+
requestContext.responseCacheGetLastModified = undefined
69+
getCacheKeySpan.recordException(
70+
new Error(
71+
`Multiple response cache keys used in single request: ["${requestContext.responseCacheKey}, "${key}"]`,
72+
),
73+
)
74+
getCacheKeySpan.setAttributes({
75+
severity: 'alert',
76+
warning: true,
77+
})
78+
return
79+
}
80+
81+
requestContext.responseCacheKey = key
82+
if (cacheValue.lastModified) {
83+
// we store it to use it later when calculating date header
84+
requestContext.responseCacheGetLastModified = cacheValue.lastModified
85+
}
86+
}
87+
3888
async get(...args: Parameters<CacheHandler['get']>): ReturnType<CacheHandler['get']> {
3989
return this.tracer.startActiveSpan('get cache key', async (span) => {
4090
const [key, ctx = {}] = args
@@ -61,6 +111,8 @@ export class NetlifyCacheHandler implements CacheHandler {
61111
return null
62112
}
63113

114+
this.captureResponseCacheLastModified(blob, key, span)
115+
64116
switch (blob.value?.kind) {
65117
case 'FETCH':
66118
span.addEvent('FETCH', { lastModified: blob.lastModified, revalidate: ctx.revalidate })

‎src/run/handlers/request-context.cts

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { AsyncLocalStorage } from 'node:async_hooks'
2+
3+
export type RequestContext = {
4+
responseCacheGetLastModified?: number
5+
responseCacheKey?: string
6+
}
7+
8+
type RequestContextAsyncLocalStorage = AsyncLocalStorage<RequestContext>
9+
10+
export function createRequestContext(): RequestContext {
11+
return {}
12+
}
13+
14+
const REQUEST_CONTEXT_GLOBAL_KEY = Symbol.for('nf-request-context-async-local-storage')
15+
let requestContextAsyncLocalStorage: RequestContextAsyncLocalStorage | undefined
16+
function getRequestContextAsyncLocalStorage() {
17+
if (requestContextAsyncLocalStorage) {
18+
return requestContextAsyncLocalStorage
19+
}
20+
// for cases when there is multiple "copies" of this module, we can't just init
21+
// AsyncLocalStorage in the module scope, because it will be different for each
22+
// copy - so first time an instance of this module is used, we store AsyncLocalStorage
23+
// in global scope and reuse it for all subsequent calls
24+
const extendedGlobalThis = globalThis as typeof globalThis & {
25+
[REQUEST_CONTEXT_GLOBAL_KEY]?: RequestContextAsyncLocalStorage
26+
}
27+
if (extendedGlobalThis[REQUEST_CONTEXT_GLOBAL_KEY]) {
28+
return extendedGlobalThis[REQUEST_CONTEXT_GLOBAL_KEY]
29+
}
30+
31+
const storage = new AsyncLocalStorage<RequestContext>()
32+
// store for future use of this instance of module
33+
requestContextAsyncLocalStorage = storage
34+
// store for future use of copy of this module
35+
extendedGlobalThis[REQUEST_CONTEXT_GLOBAL_KEY] = storage
36+
return storage
37+
}
38+
39+
export const getRequestContext = () => getRequestContextAsyncLocalStorage().getStore()
40+
41+
export function runWithRequestContext<T>(requestContext: RequestContext, fn: () => T): T {
42+
return getRequestContextAsyncLocalStorage().run(requestContext, fn)
43+
}

‎src/run/handlers/server.ts

+24-14
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import {
1414
import { nextResponseProxy } from '../revalidate.js'
1515
import { logger } from '../systemlog.js'
1616

17+
import { createRequestContext, runWithRequestContext } from './request-context.cjs'
18+
1719
let nextHandler: WorkerRequestHandler, nextConfig: NextConfigComplete, tagsManifest: TagsManifest
1820

1921
export default async (request: Request) => {
@@ -51,26 +53,34 @@ export default async (request: Request) => {
5153

5254
const resProxy = nextResponseProxy(res)
5355

56+
const requestContext = createRequestContext()
57+
5458
// We don't await this here, because it won't resolve until the response is finished.
55-
const nextHandlerPromise = nextHandler(req, resProxy).catch((error) => {
56-
logger.withError(error).error('next handler error')
57-
console.error(error)
58-
resProxy.statusCode = 500
59-
span.recordException(error)
60-
span.setAttribute('http.status_code', 500)
61-
span.setStatus({
62-
code: SpanStatusCode.ERROR,
63-
message: error instanceof Error ? error.message : String(error),
64-
})
65-
span.end()
66-
resProxy.end('Internal Server Error')
67-
})
59+
const nextHandlerPromise = runWithRequestContext(requestContext, () =>
60+
nextHandler(req, resProxy).catch((error) => {
61+
logger.withError(error).error('next handler error')
62+
console.error(error)
63+
resProxy.statusCode = 500
64+
span.recordException(error)
65+
span.setAttribute('http.status_code', 500)
66+
span.setStatus({
67+
code: SpanStatusCode.ERROR,
68+
message: error instanceof Error ? error.message : String(error),
69+
})
70+
span.end()
71+
resProxy.end('Internal Server Error')
72+
}),
73+
)
6874

6975
// Contrary to the docs, this resolves when the headers are available, not when the stream closes.
7076
// See https://github.com/fastly/http-compute-js/blob/main/src/http-compute-js/http-server.ts#L168-L173
7177
const response = await toComputeResponse(resProxy)
7278

73-
await adjustDateHeader(response.headers, request, span, tracer)
79+
if (requestContext.responseCacheKey) {
80+
span.setAttribute('responseCacheKey', requestContext.responseCacheKey)
81+
}
82+
83+
await adjustDateHeader({ headers: response.headers, request, span, tracer, requestContext })
7484

7585
setCacheControlHeaders(response.headers, request)
7686
setCacheTagsHeaders(response.headers, request, tagsManifest)

‎src/run/headers.ts

+76-24
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type { NextConfigComplete } from 'next/dist/server/config-shared.js'
55
import { encodeBlobKey } from '../shared/blobkey.js'
66

77
import type { TagsManifest } from './config.js'
8+
import type { RequestContext } from './handlers/request-context.cjs'
89

910
interface NetlifyVaryValues {
1011
headers: string[]
@@ -84,12 +85,19 @@ const fetchBeforeNextPatchedIt = globalThis.fetch
8485
* By default, Next.js sets the date header to the current time, even if it's served
8586
* from the cache, meaning that the CDN will cache it for 10 seconds, which is incorrect.
8687
*/
87-
export const adjustDateHeader = async (
88-
headers: Headers,
89-
request: Request,
90-
span: Span,
91-
tracer: Tracer,
92-
) => {
88+
export const adjustDateHeader = async ({
89+
headers,
90+
request,
91+
span,
92+
tracer,
93+
requestContext,
94+
}: {
95+
headers: Headers
96+
request: Request
97+
span: Span
98+
tracer: Tracer
99+
requestContext: RequestContext
100+
}) => {
93101
const cacheState = headers.get('x-nextjs-cache')
94102
const isServedFromCache = cacheState === 'HIT' || cacheState === 'STALE'
95103

@@ -102,28 +110,72 @@ export const adjustDateHeader = async (
102110
return
103111
}
104112
const key = new URL(request.url).pathname
105-
const blobKey = await encodeBlobKey(key)
106-
const blobStore = getDeployStore({ fetch: fetchBeforeNextPatchedIt, consistency: 'strong' })
107-
108-
// TODO: use metadata for this
109-
const { lastModified } = await tracer.startActiveSpan(
110-
'get cache to calculate date header',
111-
async (getBlobForDateSpan) => {
112-
getBlobForDateSpan.setAttributes({
113-
key,
114-
blobKey,
115-
})
116-
const blob = (await blobStore.get(blobKey, { type: 'json' })) ?? {}
117-
118-
getBlobForDateSpan.addEvent(blob ? 'Cache hit' : 'Cache miss')
119-
getBlobForDateSpan.end()
120-
return blob
121-
},
122-
)
113+
114+
let lastModified: number | undefined
115+
if (requestContext.responseCacheGetLastModified) {
116+
lastModified = requestContext.responseCacheGetLastModified
117+
} else {
118+
// this is fallback in case requestContext doesn't contain lastModified
119+
// using fallback indicate problem in the setup as assumption is that for cached responses
120+
// request context would contain lastModified value
121+
// this is not fatal as we have fallback,
122+
// but we want to know about it happening
123+
span.recordException(
124+
new Error('lastModified not found in requestContext, falling back to trying blobs'),
125+
)
126+
span.setAttributes({
127+
severity: 'alert',
128+
warning: true,
129+
})
130+
131+
const blobKey = await encodeBlobKey(key)
132+
const blobStore = getDeployStore({ fetch: fetchBeforeNextPatchedIt, consistency: 'strong' })
133+
134+
// TODO: use metadata for this
135+
lastModified = await tracer.startActiveSpan(
136+
'get cache to calculate date header',
137+
async (getBlobForDateSpan) => {
138+
getBlobForDateSpan.setAttributes({
139+
key,
140+
blobKey,
141+
})
142+
const blob = (await blobStore.get(blobKey, { type: 'json' })) ?? {}
143+
144+
getBlobForDateSpan.addEvent(blob ? 'Cache hit' : 'Cache miss')
145+
getBlobForDateSpan.end()
146+
return blob.lastModified
147+
},
148+
)
149+
}
123150

124151
if (!lastModified) {
152+
// this should never happen as we only execute this code path for cached responses
153+
// and those should always have lastModified value
154+
span.recordException(
155+
new Error(
156+
'lastModified not found in either requestContext or blobs, date header for cached response is not set',
157+
),
158+
)
159+
span.setAttributes({
160+
severity: 'alert',
161+
warning: true,
162+
})
125163
return
126164
}
165+
166+
if (lastModified === 1) {
167+
// epoch 1 time seems to not work well with the CDN
168+
// it causes to produce "Thu, 01 Jan 1970 00:00:00 GMT" date
169+
// header correctly but CDN calculates then AGE to be 0
170+
// causing response to be cached for entire max-age period
171+
// since the first hit
172+
// so instead of using 1 we use 1 year ago
173+
// this is done here instead at build time as it's unclear
174+
// what exactly is edge case in CDN and setting 1 year ago
175+
// from request time seems to work consistently
176+
lastModified = Date.now() - 31536000000
177+
}
178+
127179
const lastModifiedDate = new Date(lastModified)
128180
// Show actual date of the function call in the date header
129181
headers.set('x-nextjs-date', headers.get('date') ?? lastModifiedDate.toUTCString())

‎tests/integration/cache-handler.test.ts

+123-12
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
type FixtureTestContext,
1010
} from '../utils/fixture.js'
1111
import {
12+
countOfBlobServerGetsForKey,
1213
decodeBlobKey,
1314
encodeBlobKey,
1415
generateRandomObjectID,
@@ -64,6 +65,12 @@ describe('page router', () => {
6465
}),
6566
)
6667

68+
expect(
69+
countOfBlobServerGetsForKey(ctx, '/static/revalidate-automatic'),
70+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
71+
).toBe(1)
72+
ctx.blobServerGetSpy.mockClear()
73+
6774
// wait to have page regenerated in the background
6875
await new Promise<void>((resolve) => setTimeout(resolve, 1000))
6976

@@ -82,17 +89,38 @@ describe('page router', () => {
8289
)
8390
expect(
8491
call2Date.localeCompare(call1Date),
85-
'the date of regenerated page is newer than initial stale page',
92+
'the rendered date in regenerated page is newer than initial stale page',
93+
).toBeGreaterThan(0)
94+
expect(
95+
call2.headers['date'].toString().localeCompare(call1.headers['date'].toString()),
96+
'the date header of regenerated page is newer than initial stale page',
8697
).toBeGreaterThan(0)
8798

99+
expect(
100+
countOfBlobServerGetsForKey(ctx, '/static/revalidate-automatic'),
101+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
102+
).toBe(1)
103+
ctx.blobServerGetSpy.mockClear()
104+
88105
// ping that should serve the stale page for static/revalidate-slow, while revalidating in background
89106
await invokeFunction(ctx, { url: 'static/revalidate-slow' })
90107

108+
expect(
109+
countOfBlobServerGetsForKey(ctx, '/static/revalidate-slow'),
110+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
111+
).toBe(1)
112+
ctx.blobServerGetSpy.mockClear()
113+
91114
// wait to have a stale page
92115
await new Promise<void>((resolve) => setTimeout(resolve, 6_000))
93116

94117
// Ping this now so we can wait in parallel
95118
const callLater = await invokeFunction(ctx, { url: 'static/revalidate-slow' })
119+
expect(
120+
countOfBlobServerGetsForKey(ctx, '/static/revalidate-slow'),
121+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
122+
).toBe(1)
123+
ctx.blobServerGetSpy.mockClear()
96124

97125
// over 5 seconds since it was regenerated, so we should get stale response,
98126
// while fresh is generated in the background
@@ -104,9 +132,19 @@ describe('page router', () => {
104132
'cache-status': '"Next.js"; hit; fwd=stale',
105133
}),
106134
)
107-
expect(call3Date, 'the date was cached and is matching the initially regenerated one').toBe(
108-
call2Date,
109-
)
135+
expect(
136+
call3Date,
137+
'the rendered date was cached and is matching the initially regenerated one',
138+
).toBe(call2Date)
139+
expect(
140+
call3.headers['date'],
141+
'the date header is the same as the initially regenerated one',
142+
).toBe(call2.headers['date'])
143+
expect(
144+
countOfBlobServerGetsForKey(ctx, '/static/revalidate-automatic'),
145+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
146+
).toBe(1)
147+
ctx.blobServerGetSpy.mockClear()
110148

111149
// Slow revalidate should still be a hit, but the maxage should be updated
112150
const callLater2 = await invokeFunction(ctx, { url: 'static/revalidate-slow' })
@@ -119,6 +157,11 @@ describe('page router', () => {
119157
date: callLater.headers['date'],
120158
}),
121159
)
160+
expect(
161+
countOfBlobServerGetsForKey(ctx, '/static/revalidate-slow'),
162+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
163+
).toBe(1)
164+
ctx.blobServerGetSpy.mockClear()
122165

123166
// it does not wait for the cache.set so we have to manually wait here until the blob storage got populated
124167
await new Promise<void>((resolve) => setTimeout(resolve, 1000))
@@ -127,6 +170,10 @@ describe('page router', () => {
127170
const call4 = await invokeFunction(ctx, { url: 'static/revalidate-automatic' })
128171
const call4Date = load(call4.body)('[data-testid="date-now"]').text()
129172
expect(call4Date, 'the date was not cached').not.toBe(call3Date)
173+
expect(
174+
call4.headers['date'].toString().localeCompare(call3.headers['date'].toString()),
175+
'the date header of regenerated page is newer than initial stale page',
176+
).toBeGreaterThan(0)
130177
expect(call4.statusCode).toBe(200)
131178
expect(
132179
call4.headers,
@@ -136,6 +183,11 @@ describe('page router', () => {
136183
'cache-status': expect.stringMatching(/"Next.js"; hit/),
137184
}),
138185
)
186+
expect(
187+
countOfBlobServerGetsForKey(ctx, '/static/revalidate-automatic'),
188+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
189+
).toBe(1)
190+
ctx.blobServerGetSpy.mockClear()
139191
})
140192
})
141193

@@ -168,6 +220,11 @@ describe('app router', () => {
168220
'netlify-cdn-cache-control': 's-maxage=5, stale-while-revalidate=31536000',
169221
}),
170222
)
223+
expect(
224+
countOfBlobServerGetsForKey(ctx, '/posts/1'),
225+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
226+
).toBe(1)
227+
ctx.blobServerGetSpy.mockClear()
171228

172229
// test a prerendered page without TTL
173230
const post2 = await invokeFunction(ctx, { url: '/' })
@@ -179,6 +236,11 @@ describe('app router', () => {
179236
'netlify-cdn-cache-control': 's-maxage=31536000, stale-while-revalidate=31536000',
180237
}),
181238
)
239+
expect(
240+
countOfBlobServerGetsForKey(ctx, '/index'),
241+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
242+
).toBe(1)
243+
ctx.blobServerGetSpy.mockClear()
182244

183245
expect(await ctx.blobStore.get(encodeBlobKey('/posts/3'))).toBeNull()
184246
// this page is not pre-rendered and should result in a cache miss
@@ -193,7 +255,7 @@ describe('app router', () => {
193255

194256
// wait to have a stale page
195257
await new Promise<void>((resolve) => setTimeout(resolve, 6_000))
196-
// after the dynamic call of `posts/3` it should be in cache, not this is after the timout as the cache set happens async
258+
// after the dynamic call of `posts/3` it should be in cache, note this is after the timeout as the cache set happens async
197259
expect(await ctx.blobStore.get(encodeBlobKey('/posts/3'))).not.toBeNull()
198260

199261
const stale = await invokeFunction(ctx, { url: 'posts/1' })
@@ -204,6 +266,11 @@ describe('app router', () => {
204266
'cache-status': '"Next.js"; hit; fwd=stale',
205267
}),
206268
)
269+
expect(
270+
countOfBlobServerGetsForKey(ctx, '/posts/1'),
271+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
272+
).toBe(1)
273+
ctx.blobServerGetSpy.mockClear()
207274
// it should've been regenerated in the background after the first call
208275
// so the date should be different
209276
expect(staleDate, 'the date was cached and is matching the initial one').not.toBe(post1Date)
@@ -216,11 +283,20 @@ describe('app router', () => {
216283
const cachedDate = load(cached.body)('[data-testid="date-now"]').text()
217284
expect(cached.statusCode).toBe(200)
218285
expect(cachedDate, 'the date is not stale').not.toBe(staleDate)
286+
expect(
287+
cached.headers['date'].toString().localeCompare(post1.headers['date'].toString()),
288+
'the date header of regenerated page is newer than initial stale page',
289+
).toBeGreaterThan(0)
219290
expect(cached.headers, 'a cache hit after dynamically regenerating the stale page').toEqual(
220291
expect.objectContaining({
221292
'cache-status': '"Next.js"; hit',
222293
}),
223294
)
295+
expect(
296+
countOfBlobServerGetsForKey(ctx, '/posts/1'),
297+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
298+
).toBe(1)
299+
ctx.blobServerGetSpy.mockClear()
224300
})
225301
})
226302

@@ -257,10 +333,12 @@ describe('route', () => {
257333
})
258334
expect(blobEntry).not.toBeNull()
259335

336+
ctx.blobServerGetSpy.mockClear()
337+
260338
// test the first invocation of the route - we should get stale response while fresh is generated in the background
261339
const call1 = await invokeFunction(ctx, { url: '/api/revalidate-handler' })
262340
const call1Body = JSON.parse(call1.body)
263-
const call1Time = call1Body.time
341+
const call1Time = call1Body.time as string
264342
expect(call1.statusCode).toBe(200)
265343
expect(call1Body).toMatchObject({
266344
data: expect.objectContaining({
@@ -274,12 +352,18 @@ describe('route', () => {
274352
}),
275353
)
276354

355+
expect(
356+
countOfBlobServerGetsForKey(ctx, '/api/revalidate-handler'),
357+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
358+
).toBe(1)
359+
ctx.blobServerGetSpy.mockClear()
360+
277361
// it does not wait for the cache.set so we have to manually wait here until the blob storage got populated
278362
await new Promise<void>((resolve) => setTimeout(resolve, 1000))
279363

280364
const call2 = await invokeFunction(ctx, { url: '/api/revalidate-handler' })
281365
const call2Body = JSON.parse(call2.body)
282-
const call2Time = call2Body.time
366+
const call2Time = call2Body.time as string
283367
expect(call2.statusCode).toBe(200)
284368
expect(call2Body).toMatchObject({
285369
data: expect.objectContaining({
@@ -292,7 +376,16 @@ describe('route', () => {
292376
'cache-status': '"Next.js"; hit',
293377
}),
294378
)
295-
expect(call2Time, 'the date is new').not.toBe(call1Time)
379+
expect(call2Time.localeCompare(call1Time), 'the rendered date is new').toBeGreaterThan(0)
380+
expect(
381+
call2.headers['date'].toString().localeCompare(call1.headers['date'].toString()),
382+
'the date header of regenerated route is newer than initial stale route',
383+
).toBeGreaterThan(0)
384+
expect(
385+
countOfBlobServerGetsForKey(ctx, '/api/revalidate-handler'),
386+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
387+
).toBe(1)
388+
ctx.blobServerGetSpy.mockClear()
296389

297390
// wait to have a stale route again
298391
await new Promise<void>((resolve) => setTimeout(resolve, 8_000))
@@ -308,9 +401,18 @@ describe('route', () => {
308401
}),
309402
)
310403
expect(
311-
call2Time,
312-
'the date is the old one on the stale route, while the refresh is happening in the background',
313-
).toBe(call3Time)
404+
call3Time,
405+
'the rendered date is the old one on the stale route, while the refresh is happening in the background',
406+
).toBe(call2Time)
407+
expect(call3.headers['date'], 'the date header is the same on stale route').toBe(
408+
call2.headers['date'],
409+
)
410+
411+
expect(
412+
countOfBlobServerGetsForKey(ctx, '/api/revalidate-handler'),
413+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
414+
).toBe(1)
415+
ctx.blobServerGetSpy.mockClear()
314416

315417
// it does not wait for the cache.set so we have to manually wait here until the blob storage got populated
316418
await new Promise<void>((resolve) => setTimeout(resolve, 1000))
@@ -325,6 +427,15 @@ describe('route', () => {
325427
'cache-status': '"Next.js"; hit',
326428
}),
327429
)
328-
expect(call4Time, 'the date is new').not.toBe(call3Time)
430+
expect(call4Time.localeCompare(call3Time), 'the rendered date is new').toBeGreaterThan(0)
431+
expect(
432+
call4.headers['date'].toString().localeCompare(call3.headers['date'].toString()),
433+
'the date header of regenerated route is newer than previously stale route',
434+
).toBeGreaterThan(0)
435+
expect(
436+
countOfBlobServerGetsForKey(ctx, '/api/revalidate-handler'),
437+
'should only try to get value once from blob store (date calculation should not trigger additional blobs.get)',
438+
).toBe(1)
439+
ctx.blobServerGetSpy.mockClear()
329440
})
330441
})

‎tests/integration/request-context.test.ts

+234
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
import { getLogger } from 'lambda-local'
2+
import { v4 } from 'uuid'
3+
import { beforeEach, describe, expect, test, vi } from 'vitest'
4+
import {
5+
createFixture,
6+
invokeFunction,
7+
runPlugin,
8+
type FixtureTestContext,
9+
} from '../utils/fixture.js'
10+
import {
11+
countOfBlobServerGetsForKey,
12+
encodeBlobKey,
13+
generateRandomObjectID,
14+
startMockBlobStore,
15+
} from '../utils/helpers.js'
16+
import cjsMock from 'mock-require'
17+
import type { CacheHandler } from 'next/dist/server/lib/incremental-cache/index.js'
18+
import { SERVER_HANDLER_NAME } from '../../src/build/plugin-context.js'
19+
import { realpathSync } from 'node:fs'
20+
import { join } from 'node:path'
21+
22+
// Disable the verbose logging of the lambda-local runtime
23+
getLogger().level = 'alert'
24+
25+
beforeEach<FixtureTestContext>(async (ctx) => {
26+
// set for each test a new deployID and siteID
27+
ctx.deployID = generateRandomObjectID()
28+
ctx.siteID = v4()
29+
vi.stubEnv('SITE_ID', ctx.siteID)
30+
vi.stubEnv('DEPLOY_ID', ctx.deployID)
31+
vi.stubEnv('NETLIFY_PURGE_API_TOKEN', 'fake-token')
32+
// hide debug logs in tests
33+
// vi.spyOn(console, 'debug').mockImplementation(() => {})
34+
await startMockBlobStore(ctx)
35+
cjsMock.stopAll()
36+
})
37+
38+
function mockCacheHandlerWithPausing(ctx: FixtureTestContext) {
39+
const locks = new Map<string, (unpause: () => void) => void>()
40+
async function waitForCacheHandlerGetAndPause(key: string) {
41+
return new Promise<() => void>((resolve) => {
42+
locks.set(key, resolve)
43+
})
44+
}
45+
46+
const cacheHandlerPath = realpathSync(
47+
join(ctx.functionDist, SERVER_HANDLER_NAME, `dist/run/handlers/cache.cjs`),
48+
)
49+
const CacheHandler = require(cacheHandlerPath).default
50+
class MockedCacheHandler extends CacheHandler {
51+
async get(...args: Parameters<CacheHandler['get']>): ReturnType<CacheHandler['get']> {
52+
const getWasCalled = locks.get(args[0])
53+
54+
if (getWasCalled) {
55+
await new Promise<void>((resolve) => {
56+
getWasCalled(resolve)
57+
})
58+
}
59+
const result = await super.get(...args)
60+
61+
return result
62+
}
63+
}
64+
65+
cjsMock(cacheHandlerPath, MockedCacheHandler)
66+
67+
return {
68+
waitForCacheHandlerGetAndPause,
69+
}
70+
}
71+
72+
function spyOnRequestContext(ctx: FixtureTestContext) {
73+
const requestContextPath = realpathSync(
74+
join(ctx.functionDist, SERVER_HANDLER_NAME, `dist/run/handlers/request-context.cjs`),
75+
)
76+
const RequestContextModule = require(requestContextPath)
77+
const mockedRequestContextModule = {
78+
...RequestContextModule,
79+
getRequestContext: vi.fn(RequestContextModule.getRequestContext),
80+
}
81+
82+
cjsMock(requestContextPath, mockedRequestContextModule)
83+
84+
return mockedRequestContextModule.getRequestContext
85+
}
86+
87+
describe('request-context does NOT leak between concurrent requests', () => {
88+
test<FixtureTestContext>('pages router', async (ctx) => {
89+
await createFixture('page-router', ctx)
90+
await runPlugin(ctx)
91+
92+
const getRequestContextSpy = spyOnRequestContext(ctx)
93+
const { waitForCacheHandlerGetAndPause } = mockCacheHandlerWithPausing(ctx)
94+
95+
// setup we want to ensure that request context is not shared between concurrent requests
96+
// so we will artificially update `lastModified` timestamps for 2 tested routes
97+
// so they can be used for assertions
98+
const mockedDateForRevalidateAutomatic = 'Wed, 01 Jan 2020 00:00:00 GMT'
99+
const mockedDateForRevalidateSlow = 'Fri, 01 Jan 2021 00:00:00 GMT'
100+
101+
await ctx.blobStore.setJSON(encodeBlobKey('/static/revalidate-automatic'), {
102+
...(await ctx.blobStore.get(encodeBlobKey('/static/revalidate-automatic'), {
103+
type: 'json',
104+
})),
105+
lastModified: new Date(mockedDateForRevalidateAutomatic).getTime(),
106+
})
107+
108+
await ctx.blobStore.setJSON(encodeBlobKey('/static/revalidate-slow'), {
109+
...(await ctx.blobStore.get(encodeBlobKey('/static/revalidate-slow'), {
110+
type: 'json',
111+
})),
112+
lastModified: new Date(mockedDateForRevalidateSlow).getTime(),
113+
})
114+
115+
ctx.blobServerGetSpy.mockClear()
116+
117+
const waitForCacheHandlerGetAndPausePromise =
118+
waitForCacheHandlerGetAndPause('/static/revalidate-slow')
119+
const slowCallPromise = invokeFunction(ctx, {
120+
url: 'static/revalidate-slow',
121+
})
122+
123+
// let /static/revalidate-slow get to the point of getting response cache and pause before actually doing it
124+
// at this point we didn't yet acquired request context in CacheHandler, so if it's leaking
125+
// the previous invocation should update wrong context and result should be that
126+
// we hit blobs fallback to set date
127+
expect(getRequestContextSpy).toHaveBeenCalledTimes(0)
128+
129+
const unpauseSlowCall = await waitForCacheHandlerGetAndPausePromise
130+
131+
// start concurrent request that will finish while first request is paused
132+
const fastCall = await invokeFunction(ctx, {
133+
url: 'static/revalidate-automatic',
134+
})
135+
136+
// fastCall finished completely so it should have acquired request context
137+
expect(getRequestContextSpy).toHaveBeenCalledTimes(1)
138+
expect(getRequestContextSpy).toHaveNthReturnedWith(1, {
139+
responseCacheGetLastModified: new Date(mockedDateForRevalidateAutomatic).getTime(),
140+
responseCacheKey: '/static/revalidate-automatic',
141+
})
142+
143+
// second request finished - now we can unpause the first one
144+
unpauseSlowCall()
145+
146+
const slowCall = await slowCallPromise
147+
// slowCall finished completely so it should have acquired request context
148+
expect(getRequestContextSpy).toHaveBeenCalledTimes(2)
149+
expect(getRequestContextSpy).toHaveNthReturnedWith(2, {
150+
responseCacheGetLastModified: new Date(mockedDateForRevalidateSlow).getTime(),
151+
responseCacheKey: '/static/revalidate-slow',
152+
})
153+
154+
expect(slowCall.headers['date']).toBe(mockedDateForRevalidateSlow)
155+
expect(fastCall.headers['date']).toBe(mockedDateForRevalidateAutomatic)
156+
157+
// blobs were not used to generate date (we only get blobs inside CacheHandler)
158+
expect(countOfBlobServerGetsForKey(ctx, '/static/revalidate-slow')).toBe(1)
159+
expect(countOfBlobServerGetsForKey(ctx, '/static/revalidate-automatic')).toBe(1)
160+
})
161+
162+
test<FixtureTestContext>('app router', async (ctx) => {
163+
await createFixture('server-components', ctx)
164+
await runPlugin(ctx)
165+
166+
const getRequestContextSpy = spyOnRequestContext(ctx)
167+
const { waitForCacheHandlerGetAndPause } = mockCacheHandlerWithPausing(ctx)
168+
169+
// setup we want to ensure that request context is not shared between concurrent requests
170+
// so we will artificially update `lastModified` timestamps for 2 tested routes
171+
// so they can be used for assertions
172+
const mockedDateForStaticFetch1 = 'Wed, 01 Jan 2020 00:00:00 GMT'
173+
const mockedDateForStaticFetch2 = 'Fri, 01 Jan 2021 00:00:00 GMT'
174+
175+
await ctx.blobStore.setJSON(encodeBlobKey('/static-fetch/1'), {
176+
...(await ctx.blobStore.get(encodeBlobKey('/static-fetch/1'), {
177+
type: 'json',
178+
})),
179+
lastModified: new Date(mockedDateForStaticFetch1).getTime(),
180+
})
181+
182+
await ctx.blobStore.setJSON(encodeBlobKey('/static-fetch/2'), {
183+
...(await ctx.blobStore.get(encodeBlobKey('/static-fetch/2'), {
184+
type: 'json',
185+
})),
186+
lastModified: new Date(mockedDateForStaticFetch2).getTime(),
187+
})
188+
189+
ctx.blobServerGetSpy.mockClear()
190+
191+
const waitForCacheHandlerGetAndPausePromise = waitForCacheHandlerGetAndPause('/static-fetch/2')
192+
const slowCallPromise = invokeFunction(ctx, {
193+
url: 'static-fetch/2',
194+
})
195+
196+
// let /static-fetch/2 get to the point of getting response cache and pause before actually doing it
197+
// at this point we didn't yet acquired request context in CacheHandler, so if it's leaking
198+
// the previous invocation should update wrong context and result should be that
199+
// we hit blobs fallback to set date
200+
expect(getRequestContextSpy).toHaveBeenCalledTimes(0)
201+
202+
const unpauseSlowCall = await waitForCacheHandlerGetAndPausePromise
203+
204+
// start concurrent request that will finish while first request is paused
205+
const fastCall = await invokeFunction(ctx, {
206+
url: 'static-fetch/1',
207+
})
208+
209+
// fastCall finished completely so it should have acquired request context
210+
expect(getRequestContextSpy).toHaveBeenCalledTimes(1)
211+
expect(getRequestContextSpy).toHaveNthReturnedWith(1, {
212+
responseCacheGetLastModified: new Date(mockedDateForStaticFetch1).getTime(),
213+
responseCacheKey: '/static-fetch/1',
214+
})
215+
216+
// second request finished - now we can unpause the first one
217+
unpauseSlowCall()
218+
219+
const slowCall = await slowCallPromise
220+
// slowCall finished completely so it should have acquired request context
221+
expect(getRequestContextSpy).toHaveBeenCalledTimes(2)
222+
expect(getRequestContextSpy).toHaveNthReturnedWith(2, {
223+
responseCacheGetLastModified: new Date(mockedDateForStaticFetch2).getTime(),
224+
responseCacheKey: '/static-fetch/2',
225+
})
226+
227+
expect(slowCall.headers['date']).toBe(mockedDateForStaticFetch2)
228+
expect(fastCall.headers['date']).toBe(mockedDateForStaticFetch1)
229+
230+
// blobs were not used to generate date (we only get blobs inside CacheHandler)
231+
expect(countOfBlobServerGetsForKey(ctx, '/static-fetch/2')).toBe(1)
232+
expect(countOfBlobServerGetsForKey(ctx, '/static-fetch/1')).toBe(1)
233+
})
234+
})

‎tests/utils/fixture.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { BlobsServer, type getStore } from '@netlify/blobs'
2-
import { TestContext, assert, vi } from 'vitest'
2+
import { TestContext, assert, vi, MockInstance } from 'vitest'
33

44
import { type NetlifyPluginConstants, type NetlifyPluginOptions } from '@netlify/build'
55
import { bundle, serve } from '@netlify/edge-bundler'
@@ -32,6 +32,7 @@ export interface FixtureTestContext extends TestContext {
3232
blobStoreHost: string
3333
blobStorePort: number
3434
blobServer: BlobsServer
35+
blobServerGetSpy: MockInstance<Parameters<BlobsServer['get']>, ReturnType<BlobsServer['get']>>
3536
blobStore: ReturnType<typeof getStore>
3637
functionDist: string
3738
edgeFunctionPort: number

‎tests/utils/helpers.ts

+24
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ export const startMockBlobStore = async (ctx: FixtureTestContext) => {
6363
directory: await mkdtemp(join(tmpdir(), 'netlify-next-runtime-blob-')),
6464
})
6565
await ctx.blobServer.start()
66+
ctx.blobServerGetSpy = vi.spyOn(ctx.blobServer, 'get')
6667
ctx.blobStoreHost = `localhost:${port}`
6768
ctx.blobStorePort = port
6869
vi.stubEnv('NETLIFY_BLOBS_CONTEXT', createBlobContext(ctx))
@@ -92,6 +93,29 @@ export const getBlobEntries = async (ctx: FixtureTestContext) => {
9293
return blobs
9394
}
9495

96+
function getBlobServerGets(ctx: FixtureTestContext) {
97+
const isString = (arg: unknown): arg is string => typeof arg === 'string'
98+
return ctx.blobServerGetSpy.mock.calls
99+
.map(([request]) => {
100+
if (typeof request.url !== 'string') {
101+
return undefined
102+
}
103+
// request url is /:siteID/:deployID/:key for get
104+
// /:siteID/:deployID for list
105+
// we only want gets
106+
const urlSegments = request.url.split('/')
107+
if (urlSegments.length === 4) {
108+
return decodeBlobKey(urlSegments[3])
109+
}
110+
return undefined
111+
})
112+
.filter(isString)
113+
}
114+
115+
export function countOfBlobServerGetsForKey(ctx: FixtureTestContext, key: string) {
116+
return getBlobServerGets(ctx).reduce((acc, curr) => (curr === key ? acc + 1 : acc), 0)
117+
}
118+
95119
/**
96120
* Converts a string to base64 blob key
97121
*/

0 commit comments

Comments
 (0)
Please sign in to comment.