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.

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

Jump to bottom

Add request deduplication to image optimizer #24000

Merged
merged 3 commits into from
Apr 12, 2021
Merged

Add request deduplication to image optimizer #24000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17bb196
add image optimization deduplication
shuding Apr 12, 2021
8f33102
improve test; add try-finally to wrap the entire processor
shuding Apr 12, 2021
dce241b
Merge branch 'canary' into fix-dedupe-next-image-requests
kodiakhq[bot] Apr 12, 2021
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
314 changes: 170 additions & 144 deletions packages/next/next-server/server/image-optimizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const MODERN_TYPES = [/* AVIF, */ WEBP]
const ANIMATABLE_TYPES = [WEBP, PNG, GIF]
const VECTOR_TYPES = [SVG]

const inflightRequests = new Map<string, Promise<undefined>>()

export async function imageOptimizer(
server: Server,
req: IncomingMessage,
Expand Down Expand Up @@ -138,175 +140,199 @@ export async function imageOptimizer(
const hashDir = join(imagesDir, hash)
const now = Date.now()

if (await fileExists(hashDir, 'directory')) {
const files = await promises.readdir(hashDir)
for (let file of files) {
const [prefix, etag, extension] = file.split('.')
const expireAt = Number(prefix)
const contentType = getContentType(extension)
const fsPath = join(hashDir, file)
if (now < expireAt) {
res.setHeader('Cache-Control', 'public, max-age=0, must-revalidate')
if (sendEtagResponse(req, res, etag)) {
// If there're concurrent requests hitting the same resource and it's still
// being optimized, wait before accessing the cache.
if (inflightRequests.has(hash)) {
await inflightRequests.get(hash)
}
let dedupeResolver: (val?: PromiseLike<undefined>) => void
inflightRequests.set(
hash,
new Promise((resolve) => (dedupeResolver = resolve))
)

try {
if (await fileExists(hashDir, 'directory')) {
const files = await promises.readdir(hashDir)
for (let file of files) {
const [prefix, etag, extension] = file.split('.')
const expireAt = Number(prefix)
const contentType = getContentType(extension)
const fsPath = join(hashDir, file)
if (now < expireAt) {
res.setHeader('Cache-Control', 'public, max-age=0, must-revalidate')
if (sendEtagResponse(req, res, etag)) {
return { finished: true }
}
if (contentType) {
res.setHeader('Content-Type', contentType)
}
createReadStream(fsPath).pipe(res)
return { finished: true }
} else {
await promises.unlink(fsPath)
}
if (contentType) {
res.setHeader('Content-Type', contentType)
}
createReadStream(fsPath).pipe(res)
return { finished: true }
} else {
await promises.unlink(fsPath)
}
}
}

let upstreamBuffer: Buffer
let upstreamType: string | null
let maxAge: number
let upstreamBuffer: Buffer
let upstreamType: string | null
let maxAge: number

if (isAbsolute) {
const upstreamRes = await fetch(href)

if (isAbsolute) {
const upstreamRes = await fetch(href)
if (!upstreamRes.ok) {
res.statusCode = upstreamRes.status
res.end('"url" parameter is valid but upstream response is invalid')
return { finished: true }
}

if (!upstreamRes.ok) {
res.statusCode = upstreamRes.status
res.end('"url" parameter is valid but upstream response is invalid')
return { finished: true }
}
upstreamBuffer = Buffer.from(await upstreamRes.arrayBuffer())
upstreamType = upstreamRes.headers.get('Content-Type')
maxAge = getMaxAge(upstreamRes.headers.get('Cache-Control'))
} else {
try {
const _req: any = {
headers: req.headers,
method: req.method,
url: href,
}
const resBuffers: Buffer[] = []
const mockRes: any = new Stream.Writable()

res.statusCode = upstreamRes.status
upstreamBuffer = Buffer.from(await upstreamRes.arrayBuffer())
upstreamType = upstreamRes.headers.get('Content-Type')
maxAge = getMaxAge(upstreamRes.headers.get('Cache-Control'))
} else {
try {
const _req: any = {
headers: req.headers,
method: req.method,
url: href,
}
const resBuffers: Buffer[] = []
const mockRes: any = new Stream.Writable()
mockRes.write = (chunk: Buffer | string) => {
resBuffers.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk))
}
mockRes._write = (chunk: Buffer | string) => {
mockRes.write(chunk)
}

mockRes.write = (chunk: Buffer | string) => {
resBuffers.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk))
}
mockRes._write = (chunk: Buffer | string) => {
mockRes.write(chunk)
const mockHeaders: Record<string, string | string[]> = {}

mockRes.writeHead = (_status: any, _headers: any) =>
Object.assign(mockHeaders, _headers)
mockRes.getHeader = (name: string) => mockHeaders[name.toLowerCase()]
mockRes.getHeaders = () => mockHeaders
mockRes.getHeaderNames = () => Object.keys(mockHeaders)
mockRes.setHeader = (name: string, value: string | string[]) =>
(mockHeaders[name.toLowerCase()] = value)
mockRes._implicitHeader = () => {}
mockRes.finished = false
mockRes.statusCode = 200

await server.getRequestHandler()(
_req,
mockRes,
nodeUrl.parse(href, true)
)
res.statusCode = mockRes.statusCode

upstreamBuffer = Buffer.concat(resBuffers)
upstreamType = mockRes.getHeader('Content-Type')
maxAge = getMaxAge(mockRes.getHeader('Cache-Control'))
} catch (err) {
res.statusCode = 500
res.end('"url" parameter is valid but upstream response is invalid')
return { finished: true }
}

const mockHeaders: Record<string, string | string[]> = {}

mockRes.writeHead = (_status: any, _headers: any) =>
Object.assign(mockHeaders, _headers)
mockRes.getHeader = (name: string) => mockHeaders[name.toLowerCase()]
mockRes.getHeaders = () => mockHeaders
mockRes.getHeaderNames = () => Object.keys(mockHeaders)
mockRes.setHeader = (name: string, value: string | string[]) =>
(mockHeaders[name.toLowerCase()] = value)
mockRes._implicitHeader = () => {}
mockRes.finished = false
mockRes.statusCode = 200

await server.getRequestHandler()(_req, mockRes, nodeUrl.parse(href, true))
res.statusCode = mockRes.statusCode

upstreamBuffer = Buffer.concat(resBuffers)
upstreamType = mockRes.getHeader('Content-Type')
maxAge = getMaxAge(mockRes.getHeader('Cache-Control'))
} catch (err) {
res.statusCode = 500
res.end('"url" parameter is valid but upstream response is invalid')
return { finished: true }
}
}

const expireAt = maxAge * 1000 + now
const expireAt = maxAge * 1000 + now

if (upstreamType) {
const vector = VECTOR_TYPES.includes(upstreamType)
const animate =
ANIMATABLE_TYPES.includes(upstreamType) && isAnimated(upstreamBuffer)
if (vector || animate) {
await writeToCacheDir(hashDir, upstreamType, expireAt, upstreamBuffer)
sendResponse(req, res, upstreamType, upstreamBuffer)
return { finished: true }
}
if (upstreamType) {
const vector = VECTOR_TYPES.includes(upstreamType)
const animate =
ANIMATABLE_TYPES.includes(upstreamType) && isAnimated(upstreamBuffer)
if (vector || animate) {
await writeToCacheDir(hashDir, upstreamType, expireAt, upstreamBuffer)
sendResponse(req, res, upstreamType, upstreamBuffer)
return { finished: true }
}

// If upstream type is not a valid image type, return 400 error.
if (!upstreamType.startsWith('image/')) {
res.statusCode = 400
res.end("The requested resource isn't a valid image.")
return { finished: true }
// If upstream type is not a valid image type, return 400 error.
if (!upstreamType.startsWith('image/')) {
res.statusCode = 400
res.end("The requested resource isn't a valid image.")
return { finished: true }
}
}
}

let contentType: string
let contentType: string

if (mimeType) {
contentType = mimeType
} else if (upstreamType?.startsWith('image/') && getExtension(upstreamType)) {
contentType = upstreamType
} else {
contentType = JPEG
}
if (mimeType) {
contentType = mimeType
} else if (
upstreamType?.startsWith('image/') &&
getExtension(upstreamType)
) {
contentType = upstreamType
} else {
contentType = JPEG
}

try {
const orientation = await getOrientation(upstreamBuffer)
try {
const orientation = await getOrientation(upstreamBuffer)

const operations: Operation[] = []
const operations: Operation[] = []

if (orientation === Orientation.RIGHT_TOP) {
operations.push({ type: 'rotate', numRotations: 1 })
} else if (orientation === Orientation.BOTTOM_RIGHT) {
operations.push({ type: 'rotate', numRotations: 2 })
} else if (orientation === Orientation.LEFT_BOTTOM) {
operations.push({ type: 'rotate', numRotations: 3 })
} else {
// TODO: support more orientations
// eslint-disable-next-line @typescript-eslint/no-unused-vars
// const _: never = orientation
}
if (orientation === Orientation.RIGHT_TOP) {
operations.push({ type: 'rotate', numRotations: 1 })
} else if (orientation === Orientation.BOTTOM_RIGHT) {
operations.push({ type: 'rotate', numRotations: 2 })
} else if (orientation === Orientation.LEFT_BOTTOM) {
operations.push({ type: 'rotate', numRotations: 3 })
} else {
// TODO: support more orientations
// eslint-disable-next-line @typescript-eslint/no-unused-vars
// const _: never = orientation
}

operations.push({ type: 'resize', width })

let optimizedBuffer: Buffer | undefined
//if (contentType === AVIF) {
//} else
if (contentType === WEBP) {
optimizedBuffer = await processBuffer(
upstreamBuffer,
operations,
'webp',
quality
)
} else if (contentType === PNG) {
optimizedBuffer = await processBuffer(
upstreamBuffer,
operations,
'png',
quality
)
} else if (contentType === JPEG) {
optimizedBuffer = await processBuffer(
upstreamBuffer,
operations,
'jpeg',
quality
)
}
operations.push({ type: 'resize', width })

let optimizedBuffer: Buffer | undefined
//if (contentType === AVIF) {
//} else
if (contentType === WEBP) {
optimizedBuffer = await processBuffer(
upstreamBuffer,
operations,
'webp',
quality
)
} else if (contentType === PNG) {
optimizedBuffer = await processBuffer(
upstreamBuffer,
operations,
'png',
quality
)
} else if (contentType === JPEG) {
optimizedBuffer = await processBuffer(
upstreamBuffer,
operations,
'jpeg',
quality
)
}

if (optimizedBuffer) {
await writeToCacheDir(hashDir, contentType, expireAt, optimizedBuffer)
sendResponse(req, res, contentType, optimizedBuffer)
} else {
throw new Error('Unable to optimize buffer')
if (optimizedBuffer) {
await writeToCacheDir(hashDir, contentType, expireAt, optimizedBuffer)
sendResponse(req, res, contentType, optimizedBuffer)
} else {
throw new Error('Unable to optimize buffer')
}
} catch (error) {
sendResponse(req, res, upstreamType, upstreamBuffer)
}
} catch (error) {
sendResponse(req, res, upstreamType, upstreamBuffer)
}

return { finished: true }
return { finished: true }
} finally {
// Make sure to remove the hash in the end.
dedupeResolver!()
inflightRequests.delete(hash)
}
}

async function writeToCacheDir(
Expand Down
30 changes: 30 additions & 0 deletions test/integration/image-optimizer/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from 'next-test-utils'
import isAnimated from 'next/dist/compiled/is-animated'
import { join } from 'path'
import { createHash } from 'crypto'

jest.setTimeout(1000 * 60 * 2)

Expand Down Expand Up @@ -509,6 +510,35 @@ function runTests({ w, isDev, domains }) {
expect(res.status).toBe(400)
expect(await res.text()).toBe("The requested resource isn't a valid image.")
})

it('should handle concurrent requests', async () => {
const query = { url: '/test.png', w, q: 80 }
const opts = { headers: { accept: 'image/webp,*/*' } }
const [res1, res2] = await Promise.all([
fetchViaHTTP(appPort, '/_next/image', query, opts),
fetchViaHTTP(appPort, '/_next/image', query, opts),
])
expect(res1.status).toBe(200)
expect(res2.status).toBe(200)
expect(res1.headers.get('Content-Type')).toBe('image/webp')
expect(res2.headers.get('Content-Type')).toBe('image/webp')
await expectWidth(res1, w)
await expectWidth(res2, w)

// There should be only one image created in the cache directory.
const hashItems = [2, '/test.png', w, 80, 'image/webp']
const hash = createHash('sha256')
for (let item of hashItems) {
if (typeof item === 'number') hash.update(String(item))
else {
hash.update(item)
}
}
const hashDir = hash.digest('base64').replace(/\//g, '-')
const dir = join(imagesDir, hashDir)
const files = await fs.readdir(dir)
expect(files.length).toBe(1)
})
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check the cache folder to ensure only one entry was created here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a change. Since we have many return statements in the optimizer (and itself can potentially throw), I wrapped it with try-finally. You can review with "Hide whitespace changes". 🙏

}

describe('Image Optimizer', () => {
Expand Down