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

feat(image-optimizer): use previously cached image cache entry if upstream image is identical #67257

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from
Open
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
118 changes: 91 additions & 27 deletions packages/next/src/server/image-optimizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ import type { NextConfigComplete } from './config-shared'
import { createRequestResponseMocks } from './lib/mock-request'
import type { NextUrlWithParsedQuery } from './request-meta'
import type {
CachedImageValue,
IncrementalCacheEntry,
IncrementalCacheItem,
IncrementalCacheValue,
} from './response-cache'
import { sendEtagResponse } from './send-payload'
Expand All @@ -34,7 +36,7 @@ const SVG = 'image/svg+xml'
const ICO = 'image/x-icon'
const TIFF = 'image/tiff'
const BMP = 'image/bmp'
const CACHE_VERSION = 3
const CACHE_VERSION = 4
const ANIMATABLE_TYPES = [WEBP, PNG, GIF]
const VECTOR_TYPES = [SVG]
const BLUR_IMG_SIZE = 8 // should match `next-image-loader`
Expand Down Expand Up @@ -98,8 +100,12 @@ export function getHash(items: (string | number | Buffer)[]) {
hash.update(item)
}
}
// See https://en.wikipedia.org/wiki/Base64#Filenames
return hash.digest('base64').replace(/\//g, '-')
// See https://en.wikipedia.org/wiki/Base64#URL_applications
return hash.digest('base64url')
Copy link
Author

Choose a reason for hiding this comment

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

Not necessary for the PR, but while the CACHE_VERSION is anyways changing, it doesn't hurt? 🤷

}

export function getImageEtag(image: Buffer) {
return getHash([image])
}

async function writeToCacheDir(
Expand All @@ -108,9 +114,13 @@ async function writeToCacheDir(
maxAge: number,
expireAt: number,
buffer: Buffer,
etag: string
etag: string,
upstreamEtag: string
) {
const filename = join(dir, `${maxAge}.${expireAt}.${etag}.${extension}`)
const filename = join(
dir,
`${maxAge}.${expireAt}.${etag}.${upstreamEtag}.${extension}`
)

await promises.rm(dir, { recursive: true, force: true }).catch(() => {})

Expand Down Expand Up @@ -332,7 +342,8 @@ export class ImageOptimizerCache {
const now = Date.now()

for (const file of files) {
const [maxAgeSt, expireAtSt, etag, extension] = file.split('.', 4)
const [maxAgeSt, expireAtSt, etag, upstreamEtag, extension] =
file.split('.', 5)
const buffer = await promises.readFile(join(cacheDir, file))
const expireAt = Number(expireAtSt)
const maxAge = Number(maxAgeSt)
Expand All @@ -343,6 +354,7 @@ export class ImageOptimizerCache {
etag,
buffer,
extension,
upstreamEtag,
},
revalidateAfter:
Math.max(maxAge, this.nextConfig.images.minimumCacheTTL) * 1000 +
Expand Down Expand Up @@ -383,7 +395,8 @@ export class ImageOptimizerCache {
revalidate,
expireAt,
value.buffer,
value.etag
value.etag,
value.upstreamEtag
)
} catch (err) {
Log.error(`Failed to write image to cache ${cacheKey}`, err)
Expand Down Expand Up @@ -438,6 +451,21 @@ export function getMaxAge(str: string | null | undefined): number {
return 0
}

export function shouldUsePreviouslyCachedEntry(
upstreamImage: ImageUpstream,
previousCacheEntry: IncrementalCacheItem | undefined
): previousCacheEntry is NonNullable<IncrementalCacheItem> & {
value: CachedImageValue
} {
return (
previousCacheEntry?.value?.kind === 'IMAGE' &&
// If the cached image is optimized
previousCacheEntry.value.upstreamEtag !== previousCacheEntry.value.etag &&
// and the upstream etag is the same as the previous cache entry's
getImageEtag(upstreamImage.buffer) === previousCacheEntry.value.upstreamEtag
)
}

export async function optimizeImage({
buffer,
contentType,
Expand Down Expand Up @@ -508,7 +536,6 @@ export async function fetchExternalImage(href: string): Promise<ImageUpstream> {
const buffer = Buffer.from(await res.arrayBuffer())
const contentType = res.headers.get('Content-Type')
const cacheControl = res.headers.get('Cache-Control')

return { buffer, contentType, cacheControl }
}

Expand Down Expand Up @@ -566,40 +593,73 @@ export async function imageOptimizer(
'dangerouslyAllowSVG' | 'minimumCacheTTL'
>
},
isDev: boolean | undefined
): Promise<{ buffer: Buffer; contentType: string; maxAge: number }> {
isDev: boolean | undefined,
previousCacheEntry?: IncrementalCacheItem
): Promise<{
buffer: Buffer
contentType: string
maxAge: number
etag: string
upstreamEtag: string
}> {
const { href, quality, width, mimeType } = paramsResult
const upstreamBuffer = imageUpstream.buffer
const { buffer: upstreamBuffer } = imageUpstream
const maxAge = getMaxAge(imageUpstream.cacheControl)

if (shouldUsePreviouslyCachedEntry(imageUpstream, previousCacheEntry)) {
// Return the cached optimized image
return {
buffer: previousCacheEntry.value.buffer,
contentType: paramsResult.mimeType,
maxAge: previousCacheEntry.curRevalidate || maxAge,
etag: previousCacheEntry.value.etag,
upstreamEtag: previousCacheEntry.value.upstreamEtag,
}
}

const upstreamType =
detectContentType(upstreamBuffer) ||
imageUpstream.contentType?.toLowerCase().trim()

if (upstreamType) {
if (
upstreamType.startsWith('image/svg') &&
!nextConfig.images.dangerouslyAllowSVG
) {
Log.error(
`The requested resource "${href}" has type "${upstreamType}" but dangerouslyAllowSVG is disabled`
)
throw new ImageError(
400,
'"url" parameter is valid but image type is not allowed'
)
}
if (
upstreamType?.startsWith('image/svg') &&
!nextConfig.images.dangerouslyAllowSVG
) {
Log.error(
`The requested resource "${href}" has type "${upstreamType}" but dangerouslyAllowSVG is disabled`
)
throw new ImageError(
400,
'"url" parameter is valid but image type is not allowed'
)
}

const upstreamEtag = getImageEtag(upstreamBuffer)

if (upstreamType) {
if (ANIMATABLE_TYPES.includes(upstreamType) && isAnimated(upstreamBuffer)) {
Log.warnOnce(
`The requested resource "${href}" is an animated image so it will not be optimized. Consider adding the "unoptimized" property to the <Image>.`
)
return { buffer: upstreamBuffer, contentType: upstreamType, maxAge }
return {
buffer: upstreamBuffer,
contentType: upstreamType,
maxAge,
etag: upstreamEtag,
upstreamEtag,
}
}
if (VECTOR_TYPES.includes(upstreamType)) {
// We don't warn here because we already know that "dangerouslyAllowSVG"
// was enabled above, therefore the user explicitly opted in.
// If we add more VECTOR_TYPES besides SVG, perhaps we could warn for those.
return { buffer: upstreamBuffer, contentType: upstreamType, maxAge }
return {
buffer: upstreamBuffer,
contentType: upstreamType,
maxAge,
etag: upstreamEtag,
upstreamEtag,
}
}
if (!upstreamType.startsWith('image/') || upstreamType.includes(',')) {
Log.error(
Expand Down Expand Up @@ -653,6 +713,8 @@ export async function imageOptimizer(
buffer: optimizedBuffer,
contentType,
maxAge: Math.max(maxAge, nextConfig.images.minimumCacheTTL),
etag: getImageEtag(optimizedBuffer),
upstreamEtag,
}
} else {
throw new ImageError(500, 'Unable to optimize buffer')
Expand All @@ -664,6 +726,8 @@ export async function imageOptimizer(
buffer: upstreamBuffer,
contentType: upstreamType,
maxAge: nextConfig.images.minimumCacheTTL,
etag: upstreamEtag,
upstreamEtag,
}
} else {
throw new ImageError(
Expand Down Expand Up @@ -734,14 +798,14 @@ export function sendResponse(
url: string,
extension: string,
buffer: Buffer,
etag: string,
isStatic: boolean,
xCache: XCacheHeader,
imagesConfig: ImageConfigComplete,
maxAge: number,
isDev: boolean
) {
const contentType = getContentType(extension)
const etag = getHash([buffer])
Copy link
Author

Choose a reason for hiding this comment

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

If we are storing the etag already, shouldn't we use that instead of recalculating the etag? This can be reverted to use getImageTag.

const result = setResponseHeaders(
req,
res,
Expand Down
30 changes: 18 additions & 12 deletions packages/next/src/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ import { removeTrailingSlash } from '../shared/lib/router/utils/remove-trailing-
import { getNextPathnameInfo } from '../shared/lib/router/utils/get-next-pathname-info'
import { getCloneableBody } from './body-streams'
import { checkIsOnDemandRevalidate } from './api-utils'
import ResponseCache from './response-cache'
import ResponseCache, { type IncrementalCacheItem } from './response-cache'
import { IncrementalCache } from './lib/incremental-cache'
import { normalizeAppPath } from '../shared/lib/router/utils/app-paths'

Expand Down Expand Up @@ -591,8 +591,15 @@ export default class NextNodeServer extends BaseServer<
protected async imageOptimizer(
req: NodeNextRequest,
res: NodeNextResponse,
paramsResult: import('./image-optimizer').ImageParamsResult
): Promise<{ buffer: Buffer; contentType: string; maxAge: number }> {
paramsResult: import('./image-optimizer').ImageParamsResult,
previousCacheEntry?: IncrementalCacheItem
): Promise<{
buffer: Buffer
contentType: string
maxAge: number
upstreamEtag: string
etag: string
}> {
if (process.env.NEXT_MINIMAL) {
throw new Error(
'invariant: imageOptimizer should not be called in minimal mode'
Expand Down Expand Up @@ -632,7 +639,8 @@ export default class NextNodeServer extends BaseServer<
imageUpstream,
paramsResult,
this.nextConfig,
this.renderOpts.dev
this.renderOpts.dev,
previousCacheEntry
)
}
}
Expand Down Expand Up @@ -836,7 +844,7 @@ export default class NextNodeServer extends BaseServer<
nextConfig: this.nextConfig,
})

const { getHash, sendResponse, ImageError } =
const { sendResponse, ImageError } =
require('./image-optimizer') as typeof import('./image-optimizer')

if (!this.imageResponseCache) {
Expand Down Expand Up @@ -869,20 +877,17 @@ export default class NextNodeServer extends BaseServer<
require('./serve-static') as typeof import('./serve-static')
const cacheEntry = await this.imageResponseCache.get(
cacheKey,
async () => {
const { buffer, contentType, maxAge } = await this.imageOptimizer(
req,
res,
paramsResult
)
const etag = getHash([buffer])
async (_, prevEntry) => {
const { buffer, contentType, maxAge, upstreamEtag, etag } =
await this.imageOptimizer(req, res, paramsResult, prevEntry)

return {
value: {
kind: 'IMAGE',
buffer,
etag,
extension: getExtension(contentType) as string,
upstreamEtag,
},
revalidate: maxAge,
}
Expand All @@ -904,6 +909,7 @@ export default class NextNodeServer extends BaseServer<
paramsResult.href,
cacheEntry.value.extension,
cacheEntry.value.buffer,
cacheEntry.value.etag,
paramsResult.isStatic,
cacheEntry.isMiss ? 'MISS' : cacheEntry.isStale ? 'STALE' : 'HIT',
imagesConfig,
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/server/response-cache/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export interface CachedRouteValue {
export interface CachedImageValue {
kind: 'IMAGE'
etag: string
upstreamEtag: string
buffer: Buffer
extension: string
isMiss?: boolean
Expand Down
Loading
Loading