-
Notifications
You must be signed in to change notification settings - Fork 48
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
HTTP HEAD responses are missing content-length header #220
Comments
Thank you for the issue. This should be resolved. We can calculate and add a content length for the response to the diff --git a/src/hono-base.ts b/src/hono-base.ts
index 3e040da5..bbb8198e 100644
--- a/src/hono-base.ts
+++ b/src/hono-base.ts
@@ -396,8 +396,12 @@ class Hono<E extends Env = Env, S extends Schema = {}, BasePath extends string =
): Response | Promise<Response> {
// Handle HEAD method
if (method === 'HEAD') {
- return (async () =>
- new Response(null, await this.#dispatch(request, executionCtx, env, 'GET')))()
+ return (async () => {
+ const res = await this.#dispatch(request, executionCtx, env, 'GET')
+ const length = (await res.arrayBuffer()).byteLength
+ res.headers.set('Content-Length', length.toString())
+ return new Response(null, res)
+ })()
}
const path = this.getPath(request, { env }) I think we don't have a way to know the content length without |
Thank you @yusukebe! I appreciate you looking at this issue so quickly, and I have been really enjoying using Hono for the first time. |
Just in case anyone else has the same problem, and can't wait until the next release of @hono/node-server, here is my work-around... import { serve } from '@hono/node-server' // 1.13.7
import { Hono } from 'hono' // 4.6.14
const app = new Hono()
/*
Workaround to add a Content-Length header to HEAD responses.
Notes:
- there is no app.head() or app.on('head'), so this must be done with app.get()
- can't check for c.res.headers.has('content-length'), as this is added just before sending
(i.e. the GET response is _also_ missing a content-length header at this step)
- calling next() first means that the following statements are applied after other middleware
has been applied and the handler has been run (i.e. a response exists).
*/
app.get('*', async (c, next) => {
await next()
if (c.req.method === 'HEAD') {
const body = await c.res.text()
c.header('Content-Length', Buffer.byteLength(body).toString())
}
})
app.get('/', (c) => {
return c.json({ message: 'Hello Node.js!' })
// Note: the workaround does not handle the case where there is an empty response body.
// A ```Transfer-Encoding: chunked``` header is added for GET but not for HEAD.
// return c.body(null) // <-- this returns different headers for GET vs HEAD
})
serve(app)
/*
GET request: ```curl -v http://localhost:3000/```
< HTTP/1.1 200 OK
< content-type: application/json
< Content-Length: 28
< Date: Tue, 17 Dec 2024 20:25:25 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
HEAD request: ```curl -v --head http://localhost:3000/```
< HTTP/1.1 200 OK
< content-length: 28
< content-type: application/json
< Date: Tue, 17 Dec 2024 20:26:06 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
*/ |
Hi @StuartMoncrieff This is certainly a difficult problem. If we resolve this issue naively, there is a possibility that it will be loaded even though it doesn't need to be loaded in a use case like serve-static. (Although node-server has already dealt with this problem.) Even with a normal get request, there will be more overhead than there is now, but if we handle it with "src/context.ts", the following method might be good. diff --git a/src/context.ts b/src/context.ts
index 86124e00..43b3220f 100644
--- a/src/context.ts
+++ b/src/context.ts
@@ -272,6 +272,18 @@ const setHeaders = (headers: Headers, map: Record<string, string> = {}) => {
return headers
}
+const buildResponse = (data?: Data | null, init?: ResponseInit): Response => {
+ const res = new Response(data, init)
+ if (!res.headers.has('Content-Length')) {
+ if (typeof data === 'string') {
+ res.headers.set('Content-Length', String(data.length))
+ } else if (data instanceof ArrayBuffer) {
+ res.headers.set('Content-Length', String(data.byteLength))
+ }
+ }
+ return res
+}
+
export class Context<
// eslint-disable-next-line @typescript-eslint/no-explicit-any
E extends Env = any,
@@ -386,7 +398,7 @@ export class Context<
*/
get res(): Response {
this.#isFresh = false
- return (this.#res ||= new Response('404 Not Found', { status: 404 }))
+ return (this.#res ||= buildResponse('404 Not Found', { status: 404 }))
}
/**
@@ -415,7 +427,7 @@ export class Context<
} catch (e) {
if (e instanceof TypeError && e.message.includes('immutable')) {
// `_res` is immutable (probably a response from a fetch API), so retry with a new response.
- this.res = new Response(_res.body, {
+ this.res = buildResponse(_res.body, {
headers: _res.headers,
status: _res.status,
})
@@ -630,7 +642,7 @@ export class Context<
): Response {
// Optimized
if (this.#isFresh && !headers && !arg && this.#status === 200) {
- return new Response(data, {
+ return buildResponse(data, {
headers: this.#preparedHeaders,
})
}
@@ -648,7 +660,7 @@ export class Context<
})
}
const headers = setHeaders(header, this.#preparedHeaders)
- return new Response(data, {
+ return buildResponse(data, {
headers,
status: arg.status ?? this.#status,
})
@@ -683,7 +695,7 @@ export class Context<
}
}
- return new Response(data, {
+ return buildResponse(data, {
status,
headers: this.#headers,
})
@@ -744,7 +756,7 @@ export class Context<
if (!this.#preparedHeaders) {
if (this.#isFresh && !headers && !arg) {
// @ts-expect-error `Response` due to missing some types-only keys
- return new Response(text)
+ return buildResponse(text)
}
this.#preparedHeaders = {}
}
@@ -842,7 +854,7 @@ export class Context<
* ```
*/
notFound = (): Response | Promise<Response> => {
- this.#notFoundHandler ??= () => new Response()
+ this.#notFoundHandler ??= () => buildResponse()
return this.#notFoundHandler(this)
}
}
@StuartMoncrieff RFC 9110 states the following.
As is written here, the fact that Content-Length is not included is also assumed in the RFC, so I think that there may be no need to include Content-Length in a HEAD request if it causes overhead or significantly reduces performance in some cases. |
Hi @usualoma . Thank you for the idea. If we follow the RFC 9110 @usualoma mentioned, it would be enough to add the Content-Length on the user side, so there would be no need to do it on the Hono-side. However, it will little confusing, if you want to add the Content-Length for the app.get('/', (c) => {
c.header('Content-Length', '5') // Add Content-Length for both GET and HEAD
return c.text('index')
}) |
The HTTP headers returned by a HEAD request should be the same as the headers returned by a GET request for the same URL. The @hono/node-server does not include a
Content-Length
header in responses when the HEAD method is used.Reference: (https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD)
The text was updated successfully, but these errors were encountered: