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

HTTP HEAD responses are missing content-length header #220

Open
StuartMoncrieff opened this issue Dec 17, 2024 · 5 comments
Open

HTTP HEAD responses are missing content-length header #220

StuartMoncrieff opened this issue Dec 17, 2024 · 5 comments

Comments

@StuartMoncrieff
Copy link

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.

The HEAD HTTP method requests the metadata of a resource in the form of headers that the server would have sent if the GET method was used instead. This method can be used in cases where a URL might produce a large download, for example, a HEAD request can read the Content-Length header to check the file size before downloading the file with a GET.

Reference: (https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD)

import { serve } from '@hono/node-server' // 1.13.7
import { Hono } from 'hono' // 4.6.14
import assert from 'node:assert/strict'

const app = new Hono()

// The app.get() method matches both GET and HEAD requests.
app.get('/', (c) => {
  return c.json({ message: 'Hello Node.js!' })
})

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 03:54:56 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< 
< {"message":"Hello Node.js!"}

*/
const getResponse = await fetch('http://localhost:3000/', { method: 'GET' })
assert(getResponse.body !== null, 'GET response should have a response body')
assert(getResponse.headers.has('content-length') === true, 'GET response should have a content-length header')

/*
HEAD request: ```curl -v --head http://localhost:3000/```

The HEAD response is missing a content-length header!

< HTTP/1.1 200 OK
< content-type: application/json
< Date: Tue, 17 Dec 2024 03:55:30 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5

*/
const headResponse = await fetch('http://localhost:3000/', { method: 'HEAD' })
assert(headResponse.body === null, 'HEAD response should *not* have a response body')
assert(headResponse.headers.has('content-length') === true, 'HEAD response should have a content-length header') // AssertionError!
@yusukebe
Copy link
Member

Hi @StuartMoncrieff

Thank you for the issue. This should be resolved.

@usualoma

We can calculate and add a content length for the response to the HEAD request like the following, but the pain point is it should await for loading the entire body content:

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 await. What do you think of this? Should we use this way using await to add the content length for the response of the HEAD response?

@StuartMoncrieff
Copy link
Author

Thank you @yusukebe! I appreciate you looking at this issue so quickly, and I have been really enjoying using Hono for the first time.

@StuartMoncrieff
Copy link
Author

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

*/

@usualoma
Copy link
Member

Hi @StuartMoncrieff
Thanks for the report!

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.)
https://github.com/honojs/node-server/blob/main/src/serve-static.ts#L138-L142

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
By the way, are there any use cases where this behaviour actually causes a problem?

RFC 9110 states the following.

The server SHOULD send the same header fields in response to a HEAD request as it would have sent if the request method had been GET. However, a server MAY omit header fields for which a value is determined only while generating the content. For example, some servers buffer a dynamic response to GET until a minimum amount of data is generated so that they can more efficiently delimit it small responses or make late decisions with regard to content selection. Such a response to GET might contain Content-Length and Vary fields, for example, that are not generated within a HEAD response. These minor inconsistencies are considered preferable to generating and discarding the content for a HEAD request, since HEAD is usually requested for the sake of efficiency.
https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.2

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.

@yusukebe
Copy link
Member

Hi @usualoma . Thank you for the idea. buildResponse looks good, but although the overhead seems slight, it is significant for Hono. This is because it takes a short time to set the header in the Response object. Therefore, Hono is designed so that it does not calculate the Content-Length.

@StuartMoncrieff

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 HEAD request, you will need to do it in the app.get().

app.get('/', (c) => {
  c.header('Content-Length', '5') // Add Content-Length for both GET and HEAD
  return c.text('index')
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants