v2: perf(response,listener): Response fast-paths and responseViaCache improvements#333
Open
GavinMeierSonos wants to merge 1 commit intohonojs:v2from
Open
Conversation
…rovements Response: - Extend InternalCache to support null body (redirects, 204s) - Override Response.json() to return a LightweightResponse so the listener fast-path (cacheKey check) is hit instead of falling through to ReadableStream - Override Response.redirect() with the same approach; null body avoids spurious content-type header on redirects Listener: - responseViaCache no-header fast-path: build the final header object in one shot per body type (string, Uint8Array, Blob, stream, null) to avoid V8 hidden-class transitions from mutating a single-key object - Fix null-body responses (redirects, 204s) getting a spurious content-type: text/plain header - Extract makeCloseHandler as a module-level factory so the close-listener closure is only allocated for async responses, not every request - Replace incomingEnded boolean tracking with incoming.readableEnded Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Member
|
This commit is too large to discuss, so could you please close it and split it into the following three parts, then submit them as separate pull requests? diff --git a/src/listener.ts b/src/listener.ts
index ed25513..9205035 100644
--- a/src/listener.ts
+++ b/src/listener.ts
@@ -70,10 +70,43 @@ const responseViaCache = async (
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let [status, body, header] = (res as any)[cacheKey] as InternalCache
- let hasContentLength = false
+ // Fast path: no custom headers — create the final header object in one shot
+ // (avoids shape transitions from mutating a single-key object).
if (!header) {
- header = { 'content-type': 'text/plain; charset=UTF-8' }
- } else if (header instanceof Headers) {
+ if (body == null) {
+ outgoing.writeHead(status, {})
+ outgoing.end()
+ } else if (typeof body === 'string') {
+ outgoing.writeHead(status, {
+ 'content-type': 'text/plain; charset=UTF-8',
+ 'Content-Length': Buffer.byteLength(body),
+ })
+ outgoing.end(body)
+ } else if (body instanceof Uint8Array) {
+ outgoing.writeHead(status, {
+ 'content-type': 'text/plain; charset=UTF-8',
+ 'Content-Length': body.byteLength,
+ })
+ outgoing.end(body)
+ } else if (body instanceof Blob) {
+ outgoing.writeHead(status, {
+ 'content-type': 'text/plain; charset=UTF-8',
+ 'Content-Length': body.size,
+ })
+ outgoing.end(new Uint8Array(await body.arrayBuffer()))
+ } else {
+ outgoing.writeHead(status, { 'content-type': 'text/plain; charset=UTF-8' })
+ flushHeaders(outgoing)
+ await writeFromReadableStream(body, outgoing)?.catch(
+ (e) => handleResponseError(e, outgoing) as undefined
+ )
+ }
+ ;(outgoing as OutgoingHasOutgoingEnded)[outgoingEnded]?.()
+ return
+ }
+
+ let hasContentLength = false
+ if (header instanceof Headers) {
hasContentLength = header.has('content-length')
header = buildOutgoingHttpHeaders(header)
} else if (Array.isArray(header)) {
@@ -101,7 +134,9 @@ const responseViaCache = async (
}
outgoing.writeHead(status, header)
- if (typeof body === 'string' || body instanceof Uint8Array) {
+ if (body == null) {
+ outgoing.end()
+ } else if (typeof body === 'string' || body instanceof Uint8Array) {
outgoing.end(body)
} else if (body instanceof Blob) {
outgoing.end(new Uint8Array(await body.arrayBuffer()))
diff --git a/src/response.ts b/src/response.ts
index ae9a59a..5d15c7c 100644
--- a/src/response.ts
+++ b/src/response.ts
@@ -9,7 +9,7 @@ export const cacheKey = Symbol('cache')
export type InternalCache = [
number,
- string | ReadableStream,
+ string | ReadableStream | null,
Record<string, string> | [string, string][] | Headers | OutgoingHttpHeaders | undefined,
]
interface LightResponse {
@@ -47,12 +47,14 @@ export class Response {
}
if (
+ body === null ||
+ body === undefined ||
typeof body === 'string' ||
typeof (body as ReadableStream)?.getReader !== 'undefined' ||
body instanceof Blob ||
body instanceof Uint8Array
) {
- ;(this as any)[cacheKey] = [init?.status || 200, body, headers || init?.headers]
+ ;(this as any)[cacheKey] = [init?.status || 200, body ?? null, headers || init?.headers]
}
}
diff --git a/src/response.ts b/src/response.ts
index 5d15c7c..7821024 100644
--- a/src/response.ts
+++ b/src/response.ts
@@ -99,3 +99,38 @@ export class Response {
})
Object.setPrototypeOf(Response, GlobalResponse)
Object.setPrototypeOf(Response.prototype, GlobalResponse.prototype)
+
+// Override Response.json() to return a LightweightResponse so the listener
+// fast-path (cacheKey check) is hit instead of falling through to ReadableStream reading.
+Object.defineProperty(Response, 'redirect', {
+ value: function redirect(url: string | URL, status = 302): Response {
+ if (![301, 302, 303, 307, 308].includes(status)) {
+ throw new RangeError('Invalid status code')
+ }
+ return new Response(null, {
+ status,
+ headers: { location: typeof url === 'string' ? url : url.href },
+ })
+ },
+ writable: true,
+ configurable: true,
+})
+
+Object.defineProperty(Response, 'json', {
+ value: function json(data?: unknown, init?: ResponseInit): Response {
+ const body = JSON.stringify(data)
+ const initHeaders = init?.headers
+ let headers: Record<string, string> | Headers
+ if (initHeaders) {
+ headers = new Headers(initHeaders)
+ if (!(headers as Headers).has('content-type')) {
+ ;(headers as Headers).set('content-type', 'application/json')
+ }
+ } else {
+ headers = { 'content-type': 'application/json' }
+ }
+ return new Response(body, { status: init?.status ?? 200, statusText: init?.statusText, headers })
+ },
+ writable: true,
+ configurable: true,
+})
diff --git a/test/response.test.ts b/test/response.test.ts
index e999e2f..f1e2a40 100644
--- a/test/response.test.ts
+++ b/test/response.test.ts
@@ -100,6 +100,84 @@ describe('Response', () => {
expect(await childResponse.text()).toEqual('HONO')
})
+ describe('Response.json', () => {
+ it('should return 200 with application/json content-type by default', () => {
+ const res = Response.json({ hello: 'world' })
+ expect(res.status).toBe(200)
+ expect(res.headers.get('content-type')).toBe('application/json')
+ })
+
+ it('should serialize data correctly', async () => {
+ const data = { hello: 'world', num: 42, arr: [1, 2, 3] }
+ const res = Response.json(data)
+ expect(await res.json()).toEqual(data)
+ })
+
+ it('should use custom status from init', () => {
+ const res = Response.json({ error: 'not found' }, { status: 404 })
+ expect(res.status).toBe(404)
+ })
+
+ it('should preserve statusText from init', () => {
+ const res = Response.json({ error: 'not found' }, { status: 404, statusText: 'Not Found' })
+ expect(res.statusText).toBe('Not Found')
+ })
+
+ it('should preserve custom content-type from init headers', () => {
+ const res = Response.json(
+ { data: 'test' },
+ { headers: { 'content-type': 'application/vnd.api+json' } }
+ )
+ expect(res.headers.get('content-type')).toBe('application/vnd.api+json')
+ })
+
+ it('should set application/json when init headers do not include content-type', () => {
+ const res = Response.json({ data: 'test' }, { headers: { 'x-custom': 'value' } })
+ expect(res.headers.get('content-type')).toBe('application/json')
+ expect(res.headers.get('x-custom')).toBe('value')
+ })
+
+ it('should return a LightweightResponse with cacheKey set for the fast path', () => {
+ const res = Response.json({ ok: true })
+ expect(res).toBeInstanceOf(LightweightResponse)
+ expect(cacheKey in res).toBe(true)
+ })
+
+ it('should throw for non-serializable data', () => {
+ const circ: Record<string, unknown> = {}
+ circ.self = circ
+ expect(() => Response.json(circ)).toThrow(TypeError)
+ })
+ })
+
+ describe('Response.redirect', () => {
+ it('should return a 302 redirect by default', () => {
+ const res = Response.redirect('https://example.com')
+ expect(res.status).toBe(302)
+ expect(res.headers.get('location')).toBe('https://example.com')
+ })
+
+ it('should use a custom redirect status', () => {
+ const res = Response.redirect('https://example.com/new', 301)
+ expect(res.status).toBe(301)
+ })
+
+ it('should accept a URL object', () => {
+ const res = Response.redirect(new URL('https://example.com/path'))
+ expect(res.headers.get('location')).toBe('https://example.com/path')
+ })
+
+ it('should throw for invalid status codes', () => {
+ expect(() => Response.redirect('https://example.com', 200)).toThrow(RangeError)
+ })
+
+ it('should return a LightweightResponse with cacheKey set for the fast path', () => {
+ const res = Response.redirect('https://example.com')
+ expect(res).toBeInstanceOf(LightweightResponse)
+ expect(cacheKey in res).toBe(true)
+ })
+ })
+
describe('Fallback to GlobalResponse object', () => {
it('Should return value from internal cache', () => {
const res = new Response('Hello! Node!')diff --git a/src/listener.ts b/src/listener.ts
index 9205035..08f06a9 100644
--- a/src/listener.ts
+++ b/src/listener.ts
@@ -26,6 +26,30 @@ type OutgoingHasOutgoingEnded = Http2ServerResponse & {
[outgoingEnded]?: () => void
}
+const makeCloseHandler = (
+ req: any,
+ incoming: IncomingMessage | Http2ServerRequest,
+ outgoing: ServerResponse | Http2ServerResponse,
+ needsBodyCleanup: boolean
+): (() => void) =>
+ () => {
+ if (incoming.errored) {
+ req[abortRequest](incoming.errored.toString())
+ } else if (!outgoing.writableFinished) {
+ req[abortRequest]('Client connection prematurely closed.')
+ }
+
+ if (needsBodyCleanup && !incoming.readableEnded) {
+ setTimeout(() => {
+ if (!incoming.readableEnded) {
+ setTimeout(() => {
+ incoming.destroy()
+ })
+ }
+ })
+ }
+ }
+
const handleRequestError = (): Response =>
new Response(null, {
status: 400,
@@ -273,19 +297,18 @@ export const getRequestListener = (
) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let res, req: any
+ let needsBodyCleanup = false
try {
// `fetchCallback()` requests a Request object, but global.Request is expensive to generate,
// so generate a pseudo Request object with only the minimum required information.
req = newRequest(incoming, options.hostname)
- let incomingEnded =
- !autoCleanupIncoming || incoming.method === 'GET' || incoming.method === 'HEAD'
- if (!incomingEnded) {
+ // For non-GET/HEAD requests, mark for body stream wrapping and H2 cleanup.
+ needsBodyCleanup =
+ autoCleanupIncoming && !(incoming.method === 'GET' || incoming.method === 'HEAD')
+ if (needsBodyCleanup) {
;(incoming as IncomingMessageWithWrapBodyStream)[wrapBodyStream] = true
- incoming.on('end', () => {
- incomingEnded = true
- })
if (incoming instanceof Http2ServerRequest) {
// a Http2ServerResponse instance requires additional processing on exit
@@ -293,11 +316,11 @@ export const getRequestListener = (
// when the state is incomplete
;(outgoing as OutgoingHasOutgoingEnded)[outgoingEnded] = () => {
// incoming is not consumed to the end
- if (!incomingEnded) {
+ if (!incoming.readableEnded) {
setTimeout(() => {
// in the case of a simple POST request, the cleanup process may be done automatically
- // and end is called at this point. At that point, nothing is done.
- if (!incomingEnded) {
+ // and readableEnded is true at this point. At that point, nothing is done.
+ if (!incoming.readableEnded) {
setTimeout(() => {
incoming.destroy()
// a Http2ServerResponse instance will not terminate without also calling outgoing.destroy()
@@ -310,42 +333,32 @@ export const getRequestListener = (
}
}
- // Detect if request was aborted.
- outgoing.on('close', () => {
- let abortReason: string | undefined
- if (incoming.errored) {
- abortReason = incoming.errored.toString()
- } else if (!outgoing.writableFinished) {
- abortReason = 'Client connection prematurely closed.'
- }
- if (abortReason !== undefined) {
- req[abortRequest](abortReason)
- }
-
- // incoming is not consumed to the end
- if (!incomingEnded) {
- setTimeout(() => {
- // in the case of a simple POST request, the cleanup process may be done automatically
- // and end is called at this point. At that point, nothing is done.
- if (!incomingEnded) {
- setTimeout(() => {
- incoming.destroy()
- })
- }
- })
- }
- })
-
res = fetchCallback(req, { incoming, outgoing } as HttpBindings) as
| Response
| Promise<Response>
if (cacheKey in res) {
- // synchronous, cacheable response
+ // Synchronous cacheable response — no close listener needed.
+ // No I/O events can fire between fetchCallback returning and responseViaCache
+ // completing, so abort detection is not needed here.
+ if (needsBodyCleanup && !incoming.readableEnded) {
+ // Handler returned without consuming the body; schedule destruction so
+ // the socket is freed even if the client never sends the rest of the body.
+ setTimeout(() => {
+ if (!incoming.readableEnded) {
+ setTimeout(() => incoming.destroy())
+ }
+ })
+ }
return responseViaCache(res as Response, outgoing)
}
+ // Async response — create and register close listener only now, avoiding
+ // closure allocation on the synchronous hot path.
+ outgoing.on('close', makeCloseHandler(req, incoming, outgoing, needsBodyCleanup))
} catch (e: unknown) {
if (!res) {
if (options.errorHandler) {
+ // Async error handler — register close listener so client disconnect aborts the signal.
+ if (req) outgoing.on('close', makeCloseHandler(req, incoming, outgoing, needsBodyCleanup))
res = await options.errorHandler(req ? e : toRequestError(e))
if (!res) {
return
diff --git a/test/listener.test.ts b/test/listener.test.ts
index b98026f..9a242d4 100644
--- a/test/listener.test.ts
+++ b/test/listener.test.ts
@@ -302,6 +302,82 @@ describe('Abort request', () => {
})
})
+describe('Abort request - error path', () => {
+ it('should abort request signal when client disconnects while async error handler is running after sync throw', async () => {
+ let capturedReq: Request | undefined
+ let resolveAborted: () => void
+ const abortedPromise = new Promise<void>((r) => {
+ resolveAborted = r
+ })
+
+ const fetchCallback = (req: Request) => {
+ capturedReq = req
+ req.signal.addEventListener('abort', () => resolveAborted())
+ throw new Error('sync error')
+ }
+
+ let resolveErrorHandlerStarted: () => void
+ const errorHandlerStarted = new Promise<void>((r) => {
+ resolveErrorHandlerStarted = r
+ })
+
+ const errorHandler = async () => {
+ resolveErrorHandlerStarted()
+ await new Promise<void>(() => {}) // never resolves — client will disconnect first
+ }
+
+ const requestListener = getRequestListener(fetchCallback, { errorHandler })
+ const server = createServer(requestListener)
+
+ try {
+ const req = request(server).get('/').end(() => {})
+ await errorHandlerStarted
+ req.abort()
+ await abortedPromise
+ expect(capturedReq?.signal.aborted).toBe(true)
+ } finally {
+ server.close()
+ }
+ })
+
+ it('should abort request signal when client disconnects while async error handler is running after async throw', async () => {
+ let capturedReq: Request | undefined
+ let resolveAborted: () => void
+ const abortedPromise = new Promise<void>((r) => {
+ resolveAborted = r
+ })
+
+ const fetchCallback = async (req: Request) => {
+ capturedReq = req
+ req.signal.addEventListener('abort', () => resolveAborted())
+ throw new Error('async error')
+ }
+
+ let resolveErrorHandlerStarted: () => void
+ const errorHandlerStarted = new Promise<void>((r) => {
+ resolveErrorHandlerStarted = r
+ })
+
+ const errorHandler = async () => {
+ resolveErrorHandlerStarted()
+ await new Promise<void>(() => {}) // never resolves — client will disconnect first
+ }
+
+ const requestListener = getRequestListener(fetchCallback, { errorHandler })
+ const server = createServer(requestListener)
+
+ try {
+ const req = request(server).get('/').end(() => {})
+ await errorHandlerStarted
+ req.abort()
+ await abortedPromise
+ expect(capturedReq?.signal.aborted).toBe(true)
+ } finally {
+ server.close()
+ }
+ })
+})
+
describe('overrideGlobalObjects', () => {
const fetchCallback = vi.fn()
|
Member
|
What do you think of these changes? Are there any changes that should be merged? I think we should not accept all of them. For example, optimizing |
Member
|
I believe each change has areas that need fixing (bugs or oversights), but I generally think it’s fine to incorporate the changes as a whole.
|
Author
Member
|
Hi @GavinMeierSonos, Thank you for your reply! Sorry, I realized later that
These changes rely on the first change that allows setting Thank you for your time and effort. |
Member
|
@GavinMeierSonos Thanks!
Please! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Response:
InternalCacheto supportnullbody (redirects, 204s)Response.json()to return aLightweightResponseso the listener fast-path (cacheKeycheck) is hit instead of falling through toReadableStreamreadingResponse.redirect()with the same approach;nullbody avoids a spuriouscontent-type: text/plainheader on redirectsListener:
responseViaCacheno-header fast-path: build the final header object in one shot per body type (string,Uint8Array,Blob, stream,null) to avoid V8 hidden-class transitions from mutating a single-key objectnull-body responses (redirects, 204s) getting a spuriouscontent-type: text/plainheadermakeCloseHandleras a module-level factory so the close-listener closure is only allocated for async responses, not every requestincomingEndedboolean tracking withincoming.readableEndedChanges
src/response.tssrc/listener.tstest/response.test.tstest/listener.test.ts