Conversation
Use a shared, guarded HTTP/1 _contentLength setter for plain header records and responses without custom headers. Let Node serialize the length while avoiding caller-header mutation and redundant header processing. Retain explicit-length fallbacks for HTTP/2, HEAD, bodyless statuses and incompatible native state. Cover shared and frozen records, empty and multibyte bodies, byte arrays, Blobs and strict length checking. Keep the original Hono dependency and verify plain-record serialization independently of how c.json() constructs its headers.
Add local serialization, Hono pipeline and HTTP benchmarks with rotating process order, natural GC and recorded bundle hashes. Support comparisons against both the original mutation and the preceding native-length implementation. Record the installed Hono version and actual header representation without changing dependencies. Include diagnostic scripts and command-line summaries for repeatable measurements.
324c9ee to
3cb4d6c
Compare
|
Hi @yusukebe, I was a bit surprised to see roughly a 10% improvement in the pipeline benchmark without socket I/O. The difference is much smaller when HTTP I/O is included, but the results suggest there may still be a small performance benefit. This relies fairly heavily on Node.js internals, though I think that’s an acceptable trade-off for node-server. |
There was a problem hiding this comment.
I think the Hono version this benchmark affects is v4.13 with honojs/hono#5122, or later. So it's better to update the lockfile and devDependencies of hono (it uses 4.12.8)
|
Hey @usualoma Thanks. This is an interesting approach and great! Let's go with it. One thing I noticed is that if you set invalid header values and diff --git a/test/content-length.test.ts b/test/content-length.test.ts
index 7c59e4d..5507b1d 100644
--- a/test/content-length.test.ts
+++ b/test/content-length.test.ts
@@ -1,7 +1,10 @@
import { Hono } from 'hono'
+import { once } from 'node:events'
import { createServer } from 'node:http'
import type { IncomingMessage, ServerResponse } from 'node:http'
import { createServer as createHttp2Server } from 'node:http2'
+import { connect as connectNet } from 'node:net'
+import type { AddressInfo } from 'node:net'
import { getRequestListener } from '../src/listener'
import { defaultContentType } from '../src/response'
import { createAdaptorServer } from '../src/server'
@@ -193,4 +196,44 @@ describe('automatic Content-Length compatibility', () => {
expect(res.headers.get('content-length')).toBe('00005')
expect(await res.text()).toBe('hello')
})
+
+ it.each([
+ {
+ name: 'an invalid header value',
+ response: () => new Response('hi', { headers: { 'x-bad': 'a\r\nb' } }),
+ },
+ {
+ name: 'an invalid status code',
+ response: () => new Response('hi', { status: 1000, headers: { 'x-ok': '1' } }),
+ },
+ ])('does not leak a stale length into the 500 response after $name', async ({ response }) => {
+ const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+ const server = createServer(getRequestListener(async () => response()))
+ server.listen(0, '127.0.0.1')
+ await once(server, 'listening')
+ try {
+ const { port } = server.address() as AddressInfo
+ const raw = await new Promise<string>((resolve, reject) => {
+ const chunks: Buffer[] = []
+ const socket = connectNet(port, '127.0.0.1', () => {
+ socket.write('GET / HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n')
+ })
+ socket.on('data', (chunk) => chunks.push(chunk))
+ socket.on('end', () => resolve(Buffer.concat(chunks).toString('latin1')))
+ socket.on('error', reject)
+ })
+ const separator = raw.indexOf('\r\n\r\n')
+ const head = raw.slice(0, separator)
+ const body = raw.slice(separator + 4)
+ expect(head).toMatch(/^HTTP\/1\.1 500 /)
+ expect(body).toContain('Error: ')
+ const contentLength = head.match(/^content-length: (\d+)$/im)?.[1]
+ if (contentLength !== undefined) {
+ expect(Number(contentLength)).toBe(Buffer.byteLength(body, 'latin1'))
+ }
+ } finally {
+ await new Promise<void>((resolve) => server.close(() => resolve()))
+ consoleSpy.mockRestore()
+ }
+ })
}) |
This PR fixes #400 while also optimizing Content-Length handling on the HTTP/1 response fast paths.
I confirmed that copying the header record, as suggested in #400, also resolves the issue. In local benchmarks with Hono 4.13.8, that approach showed a performance regression on Node 20, but no clear regression on Node 22 or 24. It remains a reasonable option to consider.
#401 already implements that approach. However, the original reporter offered, “Happy to open a PR if the approach looks right.” If we decide to proceed with the copying approach, I think we should respect that offer, close #401 for now, and give the reporter the opportunity to submit their PR.
Implementation
This PR uses Node’s internal
_contentLengthfield to let Node serialize the length without copying caller-owned header records. The same mechanism also handles responses without custom headers, avoiding explicit Content-Length header processing.This deliberately depends on Node.js implementation details, including private response fields whose behavior is not guaranteed by the public API. node-server has made similar implementation-dependent optimizations several times before. Given the measured improvements in the response fast paths, I consider the gains substantial enough to justify this dependency.
Compatibility guards retain explicit-length fallbacks for HTTP/2 and other cases where automatic serialization cannot preserve the existing behavior.
The regression tests check both response correctness and whether the intended fast paths are used. These tests run in the existing Node.js CI matrix, so incompatible changes should be detected promptly when an affected Node.js release is tested.
Benchmarks
The final implementation was compared with the baseline (
82ba34e6, v2.1.0) and a plain-header-copy variant on an Apple M5 Pro (arm64, macOS).Each variant ran in five independent processes, with rotating execution order. Each process performed 300,000 warm-up requests followed by five samples of 200,000 requests, using natural GC.
The table reports median time per request in nanoseconds; lower is better. This includes Hono dispatch, response creation, and Node’s header serialization. Socket I/O is excluded.
c.json()c.json()c.json()c.text()c.text()c.text()In these runs, copying increased
c.json()processing time by 33.89% on Node 20, while showing no slowdown on Node 22 or 24. The copy column measures the benchmark harness’s plain-record-copy implementation, rather than the exact #401 implementation.A control case using JSON with seven additional headers exercises the
Headersconversion path. Its median changes were −1.42%, −0.72%, and −0.47% on Node 20, 22, and 24 respectively, with overlapping baseline and candidate ranges. These small differences do not establish an improvement on that path.